ci: cache-thrash + compile-time + waste-on-failure optimizations - #120
Conversation
Safe, coverage-preserving CI speedups (from an exhaustive 2024-2026 best- practices pass, reconciled against the actual ci.yml + rust-setup): - save-if (rust-setup): only WRITE the cargo cache from `main`. A PR-branch save is scoped to that branch (unreadable by other PRs) but still counts against the 10 GB repo budget and LRU-evicts the shared `main` caches every PR restores from. Restore-on-PR + save-on-main keeps the hit rate high without the thrash. - CARGO_PROFILE_DEV/TEST_DEBUG = line-tables-only: CI builds clean, so the dev/test `debug = 2` default just slows linking + bloats target/ (worse caching). line-tables-only keeps panic/backtrace file:line while cutting debug compile ~20-40%. Set via CARGO_PROFILE_* (stable cache key), not RUSTFLAGS (which would bust the cache + force a dep rebuild). - needs: lint on the two heaviest jobs (the 3-OS `test` matrix + release-mode `test-roms`): a fmt/clippy/rustdoc slip fails in seconds, so don't spin up the matrix or pay the release ROM compile until the cheap gate is green. ~+lint-duration happy-path latency; saves the full matrix's runner-minutes on every lint failure. - cache-on-failure on test-roms: a ROM-assertion (test-stage) failure keeps the expensive release dependency compile cached instead of discarding it. YAML validated. Further-gain follow-ups (arm64 runner, cargo-hack feature-combo clippy to close the scripting/hd-pack/retroachievements CI gap, paths-filter per-job skips, merge_group + PR-ubuntu-only matrix, cargo-nextest) are written up in the PR for maintainer review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Rust setup GitHub Action to support a cache-on-failure input and restricts cache saving to the default branch to prevent cache thrashing on PR branches. Feedback suggests avoiding hardcoding the default branch as 'refs/heads/main' and instead using 'github.event.repository.default_branch' to ensure compatibility with forks and repositories using different default branch names.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Locks in the maintainer-decided follow-ups (actionlint-clean): - merge_group hybrid (OS matrix): a `setup` job picks the matrix by event — `pull_request` runs ubuntu-only (cheap, fast feedback); push-to-main, the merge queue, and dispatch run the full ubuntu+macOS+Windows matrix. macOS (10x) + Windows (2x) leave every PR push; full cross-platform coverage still runs pre-merge (merge_group, once the queue is enabled) and post-merge (push). Added the `merge_group` trigger and a `CI success` aggregator job — the single stable check to require in branch protection / the queue (skipped legs count as success, so the event-varying matrix can't block it). NOTE: test-roms (the AccuracyCoin gate) deliberately STILL runs on PRs — it's ubuntu (1x) and PR-time accuracy feedback is too valuable for this project to defer to the queue. One-line change if queue-only is preferred. - Feature-combo clippy in `lint`: the `scripting` / `scripting,hd-pack` / `retroachievements` gates CLAUDE.md mandates previously ran ONLY locally — a feature-gated lint regression could land CI-green. Now run serially on the warm lint runner (NOT --all-features: scripting/script-wasm can't co-resolve). - save-if: compare github.ref_name to the repo's actual default_branch instead of hardcoding "main" (robust on forks / renamed default; adopts the PR #120 review comment). REQUIRES (maintainer, one-time, in repo settings) to fully activate the queue: enable the merge queue on `main` + add a branch-protection rule requiring the `CI success` check. Until then the workflow is safe: PRs run ubuntu-only + test-roms; push-to-main runs the full matrix (post-merge coverage). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Maintainer decisions now implemented in this PR (was: proposed): merge_group OS-matrix hybrid (ubuntu-only PRs, full matrix on push/merge-queue/dispatch, One-time settings step to fully activate the merge queue (only you can do this): in repo Settings → Branches, add a branch-protection rule on |
CI workflow optimizations — cache thrash, compile time, wasted-on-failure compute
From an exhaustive 2024–2026 best-practices research pass (rust-cache README, docs.github.com/actions, nexte.st, dorny/paths-filter, taiki-e/install-action, cargo-hack, the Rust perf-book / kobzol / corrode.dev compile-time posts, and real ripgrep/rust-analyzer/tokio workflows), reconciled against the actual
ci.yml+rust-setupcomposite (not assumptions).Implemented here (safe, high-ROI, zero coverage loss, no behaviour change)
save-if: ${{ github.ref == 'refs/heads/main' }}on the cargo cachemaincaches every PR restores from — i.e. PR saves are write-only garbage that lower everyone's hit rate. Restore-on-PR + save-on-mainremoves the thrash.rust-setup/action.yml(covers all callers)cache-on-failure: trueontest-romsrust-setupinput +ci.ymlCARGO_PROFILE_DEV/TEST_DEBUG: line-tables-onlydebug = 2default only slows linking + bloatstarget/(worse caching).line-tables-onlykeeps panic/backtracefile:line(failing-test diagnostics stay readable) while cutting debug compile ~20–40%. Set viaCARGO_PROFILE_*(a stable cache-key contributor) — notRUSTFLAGS=-Cdebuginfo=0, which busts the rust-cache key and forces a full dep rebuild. Release profile (test-roms/bench) is unaffected (alreadydebug = 0).ci.ymlenvneeds: linton the 3-OStestmatrix +test-romsci.ymlOne-time cost: the first post-merge
mainbuild does a cold cache save (#1) and a one-time recompile at the new debug level (#3); PRs are unaffected.Confirmed already-optimal (NOT changed)
CARGO_INCREMENTAL=0— rust-cache sets this automatically.actions/checkoutfetch-depth: 1— already the v4/v6 default.keys (not ashared-key) — correct here: the OS / wasm32 / thumbv7emtarget/dirs are mutually incompatible, so a shared key would be ~100% miss + constant re-save.bin/proc-macro/FFI-build-script crates this workspace ships.Proposed follow-ups (left for maintainer review — some trade coverage/behaviour)
scripting/scripting,hd-pack/retroachievementsclippy gates CLAUDE.md mandates currently run only locally, never in CI.cargo hack clippy -p rustynes-frontend --feature-powerset --depth 2 --mutually-exclusive-features scripting,script-wasm -- -D warningscloses that on the one warmlintrunner. (Adds lint time — a completeness vs speed trade.)ubuntu-24.04-armmatrix leg — free on this public repo (GA 2025-08), real arm64 determinism coverage at $0; parallel, so no wall-clock cost.dorny/paths-filterper-job skips — skip the frontend/wasm/Pages jobs on chip-stack-only PRs (more granular than the current trigger-levelpaths-ignore); pair with aci-successaggregator so skips stay safe.merge_group+ matrix split (maintainer decision): PRs run ubuntu-only; full macOS/Windows matrix + heavytest-romsrun on the merge queue + a nightly backstop. Highest runner-minute saver (macOS 10×, Windows 2× multipliers) but trades PR-time cross-platform coverage — risky for this repo specifically (wgpu backend divergence, path separators, CRLF in.sym/golden text). Recommended with afull-cilabel escape hatch.cargo-nextest(maintainer decision): process-per-test isolation (good for the ROM/FFI suite) but only ~1.3–1.5× on CI's run phase (build-dominated here), and needs care: addcargo test --doc, translate#[ignore]probes, and no blanketretries(a green-on-retry would hide exactly the non-determinism the contract forbids).cargo build --timingsartifact to decide whethercargo-hakariworkspace-hack is worth it.🤖 Generated with Claude Code