-
Notifications
You must be signed in to change notification settings - Fork 280
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
Add option to remove unaffected ports from ci
action
#210
Conversation
508aeb2
to
bb800a1
Compare
What's the idea here? We still need to do a full test of all of the ports, so where would this make a difference for our CI? |
Not sure if you really took the time to read. Let's turn the question around: |
@dg0yt because that's part of the SLA/value-add of vcpkg; we build all of these ports together so you can trust that, if you use a specific baseline, you'll get a universe that works. |
@strega-nil-ms No, this is neither observed nor implemented for pull requests. In my example, on a PR changing
Whether
(Note that in most cases, PR CI never tests the state which results from the GH merge action: When the merge is done, the state of master is already different from the state when the PR was tested.) So this PR is just adds the opportunity to reliably isolate the installation plan from changes which are caused by the state of master (provided as parent hashes). What is actually build still depends on the state of the cache. |
@strega-nil-ms Note how all PRs are rebuilding all ports for osx now, because the cache was empty when determining the installation plan. A pointless exercise. The PRs scheduled first will run into the timeout again. And https://github.com/microsoft/vcpkg/pull/19034/files doesn't affect a single other port. |
ci
actionci
action
src/vcpkg/commands.ci.cpp
Outdated
to_keep.insert(it->spec); | ||
} | ||
|
||
if (Util::Sets::contains(to_keep, it->spec) && it->plan_type != InstallPlanType::EXCLUDED) |
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 Note that there was an extra condition here: Don't keep specs just for being dependencies of an affected port if the affected port is excluded. I think this can be safely taken to the combined reduce_actions
, even if it may sometimes reduce the installation plan when no parent hashes are given.
That's not exactly what our existing PR validation does though: it is affected by edits to The approach described here ( exclude ABI hashes already in the master branch for purposes of determining the cone of destruction since they aren't affected by the PR being validated ) seems a reasonable tradeoff to make to me, and might let us add another triplet or similar in exchange without blowing the Azure budget. |
@dg0yt you are buying our own release-only triplet with this work, no need to wait for extra budget! or do you have anything better to invest this azure time into? you deserve it man if you need it! |
What a pity that this is not in today's release. In its original formal, it would have been easy to put the behavioural variation behind an |
We can turn the release crank whenever we think it's important to do so, don't worry :). This is important enough that IMO we would do a new release just for it. |
Also pushed a merge with |
Thanks for your contribution! |
With the the updated tool used in boostrapping, I moved this forward in microsoft/vcpkg#21078. |
Implementing an idea for how to reduce the set of packages to build during vcpkg PR CI, in particular after significant merges to master. Originating in #201 (comment).
ATM CI rebuilds all packages which don't have a matching cache entry, including unrelated changes from master. This can include a significant number of ports which are not affected by a PR.
In addition, the install list is created at the beginning of the CI command, possibly hours before building leaf ports. Different PR CI runs will build the same ports in parallel.
In case of broken artifact caching, as on 2021-10-03 for x64-osx, all PRs will build all ports (>1500, >20 hrs).
Building unrelated ports also increase the vulnerability for unrelated issues such as download problems.
This PR adds an optional step which removes the packages from the installation plan which have identical ABI hashes in a json file provided by the user and are not required as a dependency.
This json file can be created in a dry run, even with binary caching disabled, for the "known" state before the PR, typically the master parent of the PR merge commit (i.e. HEAD~1).
Simulating the impact for 2a31089 (port gsl-lite change), worst case (nothing cached):
So instead of building (or considering) 1725 ports, we are down to 52 ports. And it is possible to inspect that these are indeed the ports which are needed to build, or depend on, port gsl-lite.
Disclaimer:
This started as an attempt into the feasibility of an implementation. In particular the code for loading the json data into a list of strings (hashes) doesn't do much error handling.