ci: stop compiling the tree three times, and cache what main builds - #2801
ci: stop compiling the tree three times, and cache what main builds#2801kixelated wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe pull request adds a GitHub Actions workflow that warms a shared Rust cache and updates cache usage in concurrent check and test jobs. It changes development builds to line-table debug information. Go, Kotlin, and Swift scripts now support debug and release FFI profiles. FFI packaging documents size-check behavior for debug artifacts. Rust tests can optionally use 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go/scripts/package-ffi.sh`:
- Around line 62-65: Update the help-text comments consumed by the script’s help
generator near the optional arguments to document --no-size-check, stating that
it bypasses the GitHub file-size preflight for temporary debug artifacts; keep
the existing SIZE_CHECK handling unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 434fe15a-5c13-4de9-b679-399e61fac238
📒 Files selected for processing (11)
.github/workflows/alert.yml.github/workflows/cache.yml.github/workflows/check.ymlCLAUDE.mdCargo.tomlgo/scripts/check.shgo/scripts/package-ffi.shkt/scripts/generate.shrs/CLAUDE.mdrs/justfileswift/scripts/check.sh
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1199d42879
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Run the test suite, reporting clippy findings from the same compile. | ||
| test *args: | ||
| cargo nextest run --all-targets {{ args }} | ||
| RUSTC_WRAPPER="$(command -v clippy-driver || true)" cargo nextest run --all-targets {{ args }} |
There was a problem hiding this comment.
When a developer has already compiled the selected crates with ordinary cargo (for example, via cargo test --no-run or one of the binding checks), Cargo considers those artifacts fresh after only RUSTC_WRAPPER changes, so this command never invokes clippy-driver and reports none of the promised Clippy findings. This makes just test unreliable as the documented single-compile lint-and-test command; use a fingerprinted mechanism such as RUSTC_WORKSPACE_WRAPPER or otherwise force the relevant workspace crates through Clippy. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L99-L99
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed, and thank you for this one: it was a real bug that made the feature not work. Reproduced exactly as described. After cargo test --no-run -p kio built the binary with no wrapper, RUSTC_WRAPPER=clippy-driver cargo nextest run -p kio emitted 0 clippy lines, because cargo considered the artifacts fresh. So just test silently skipped linting whenever bare cargo had run first, which is the common case.
Switched to RUSTC_WORKSPACE_WRAPPER as you suggested. Verified in the same failing scenario: 2 lint lines on the first run and 2 on a repeat, so it is stable rather than a one-off rebuild. It also has a second benefit worth recording: it wraps only workspace crates, so dependencies keep the artifacts they share with a plain cargo test and only our ~28 crates duplicate. I corrected the note in rs/CLAUDE.md, which had overstated that as a full second copy of the tree.
(Written by Opus 5)
🤖 Addressed by Claude Code
CI was compiling the Rust tree three times over at three artifact sets that share nothing, and throwing all of it away afterwards. Measured on the Check run for #2769: clippy 232s, nextest 415s, and a release build of moq-ffi 287s (`Finished 'release' profile in 4m 47s`), the last of which nobody had chosen deliberately. The cache never worked. Actions scopes cache reads to the current branch plus the default branch, and check.yml runs on `pull_request` only, so nothing was ever written to `main` and every run started with "No cache found". Each PR then saved its own 2-3 GB entry that no other PR could read: 9.7 GB of the repository's 10 GB budget, spread over five refs, none of them reachable, all evicting each other. cache.yml makes `main` the single writer under a shared key and PRs restore with `save-if: false`. `check` and `test` become concurrent jobs. Clippy emits rmeta without codegen while nextest codegens and links, so there was nearly nothing for one to reuse from the other; running them in sequence paid both for no benefit. The binding check scripts (kt, swift, go) built moq-ffi with `--release`, which shares no artifacts with the debug builds `check` and `test` already produce. They take MOQ_FFI_PROFILE, now debug, so a runner carries one target tree instead of two. The shipped artifacts still come from rs/moq-ffi/build.sh, which is release regardless. That change tripped a guard: `go check` reuses package-ffi.sh to stage a throwaway module, and its 100 MiB check exists for the mirror push (GH001). A debug staticlib is 619 MiB, so it fired on artifacts that are never pushed. package-ffi.sh takes --no-size-check, which only `go check` passes; the release publish path still enforces it. `just test` wraps rustc in clippy-driver, so one compilation both builds the test binaries and reports clippy. `cargo clippy` cannot be reused for this: it asks for metadata only, and metadata and codegen builds have different fingerprints, so running both compiles the tree twice. Findings stay warnings, since `check` is the gate and a hard failure would block running tests over an unused import mid-edit. No RUSTFLAGS, which is part of the fingerprint and would split the artifact cache against CI's warm one. Finally, debug builds carry full DWARF, which is what makes them enormous: the debug libmoq_ffi.a measured 619 MiB, and ten agent worktrees were holding 59 GB on a disk that had filled to 100%. `debug = "line-tables-only"` takes that staticlib to 367 MiB while keeping file and line in every backtrace, dropping only the records an interactive debugger reads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
macOS ships bash 3.2, which treats expanding an empty array under `set -u` as
an unbound variable. `swift check` builds moq-ffi on a macOS runner, so
`"${CARGO_PROFILE[@]}"` aborted the moment the profile was debug (the new
default) and the array was empty. Local runs missed it: the nix dev shell
carries bash 5, where the same expansion is fine.
`${CARGO_PROFILE[@]+"${CARGO_PROFILE[@]}"}` is the guarded form, already used
for `xcode_sdk_env` a few lines down in the same script.
Verified against /bin/bash 3.2.57 directly: the old form reproduces the
failure, and each script's real profile block now emits `cargo build --package
moq-ffi` for debug and `cargo build --release --package moq-ffi` for release.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1199d42 to
8c623ee
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c623ee4e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - name: Rust Cache | ||
| uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 | ||
| with: | ||
| shared-key: rust |
There was a problem hiding this comment.
When main receives another Rust source change without a Cargo.lock, toolchain, or relevant environment change, this constant shared-key produces the same automatic key because rust-cache keys do not include the checked-out revision. GitHub caches are immutable, so an exact hit cannot be replaced after this run; the first warm cache remains while newer main artifacts are never published, causing future PRs to rebuild every workspace crate changed since that initial warm. Use a controlled rotating generation for the main writer, with old-generation eviction, while retaining restore-only PR jobs.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not acting on this one. rust-cache prunes the target directory before saving (its post step logs ... Cleaning /home/runner/work/moq/moq/target ...), keeping dependency artifacts and discarding workspace-crate ones. The saved caches bear that out: 2-3 GB against a ~10 GB target/debug.
So the cache holds dependencies only, and dependencies change only when Cargo.lock changes, which is already an input to the key. The key therefore rotates exactly when the contents need to, and the stale-workspace-artifacts scenario cannot arise because those artifacts are never stored. A rotating generation would instead write a new multi-GB entry per main push against a 10 GB budget, evicting the very cache it just wrote.
(Written by Opus 5)
(Written by Opus 5)
🤖 Addressed by Claude Code
Every job that entered the dev shell was compiling parts of it from source.
The flake declares kixelated.cachix.org under `nixConfig`, but that is
untrusted by default, so nix logged
warning: ignoring untrusted flake configuration setting 'extra-substituters'
Pass '--accept-flake-config' to trust it
and fell back to building. CI has therefore never used the cache at all. It
only became expensive when #2782 added uniffi-bindgen-go, a buildRustPackage
from a git tag that nixpkgs cannot serve prebuilt: 678s per job, and both
Check and Test pay it independently, as does Smoke. That is more than the
whole-workspace clippy (222s) and the moq-ffi build (177s) combined.
Passing the substituter through the installer's `extra-conf` puts it in
/etc/nix/nix.conf, where it is trusted, rather than relying on a flake setting
nix refuses.
Nothing was populating it either: cachix.yml fires only on release tags and
pushes only the tagged package, never the shell. cache.yml now realises the
shell into a profile and pushes that closure, so it warms the same set the
pull-request jobs need.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0db0be19d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # check.yml runs on `pull_request` only, so nothing was ever written to `main` | ||
| # and every PR started from scratch ("No cache found"), recompiling the world. | ||
| # Worse, each PR saved its own 2-3 GB cache that no other PR could read, which | ||
| # filled the repository's 10 GB budget with entries nobody could use and evicted | ||
| # them before a re-run could benefit. |
There was a problem hiding this comment.
This block describes the workflow's history (nothing was ever written, every PR started from scratch, and each PR saved its own cache) rather than its current behavior. The current main-writer/read-only-PR rationale is already explained immediately below, so keep that explanation and move the historical diagnosis to the commit or PR description. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L99-L99
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The header now states the current arrangement (main is the single writer, PRs are readers, and why Actions cache scoping forces that) without the before-and-after. The diagnosis moved to the commit message and PR description, per the "comments reflect the current state, not the history" rule in CLAUDE.md.
(Written by Opus 5)
🤖 Addressed by Claude Code
…lly appear `just test` wrapped rustc in clippy-driver via RUSTC_WRAPPER, which cargo does not fold into the fingerprint. A crate already built by a plain `cargo test` was therefore reported fresh and never recompiled, so the run produced no clippy findings at all while claiming to. Reproduced: after `cargo test --no-run -p kio`, the wrapped run emitted zero lint lines for a deliberate `needless_range_loop`. RUSTC_WORKSPACE_WRAPPER is fingerprinted, so the same scenario now emits the lint, and does so on a repeat run rather than only on the first. It is also what `cargo clippy` sets internally, and it applies to workspace crates only, so dependencies keep the artifacts they share with a plain `cargo test` instead of duplicating the tree. rs/CLAUDE.md overstated that cost and is corrected. The cache.yml header also described what CI used to do rather than what it does. Comments track the current state; the diagnosis belongs in the commit and the PR description. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b798f8091
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Debug by default. This is a compile-and-test gate, not a benchmark, and a | ||
| # release build of moq-ffi shares no artifacts with the debug ones `just check` | ||
| # and `just test` already produce, so it was a third full compile of the | ||
| # dependency tree (~5 min of CI on its own, plus a whole target/release tree on | ||
| # a runner that was already tight on disk). Set MOQ_FFI_PROFILE=release for an |
There was a problem hiding this comment.
In each of the Go, Kotlin, and Swift profile blocks, this comment records implementation history and transient measurements by saying the release build “was a third full compile” and describing an earlier runner's timing and disk pressure. Those details will become misleading as CI changes. Keep the useful current rationale, such as debug builds sharing the active artifact tree while release artifacts use rs/moq-ffi/build.sh, and leave the historical diagnosis in the PR description. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L99-L99
Useful? React with 👍 / 👎.
1 participant
Summary
The largest single cost turned out not to be Rust at all. Profiling a run of this branch showed 678s -- over half the Check job, and paid again in Test -- going to a nix source build before any compilation starts. CI has never used the binary cache: the flake declares
kixelated.cachix.orgundernixConfig, which is untrusted by default, so every run loggedwarning: ignoring untrusted flake configuration setting 'extra-substituters'and rebuilt from source. It only became expensive when #2782 addeduniffi-bindgen-go, abuildRustPackagefrom a git tag that nixpkgs cannot serve prebuilt. For scale, that one build exceeds the whole-workspace clippy (222s) and the moq-ffi build (177s) combined.Fixed on both sides: the substituter now goes through the installer's
extra-conf(into/etc/nix/nix.conf, where it is trusted) so runners can read the cache, andcache.ymlrealises the dev shell into a profile and pushes it so something writes it.cachix.ymlonly ever fired on release tags and pushed only the tagged package, never the shell.CI compiled the Rust tree three times at three artifact sets that share nothing, then discarded all of it. Measured from the Check run on #2769:
cargo nextest run --workspacejust kt check— of whichcargo build --release -p moq-ffiis 287cargo clippy --workspacecheck.ymlruns onpull_requestonly — so nothing was ever written tomainand every run began withNo cache found. Each PR then saved its own 2-3 GB entry no other PR could read: 9.7 GB of the 10 GB budget across five refs, none reachable, all evicting each other. Newcache.ymlmakesmainthe single writer undershared-key: rust; PRs restore withsave-if: false.checkandtestare now concurrent jobs. Clippy emits rmeta without codegen, nextest codegens and links, so sequencing them paid for both with nearly no reuse.--release. kt/swift/go takeMOQ_FFI_PROFILE, defaulting to debug, so a runner carries one target tree instead of two. Shipped artifacts still come fromrs/moq-ffi/build.sh, release regardless.just testlints from the same compile viaRUSTC_WRAPPER=clippy-driver, making it a single-compile substitute forcheck+testlocally.cargo clippycan't be reused for this: metadata-only and codegen builds have different fingerprints, so running both compiles the tree twice.debug = "line-tables-only". Full DWARF is what makes debug builds enormous; see the disk section below.A guard my own change tripped
go checkreusespackage-ffi.shto stage a throwaway module, and that script's 100 MiB check exists for the mirror push (GH001). A debug staticlib is 619 MiB, so it fired on artifacts that are never published. #2782 landed the same fix independently while this was open, under the name--skip-size-check; this branch adopts that flag rather than shipping a second one, and corrects the comment on main that justified the skip with "the lib above is a plaincargo build --release", which stopped being true once the profile defaulted to debug.Disk
The disk was at 100% (3.4 GB free of 461 GB). The cause was not artifact size but that ten agent worktrees each keep a full
target/, totalling 59 GB.CARGO_TARGET_DIRsharing is deliberately off (sccache covers CPU reuse, not disk), so the mitigations are per-artifact size and hygiene:libmoq_ffi.ars/CLAUDE.mdgains a note on why barecargoalongsidejustdoubles the tree (cargo fingerprints by wrapper and emit kind), including pointing rust-analyzer at clippy so it shares withjust checkinstead of opening a third set.Public API changes
None. Build tooling, workflows, shell scripts, and docs only.
[profile.dev] debugis a build-profile change with no effect on published artifacts (release profiles untouched).Measured on CI
Absolute run-to-run times here are not comparable. Two runs of this branch did identical work (1158 crates compiled, ~2800 tests, nextest itself 33.4s vs 33.3s) and the Check job took 611s in one and 1282s in the other. Runner variance is ~2x, so any single-run before/after number, including the 25m baseline this PR started from, says more about which runner you drew than about the change.
What is measurable is the shape, comparing jobs within one run:
The split converts
check + testintomax(check, test), and the two are close in duration, so wall clock roughly halves regardless of runner speed. That part is arithmetic, not a benchmark.The other reductions are in total work rather than wall clock, and were measured from the logs of a single combined run: the
--releasemoq-ffi build was 287s (Finished 'release' profile in 4m 47s), and it is gone. The cache is unmeasured untilmainpopulates one.Test plan
In the Nix dev shell on macOS, with
MOQ_STRICT=1andNEXTEST_PROFILE=cias CI sets them:just fix-> 0,just check-> 0,just test-> 0, 2801 tests passed (post-rebase onto build(nix): package the Go toolchain sojust go checkactually runs #2782).just go checkruns end-to-end now rather than skipping: build -> generating bindings -> assembling ffi module -> staging wrapper ->go vet.just kt checkgreen end-to-end on the debug profile:Finished 'dev' profile, cdylib resolved attarget/debug/libmoq_ffi.dylib,BUILD SUCCESSFUL, jvmTest ran.clippy::needless_range_loopprobe: the lint was emitted from a codegen build that also ran 48 tests, andRUSTFLAGS=-Dwarningsmade it fail (exit 101). Probe removed.actionlintclean;alert.sh check-coveragepasses withCacheregistered (25 non-PR workflows).CARGO_PROFILE[@]: unbound variableon the macOS Swift runner, which no local run could reproduce (the nix shell has bash 5). Fixed with the guarded${CARGO_PROFILE[@]+"..."}form already used forxcode_sdk_envin the same file, and verified against/bin/bash 3.2.57directly: the old form reproduces the failure, and each script's real profile block now emits the right command line in both profile states.libmoq_ffi.a619 -> 367 MiB.Not verified locally: the cache behaviour itself, which only exists on a runner. The first merge to
mainpopulates it; PRs after that should show a restore instead ofNo cache found.Reviewer notes
[profile.dev]re-fingerprints every debug artifact and invalidates the cache once, so the first run after merge is slower, not faster.--releaseremoval is worth less alone than the raw 287s suggests. Debug codegen doesn't reuse clippy's artifacts, so it remains a full codegen build, just unoptimized and ~41% smaller. Its real payoff is one artifact tree instead of two, which compounds with the cache. The cache is the dominant lever.mainwarm may have nowhere to land until they age out or are cleared.just go checkactually runs #2782 packaged the Go toolchain, sojust go checknow runs for real in CI. The debug-profile change therefore applies to a path that previously skipped silently, and this branch's verification exercises it end-to-end (build -> bindgen -> stage -> vet).(Written by Opus 5)