Repository navigation
ci: add sccache beside rust-cache in the PR-path Rust jobs - #915
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: approve. No blocking defect found on exact head ad11b8e9a2c425db9ea829fd55afcb149a49d50d. The inline comment is an optional measurement improvement.
This PR tries to reduce repeated Rust compilation when a whole-target archive is unavailable. It adds sccache before Cargo in nine existing job definitions. The archive remains the first cache. When Cargo invokes rustc, sccache can reuse a matching compilation. Only main writes entries. PR, queue and tag refs read them. Test commands, matrices, permissions and timeouts remain unchanged.
The cause is a mismatch between the archive's unit of reuse and a compiler invocation's inputs. The shared compiler wrapper addresses that general class across the selected jobs. It does not special-case a lockfile or dependency name. A lockfile change can still change dependency hashes, features or compiler inputs, so reuse is conditional. This change does not promise that unchanged package versions always produce hits.
Supporting code: job environment, setup order, and GQT conditional installation. I checked the exact action source at 7d986dd989559c6ecdb630a3fd2557667be217ad. It honors the binary version, verifies its downloaded checksum, exports the Actions cache runtime variables and adds the binary to PATH. I also checked sccache v0.16.0's GHA configuration parser, namespace and remote-storage implementation. READ_ONLY is supported. Ordinary remote read failures become misses, while installation or storage initialization can still fail the job. This adds an availability dependency even though a cache hit is not required for correctness.
Tradeoffs and liability: per-compilation entries can survive loss of one large archive and can reuse a subset of a changed graph. They also add lookups, compression, entries competing for the same quota, and another daemon and downloaded tool. Linking remains uncached. The main-only write policy reduces PR eviction pressure but prevents reuse of PR-only compiles. Pinning both action and binary limits update surprises. The nine repeated settings add maintenance cost, but introducing a general wrapper framework would add more machinery for this small configuration. Five similar additions should preserve explicit setup order and one consistent write policy. Overall, this is a reasonable increase in operational liability for a potential build-cost reduction. Retain it based on measured benefit, not the existence of a cache.
Validation:
- Parsed all three changed workflows. After removing only the sccache environment entries and install steps, they equal main parent
26ef16091437cf138d3ccb04f5ec5351b0846121. Checked seven CI job definitions, one GQT matrix definition and one DST definition. Each installation follows rust-cache and precedes Cargo. - Local action-pin checks passed for 86 external uses. Merge-group/CI Gate checks, CI-cell checks for 48 required test names, classifier-copy checks, and all 14 storage-upgrade configuration tests passed.
- Documentation checks passed for 228 Markdown files. AGENTS checks passed for 53 links and 50 docs. Diff whitespace checks passed. The pinned rust-cache source exports
CARGO_INCREMENTAL=0, as the documentation states. - Separate exact-head CI, GQT and DST passed. This includes workspace tests, Clippy, storage upgrades, the format fence, RustFS and Azurite.
The workspace post-step reports zero compilation requests, hits and misses despite earlier compilation. The pinned daemon has a 600-second idle timeout. The final test compilation finished at 13:40:12 UTC, and statistics were collected at 13:50:32. The inline comment explains how to preserve useful evidence. These counters do not establish a speedup, and I did not benchmark a seeded main-to-PR cache or quota retention.
GQT cannot express compiler-cache reuse, Actions permissions or daemon lifetime. The existing workflow/configuration owners are appropriate here. The structural comparison proves unchanged job contracts, not a behavioral before/after performance regression. I ran no local Rust build and added no superficial GQT case. No engine, storage contract or pinned Lance implementation changes are involved. The isolated checkout is clean, and I rechecked open state, head, reviews and comments before posting.
What & why
A
Cargo.lockchange no longer recompiles the whole dependency graph in CI. This PR adds sccache, stored in the GitHub Actions cache, to the nine PR-path Rust jobs that already restore aSwatinem/rust-cachearchive (seven inci.yml,GQTingq-logic-tests.yml, the DST suite indst.yml).The problem. #912's
GQT (ordinary)job was still compilingdatafusion-*14 minutes in, aftertokio,arrow, the AWS SDK andopendal. The PR's only Cargo change was two lines inCargo.lock(tracingandtracing-subscriberonomnigraph-gqt). Every recompiled crate had the same version and the same features as onmain, so nothing about those crates had changed. They were rebuilt because the cache did not have them.Why rust-cache alone cannot fix it. rust-cache snapshots the whole
target/under one key, and that key hashesCargo.lockand everyCargo.toml. One changed line misses the exact key. The fallback is a prefix key that can only find an archivemainsaved, anddocs/dev/ci.mdalready records that the repository's caches exceed GitHub's 10 GB cap, somain's archive is routinely evicted by the saves of other jobs. An archive is all or nothing: when it is gone, 600 crates compile from source, 27 minutes for a coldGQT (ordinary)build, three times over for the matrix. No key design fixes that, because the unit of caching is the directory, not the crate.How sccache works. sccache sits in front of rustc as
RUSTC_WRAPPER. Before each crate compile it hashes that compile's inputs (source files, compiler version, flags, paths, the hashes of the crates it depends on) and looks the hash up in a store. A hit downloads the compiled output and skips rustc; a miss compiles and uploads the output under that hash. The unit is one crate, and the key is the crate's own inputs, not the lockfile.How that solves the problem. When the archive misses, cargo still asks rustc to compile
tokio,arrowanddatafusion, but their inputs are unchanged, so sccache answers each one from the store and only the crates whose inputs really changed compile. rust-cache exportsCARGO_INCREMENTAL=0in CI, so workspace library crates a PR did not touch are hits as well. Linking stays uncached (test binaries, bins, proc-macro crates), and rust-cache stays the fast path when the lock matchesmain: one archive download, no per-crate lookups.Two policies carried over from the archive cache:
SCCACHE_GHA_RW_MODEisREAD_WRITEonrefs/heads/mainandREAD_ONLYon every other ref. rust-cache'ssave-ifdoes not govern sccache, so the rule is repeated per job.v0.16.0: withoutversionthe action downloads the latest release, and the binary version selects the cache namespace, so a release would silently empty the cache.Speedup unmeasured. Measure after merge with the action's post-step hit/miss counts and build timings on
mainand PR runs; Cargo printsCompilingon a cache hit, so the log line is not evidence either way.Backing issue / RFC
GQT (ordinary)job recompiling the unchanged dependency graphChecklist
docs/dev/ci.mdparagraphs)docs/dev/ci.md; no user-facing surface)Local verification
python3 scripts/check-workflow-action-pins.py: 85 external uses, all commit-pinnedpython3 -c "import yaml; [yaml.safe_load(open(f)) for f in ['.github/workflows/ci.yml', '.github/workflows/gq-logic-tests.yml', '.github/workflows/dst.yml']]": all three parse (PyYAML 6.0.2)python3 scripts/check-ci-cells.py --self-test: ok; 48 required test names definedpython3 scripts/check-classify-copy.py: both copies matchci.ymlpython3 scripts/check-merge-group-triggers.py --self-test: ok; 12 required contextspython3 scripts/check-storage-upgrade-ci.py --self-test: 13 tests passedpython3 scripts/check-docs.py: 228 Markdown files checkedbash scripts/check-agents-md.sh: 53 links, 50 docsgit diff --check: cleancargo check/test/clippy: not run: no Rust changeNotes for reviewers
RUSTC_WRAPPERenters rust-cache's environment hash, and sccache's namespace is empty until main writes it.