-
Notifications
You must be signed in to change notification settings - Fork 335
Don't include features from skipped ports in CI runs. #1822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't include features from skipped ports in CI runs. #1822
Conversation
…icrosoft/vcpkg#47735 OK, so here's the situation: `ci.baseline.txt` has: ```console # Missing system libraries qtwayland:arm64-osx=skip qtwayland:x64-osx=skip ``` In microsoft/vcpkg#47735, `qtwayland` depends on `qtbase[wayland]`, which implicates the same missing system libraries. This causes `qtbase` to contain the feature `wayland` when we try to build it, even though `qtwayland` is skipped.
|
Alternative to #1821 |
|
Before the fix, the test output is something like: Detecting compiler hash for triplet x86-windows...
Compiler found: C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/14.44.35207/bin/Hostx64/x86/cl.exe
skipped-features:x86-windows: *: 54830d1c353d4c11b0fe77014a1b7973eba052729c77fc1597c2eb5506df875c
skipped-depends:x86-windows: skip: d5aa71e67e593c1c94e5090c89621c2e0d01c0c928149ba77ea38b8b0a18176c
Time to determine pass/fail: 2.8 s
Installing 1/1 skipped-features[core,fail-if-included]:[email protected]...
Building skipped-features[core,fail-if-included]:[email protected]...
CMake Error at C:/Dev/vcpkg-tool/azure-pipelines/e2e-assets/ci-skipped-features/skipped-features/portfile.cmake:3 (message):
The feature 'fail-if-included' should not be included.
Call Stack (most recent call first):
scripts/ports.cmake:206 (include)
error: building skipped-features:x86-windows failed with: BUILD_FAILED
See https://learn.microsoft.com/vcpkg/troubleshoot/build-failures?WT.mc_id=vcpkg_inproduct_cli for more information.
Elapsed time to handle skipped-features:x86-windows: 39.1 ms
Total install time: 39.8 ms
Triplet: x86-windows
RESULTS
skipped-features:x86-windows: BUILD_FAILED: 39.1 ms
SUMMARY FOR x86-windows
BUILD_FAILED: 1After: Compiler found: C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/14.44.35207/bin/Hostx64/x86/cl.exe
skipped-depends:x86-windows: skip
skipped-features:x86-windows: *: bd8a73d4f54b8505b84fcb4f15268b44c4b7063986ce00a843ff904a0d8f410b
Time to determine pass/fail: 3.8 s
Installing 1/1 skipped-features:[email protected]...
Building skipped-features:[email protected]...
-- Skipping post-build validation due to VCPKG_POLICY_EMPTY_PACKAGE
Elapsed time to handle skipped-features:x86-windows: 61.4 ms
skipped-features:x86-windows package ABI: bd8a73d4f54b8505b84fcb4f15268b44c4b7063986ce00a843ff904a0d8f410b
Total install time: 63.8 ms
Triplet: x86-windows
skipped-features:x86-windows: SUCCEEDED: 61.4 ms
SUMMARY FOR x86-windows
SUCCEEDED: 1
EXCLUDED: 1Notes:
|
|
Oh hmmm this is suboptimal because the skip isn't in the report at the beginning. Hmmm.... |
|
I need to consider / test the impact of known fails here. |
Example output where we don't print the ABI sha comes from this line: vcpkg-tool/src/vcpkg/commands.ci.cpp Line 407 in 71538f2
|
|
@ras0219-msft @vicroms @MahmoudGSaleh @AugP and I discussed today. Rough feedback sounds like:
================= I also observe that I should probably test the XUnit and summary output for these cases. |
|
@BillyONeal Do you have an estimate when this is finished since it is blocking the qt pr? |
src/vcpkg/commands.ci.cpp
Outdated
| } | ||
| } | ||
|
|
||
| // FIXME this is the root of the problem: `results` should have *all* of the results, not need to be somehow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fix that 'excluded' is not properly accounted for in the build report, and in the xunit output, etc.
src/vcpkg/commands.ci.cpp
Outdated
| } | ||
| else | ||
| { | ||
| result.excluded.emplace_back(PackageSpec{scfl->to_name(), target_triplet}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also calculate exclusions for the host triplet? For example:
qt-complicated:x64-linux=skip
and
foo:arm64-android -> qt-complicated:$host
qt-complicated:$host -> qtbase[problem]:$host
where qtbase[problem]:x64-linux is a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because this calculating what we morally give to the 'vcpkg install' inside 'ci', and we don't ever add host packages there. Any handling of host dependencies needs to happen in prune_entirely_known_action_branches (formerly known as reduce_action_plan)
| CreateInstallPlanOptions create_install_plan_options( | ||
| randomizer, host_triplet, UnsupportedPortAction::Warn, UseHeadVersion::No, Editable::No); | ||
| randomizer.get(), host_triplet, UnsupportedPortAction::Warn, UseHeadVersion::No, Editable::No); | ||
| auto action_plan = compute_full_plan( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like there's still an unfortunate result when a user makes a small deviation outside a correct plan. For example, adding a dependency to an excluded package. While we know by construction that the resulting CI plan is "invalid", there's a bad constraint amplification problem where one bad constraint (use the excluded package) gets amplified into all of the excluded package's transitive constraints (that got it excluded in the first place).
Probably not worth solving right now, because it would require the dependency calculator to become aware of exclusions and ignore transitive constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know this problem is what we invented =pass for. I'm not sure what you mean by "invalid", that would still be a valid plan, even if it would be empty
Co-authored-by: Robert Schumacher <[email protected]>
Co-authored-by: Robert Schumacher <[email protected]>
…ures # Conflicts: # src/vcpkg/commands.ci.cpp
…ures # Conflicts: # azure-pipelines/end-to-end-tests-dir/ci.ps1
…Cascade Also want to add some more test cases.
…we consider "real" cascades equivalent to statically known cascades in reporting.
src/vcpkg/xunitwriter.cpp
Outdated
| case BuildResult::ExcludedByParent: | ||
| case BuildResult::ExcludedByDryRun: | ||
| case BuildResult::Unsupported: | ||
| case BuildResult::Cached: // should this be "Pass" instead? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a question for reviewers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to have this match ExcludedByParent. I don't think ExcludedByParent should be Pass, so this LGTM.
| .line_break(); | ||
| } | ||
|
|
||
| if (!test.features.empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the "we know what features but don't know the ABI tag or vice versa" condition is no longer possible. If we know what features you have, that means we got a plan with you in it, which means we have an ABI hash. Otherwise we don't know either.
| throw "feature-not-sup's baseline pass entry should result in a regression because the port is not supported" | ||
| } | ||
| if (-not ($ErrorOutput.Contains("REGRESSION: dep-on-feature-not-sup:${Triplet} is marked as pass but one dependency is not supported for ${Triplet}."))) { | ||
| if (-not ($ErrorOutput.Contains("REGRESSION: dep-on-feature-not-sup:${Triplet} cascaded, but it is required to pass. ("))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a change in behavior: before, a cascade implied by ci.baseline.txt behaved differently than a "surprise" cascade, the new behavior treats all cascades the same. I think the old behavior was a bug based on the fact that we were implementing how that stuff got reported multiple times and all cascades should be the same for reporting purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Excluded, | ||
| ExcludedByParent, | ||
| ExcludedByDryRun, | ||
| Unsupported, | ||
| CacheMissing, | ||
| Cached, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ras0219-msft These are the names you said you wanted to bikeshed, go for it
| const CiBaselineData& cidata, | ||
| const std::string* cifile, | ||
| bool allow_unexpected_passing, | ||
| bool is_independent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RE: Removal of the "independent" concept, see discussion #1830
…ures # Conflicts: # azure-pipelines/end-to-end-tests-dir/ci.ps1
ras0219-msft
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
All comments are considerations that would be suitable to investigate in separate PRs.
| result_string = "Skip"; | ||
| break; | ||
| case BuildResult::CacheMissing: | ||
| case BuildResult::Downloaded: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems unfortunate to have so many lines dedicated to an assertion buried deep in the formatting logic, since the code flaw is likely much higher up the stack. No change requested.
| { | ||
| StringLiteral result_string = ""; | ||
| switch (test.result) | ||
| switch (test.ci_result.code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This switch is long enough that a table lookup might be clearer. No change requested.
// Do we have 20 yet?
using enum BuildResult;
enum { error, pass, fail, skip };
static uint8_t codemap[] = { [Succeeded] = pass, [BuildFailed] = fail, [PostBuildChecksFailed] = fail, [FileConflicts] = fail, ... };
if (static_cast<int>(test.ci_result.code) > sizeof(codemap)) abort();| }; | ||
|
|
||
| struct UnknownCIPortsResults | ||
| struct CIPreBuildStatus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type mixes logic results (known) with formatting results (report_lines / abis); it might be cleaner to reduce this to just std::map<PackageSpec, BuildResult> known; and have the report code deal with calculating report_lines / abis on the fly from the action_plan and/or excluded.
Request no change; consider follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and have the report code deal with calculating report_lines / abis on the fly from the action_plan and/or excluded
I tried to do this. Unfortunately that forces recalculating a bunch of information we have in the for loop on line 125; I considered mixing the formatting concern the lesser evil than duplicating that control flow.
| void reduce_action_plan(ActionPlan& action_plan, | ||
| const std::map<PackageSpec, BuildResult>& known, | ||
| View<std::string> parent_hashes) | ||
| void prune_entirely_known_action_branches(ActionPlan& action_plan, const std::map<PackageSpec, BuildResult>& known) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since known now must contain every spec in the action plan, should it be a simpler std::unique_ptr<BuildResult[]> or vector?
No changes requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
known now must contain every spec in the action plan
known contains things not in the action plan as well which would make that somewhat painful
| format_ci_result(r.first, r.second, cidata, ci_baseline_file_name, allow_unexpected_passing, true); | ||
| if (!msg.empty()) | ||
| if (scfl->source_control_file->has_qualified_dependencies() || | ||
| !scfl->source_control_file->core_paragraph->supports_expression.is_empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably check features as well for qualified dependencies and supports expressions.
But I think this is just a performance optimization, not correctness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure but I didn't actually change this so I'm going to avoid changing it. (I just extracted this block as a function:
vcpkg-tool/src/vcpkg/commands.ci.cpp
Lines 90 to 99 in 4819c23
| std::vector<PackageSpec> packages_with_qualified_deps; | |
| for (auto&& spec : specs) | |
| { | |
| auto&& scfl = provider.get_control_file(spec.package_spec.name()).value_or_exit(VCPKG_LINE_INFO); | |
| if (scfl.source_control_file->has_qualified_dependencies() || | |
| !scfl.source_control_file->core_paragraph->supports_expression.is_empty()) | |
| { | |
| packages_with_qualified_deps.push_back(spec.package_spec); | |
| } | |
| } |
)
src/vcpkg/commands.ci.cpp
Outdated
| const TripletExclusions* target_triplet_exclusions = nullptr; | ||
| for (auto&& te : exclusions_map.triplets) | ||
| { | ||
| msg::write_unlocalized_text_to_stderr(Color::none, output); | ||
| if (te.triplet == target_triplet) | ||
| { | ||
| target_triplet_exclusions = &te; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const TripletExclusions* target_triplet_exclusions = nullptr; | |
| for (auto&& te : exclusions_map.triplets) | |
| { | |
| msg::write_unlocalized_text_to_stderr(Color::none, output); | |
| if (te.triplet == target_triplet) | |
| { | |
| target_triplet_exclusions = &te; | |
| break; | |
| } | |
| } | |
| auto it = Util::find_if(exclusion_map.triplets, [&](auto& x) { return x.triplet == target_triplet; }); | |
| const TripletExclusions* target_triplet_exclusions = it != exclusion_map.triplets.end() ? &*it : nullptr; |
Could also simplify target_triplet_exclusions to:
const SortedVector<std::string>* exclusions;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not that much shorter to do this and it involves lambdas which is a tiebreaker-against for me but I don't care THAT much so I applied it.
src/vcpkg/commands.ci.cpp
Outdated
| const auto parsed_object = | ||
| Json::parse(parent_hashes_text.content, parent_hashes_text.origin).value_or_exit(VCPKG_LINE_INFO); | ||
| const auto& parent_hashes_array = parsed_object.value.array(VCPKG_LINE_INFO); | ||
| for (const Json::Value& array_value : parent_hashes_array) | ||
| { | ||
| auto abi = array_value.object(VCPKG_LINE_INFO).get(JsonIdAbi); | ||
| Checks::check_exit(VCPKG_LINE_INFO, abi); | ||
| parent_hashes.insert(abi->string(VCPKG_LINE_INFO).to_string()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nice to extract this as a function:
ExpectedL<std::unordered_set<std::string>> parse_parent_hashes(const std::string& text_content, int origin);
Billy made a bit of a mistake in microsoft#1822 resulting in the blizzard of failures in https://dev.azure.com/vcpkg/public/_build/results?buildId=122788
Billy made a bit of a mistake in #1822 resulting in the blizzard of failures in https://dev.azure.com/vcpkg/public/_build/results?buildId=122788

See related microsoft/vcpkg#47735
OK, so here's the situation:
ci.baseline.txthas:In microsoft/vcpkg#47735 ,
qtwaylanddepends onqtbase[wayland], which implicates the same missing system libraries. This causesqtbaseto contain the featurewaylandwhen we try to build it, even thoughqtwaylandis skipped. That makesqtbasefail to build and breaks everything that depends on qt.The fix is just to not include explicitly skipped ports when we are forming the list of specs we want when building the initial action plan.
======================
As a result of reviewing competing change from @Neumann-A #1821 (comment) which also added more correct skip outputs, this also contains a full overhaul of our console output for the ci command to include more status in the "table" at the beginning, and ensure every port is included in the table at the beginning and in the summary counts at the end.
I think @Neumann-A and @dg0yt should be listed as effective coauthors on this when merging. @Neumann-A suggested the original behavior change and wrote #1821 with an initial shot at it. @dg0yt contributed several helpful test cases.