ci: run the two package-scoped builds only when the change can affect them - #1265
Merged
Conversation
… them `Mobile build` cross-compiles vta-mobile-core for iOS and Android and takes ~22 minutes. Its workspace dependency closure is two crates: vta-mobile-core and vta-sdk. `Enclave build` runs `cargo check -p vta-enclave`, whose closure of 21 crates contains no vtc-* and no room-host. Every pull request touching neither waits for both anyway. On the last 120 commits, 13% touch only vtc-*, 53% touch no VTC code at all and 18% are docs or CI only — so most changes here pay for at least one build that cannot possibly be affected by them. The closure is DERIVED, never listed. `cargo metadata` already knows what a package depends on; a hand-written list of "paths that affect mobile" would be a second description of the same fact, and this repo has now paid three times for two lists of one thing drifting apart (#1252, #1256, #1259). A crate added to the graph tomorrow is covered with no edit here. Every uncertainty resolves to RUN, because a skipped job is a check that did not run and this repo has already shipped breaks behind checks that silently covered less than they claimed: - not a pull request -> run - lockfile, root manifest, .github/, scripts/ -> run - a changed path owned by no workspace crate -> run - no base ref, or no usable diff -> run and the job condition is `!= 'false'`, not `== 'true'`, so a missing output runs the job rather than skipping it. Two things worth knowing about the implementation: The graph walk is scripts/ci-closure.py, a real file rather than a `python3 -c` heredoc. The inline form is quietly broken — the program sits inside shell single quotes, and every `'.'`, `p['name']` or `'IN'` inside it ENDS that string. It still runs, still exits non-zero, and with a fail-safe wrapper the filter reverts to always-running while looking installed. That is what the first draft did; only running it found out. Base resolution tries `origin/$GITHUB_BASE_REF`, then `HEAD^1`, then `origin/main`. On a pull_request event Actions checks out the MERGE commit, where `origin/main` is frequently not a named ref — so the obvious `git diff origin/main` fails and the filter silently degrades to no filter at all. HEAD^1 of that merge commit is the base tip, which is exactly what is wanted. `affects` is deliberately not gated on `pull_request`: a job whose `needs` were skipped is itself skipped, which would stop both builds running on pushes to main. Verified against nine cases in a scratch clone — vtc-service, room-host and vti-rooms source changes skip; vta-sdk and vta-mobile-core run; docs-only skips; Cargo.lock runs; an unattributable new directory runs; and the merge-commit shape with no origin remote resolves `HEAD^1` and still decides correctly. Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A VTC-only change currently waits for
Mobile build(~22 min, closure of twocrates) and
Enclave build— neither of which anyvtc-*crate is in. Thisgates both on whether the change can actually affect them.
The numbers that motivated it
Last 120 commits on main:
vta-mobile-core's workspace closure is{vta-mobile-core, vta-sdk}.vta-enclave's is 21 crates, containing novtc-*and noroom-host. Somost changes here pay for at least one build that cannot possibly be affected.
(Worth noting: this is also why I'd argue against splitting VTC into its own
repo — more commits cross the boundary (16%) than are confined to VTC (13%), so a
split taxes the larger group. Path filtering helps both directions and touches
nothing about releases.)
The closure is derived, never listed
cargo metadataalready knows what a package depends on. A hand-written list of"paths that affect mobile" would be a second description of the same fact, and
this repo has paid three times over for two lists of one thing drifting apart
(#1252, #1256, #1259). A crate added to the graph tomorrow is covered with no
edit here.
Every uncertainty resolves to RUN
A skipped job is a check that did not run, and this repo has already shipped
breaks behind checks that silently covered less than they claimed.
.github/,scripts/changed → runThe job condition is
!= 'false', not== 'true', so a missing output runs thejob rather than skipping it.
Two traps worth knowing about
The graph walk is a real file,
scripts/ci-closure.py, not apython3 -cheredoc. The inline form is quietly broken: the program sits inside shell single
quotes and every
'.',p['name']or'IN'inside it ends that string. Itstill runs, still exits non-zero, and with a fail-safe wrapper the filter reverts
to always-running while looking installed. That is exactly what my first draft
did — review would not have caught it; running it did.
Base resolution tries
origin/$GITHUB_BASE_REF, thenHEAD^1, thenorigin/main. On apull_requestevent Actions checks out the merge commit,where
origin/mainis frequently not a named ref — so the obviousgit diff origin/mainfails and the filter silently degrades to no filter.HEAD^1ofthat merge commit is the base tip, which is what is wanted.
affectsis deliberately not gated onpull_request: a job whoseneedswere skipped is itself skipped, which would stop both builds running on pushes to
main.
Verified
Nine cases in a scratch clone:
Next, if this lands well
The Test job is the bigger prize (
vta_servicealone is 906s of 2203s, VTC's 52integration binaries another 848s), but it is also where a wrong filter is
costly, so I would rather see this pattern proven on the two cheap-to-verify
builds first.