Skip to content

Conversation

@dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Nov 4, 2022

This is an attempt to change CI output for regressions building ports which are independent, i.e. not affected by the changes in a PR, vs. regression in ports which are affected by changes.
(I try to avoid the word "baseline" here.)

Simplified scenario

vcpkg "parent" state:
Port A.
Port B.
Port C depends on A and on B.
CI successful.

After a vcpkg PR modifying C.
Port A.
Port B.
Port C depends on A and on B[non-default-feature].
CI fails on B[non-default-feature] which wasn't built before:

REGRESSION: B[non-default-feature] failed with BUILD_FAILED. If expected, add ...

In less simple cases with more dependencies, contributors and reviewers are often confused:
Reviewer: "You broke B!"
Contributor: "I didn't change it at all!".
With a modified tagging of the B[non-default-feauture] failure, both parties can recognize the problem more easily:

REGRESSION: Independent B[non-default-feature] failed with BUILD_FAILED.

I started this change a while ago, and I need some time to recover commands for testing the issue and change. But maybe it is already useful to discuss at this point. Actual changes start after "Merge branch 'command-ci' into HEAD" (0403be0).

dg0yt added 7 commits November 3, 2022 07:32
From a ci command perspective, errors are regressions by default.
The baseline file is only for skips, expected failures and unexpected
cascades.
If a port is not changed with regard to its parent hash
but fails to build, it is a real baseline regression.
@dg0yt dg0yt force-pushed the ci-find-regression branch from f0abfd6 to c52f9e3 Compare November 4, 2022 08:37
@dg0yt dg0yt marked this pull request as ready for review November 8, 2022 06:40
@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 8, 2022

WIP, but looking for feedback.

@BillyONeal
Copy link
Member

In the future if you run the format and generate-message-map targets it will fix what CI complains about for you locally. I think nobody looked at this PR because it was Red.

@BillyONeal
Copy link
Member

I am support of improving the output to better distinguish such output. However, I think it would be better to describe more directly what happened. Now that we have some idea of what the user changed anyway. I would prefer to include output indicating whether the thing is in the 'cone of dependencies' or 'cone of destruction' and include example causes. For example:

Given:
A
B
C -> A, B
D -> C

DEPENDENCY-REGRESSION: b[non-default-feature] failed...
REGRESSION: c[...] failed...
DEPENDENT-REGRESSION: d[...] failed...

(If there were any DEPENDENCY-REGRESSIONs:)
A DEPENDENCY-REGRESSION means that a port upon which a port you changed in this pull request failed to build. This is usually a problem with the dependency itself rather than with what your PR changed. This can be caused by one or more of:
* The dependency is currently broken in vcpkg's curated registry. Check a recent ci run for similar failures.
* Your change tried to build a combination of features which has never been tested in the dependency which is broken in some way.
* The dependency is broken by some combination of its dependencies being installed first, or not.


(If there were any DEPENDENT-REGRESSIONs:)
A DEPENDENT-REGRESSION means that a port that depends on the port you changed failed to build. This usually indicates a problem with the port you changed breaking some contract your port's customers are using. We usually patch downstream dependencies to work before taking such updates.

Of course those blobs probably count as documentation or at least documentation-adjacent stuff. /cc @AugP @ras0219-msft

Implementation wise I'm not sure whether 'user-requested-install' is the right metric to track here?

@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 17, 2022

Thanks @BillyONeal

DEPENDENCY-REGRESSION: b[non-default-feature] failed...
REGRESSION: c[...] failed...
DEPENDENT-REGRESSION: d[...] failed...

I would have chosen three cases, too. But at the moment, we cannot generally distinguish REGRESSION from DEPENDENT-REGRESSION. This would require to track source file (hash) changes independent from ABI hashes of dependencies.

(We can detect regressions as not being DEPENDENT-REGRESSION if their dependencies didn't change. But some changed port may have changed dependencies.)

Implementation wise I'm not sure whether 'user-requested-install' is the right metric to track here?

I am aware of the mismatch. OTOH for the current two-case scenario, it maps quite well to what is present for regular vcpkg install.

@BillyONeal
Copy link
Member

I am happy that this is an improvement on the status quo even if it isn't what we really want.

# Conflicts:
#	include/vcpkg/base/messages.h
#	src/vcpkg/base/messages.cpp
@BillyONeal BillyONeal merged commit 8ed7a6e into microsoft:main Apr 4, 2023
@BillyONeal
Copy link
Member

Thanks!

@dg0yt dg0yt deleted the ci-find-regression branch April 4, 2023 07:00
@BillyONeal
Copy link
Member

Does this actually do anything? Every spec in ports/ gets added to our initial plan so they should all have user_requested_install set to true...

@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 24, 2025

Does this actually do anything?

I'm sure it did make a difference two years ago, but there is no test.

Every spec in ports/ gets added to our initial plan so they should all have user_requested_install set to true...

I can't dig deeper now, but IIRC "user_requested_install" was the indicator for the particular case of ABI unhash unchanged (= independent), cached artifact missing.
This might be rare now when port modifications must pass the feature test. But in 2024, some feature combinations were only invoked for modifying independent ports, via the specific cone of destruction.

@BillyONeal
Copy link
Member

I'm sure it did make a difference two years ago, but there is no test.

Maybe this had something to do with it:

it->request_type = RequestType::USER_REQUESTED;

@BillyONeal
Copy link
Member

but there is no test.

This is actually why I went back here. I'm trying to make sure I don't break this in #1821 / #1822 but am having difficulty in figuring out how to specifically trigger the difference in order to write such a test.

@BillyONeal
Copy link
Member

Given that searching for "REGRESSION: Independent" gives no results (I would have expected over 2 years someone to have copied that into a PR), I can't seem to repro this failure, and there are no tests, I'm going to remove this code path in #1821 / #1822 . At a minimum, detecting the condition described in the description would require comparing the ABI hash of ports with their ABI hash by name... and this isn't doing that. parent_hashes in this PR are still being morally smashed into a vector<ABI>.

I am also improving the report at the top a bit:

image

@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 4, 2025

vcpkg tool...
pass was broken until yesterday.
--parent-hashes isn't tested until #1829.
And yes, this doesn't work at the moment. Testing would be like #1830.

@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 4, 2025

And with the test, restoring the capability was easy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants