prototype: Rust CI checks via cargo (sccache) instead of crane#1821
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe flake 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 `@rs/justfile`:
- Around line 53-57: Add the `--locked` flag to all the cargo commands in the
justfile task: specifically to `cargo check`, `cargo clippy`, `cargo doc`, and
`cargo test`. This flag enforces that the exact versions from Cargo.lock are
used during CI resolution, preventing lockfile drift and ensuring reproducible
deterministic builds. Append `--locked` to each cargo invocation to maintain
dependency stability.
🪄 Autofix (Beta)
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
Run ID: da5dbbea-99c5-4ffc-8bce-05a45c1b74f7
📒 Files selected for processing (3)
flake.nixjustfilers/justfile
| cargo check --workspace --all-targets | ||
| cargo check --workspace --no-default-features | ||
| cargo deny check --show-stats | ||
| cargo clippy --workspace --all-targets --all-features -- -D warnings | ||
| RUSTDOCFLAGS="-D warnings" cargo doc --workspace --all-features --no-deps | ||
| cargo test --workspace --all-targets --all-features |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Inspect cargo CI invocations and lock enforcement:"
rg -n '\bcargo (check|clippy|doc|test)\b' justfile rs/justfile .github/workflows -g '*'
echo
echo "Current --locked usage:"
rg -n '\--locked\b' justfile rs/justfile .github/workflows -g '*'Repository: moq-dev/moq
Length of output: 1167
🏁 Script executed:
# Check if Cargo.lock exists
ls -la | grep -i lock || echo "No Cargo.lock in root"
find . -maxdepth 2 -name "Cargo.lock" -type fRepository: moq-dev/moq
Length of output: 315
🏁 Script executed:
# Read context around lines 53-57 in rs/justfile
sed -n '45,60p' rs/justfileRepository: moq-dev/moq
Length of output: 1039
🏁 Script executed:
# Check if justfile commands are called in CI workflows
grep -r "just " .github/workflows || echo "No justfile invocations found in workflows"
grep -r "justfile" .github/workflows || echo "No justfile references in workflows"Repository: moq-dev/moq
Length of output: 768
🏁 Script executed:
# Check git history to see if these lines were recently added
git log --oneline -p rs/justfile | head -200 | grep -A 10 "cargo check --workspace --all-targets"Repository: moq-dev/moq
Length of output: 516
🏁 Script executed:
# Verify --locked flag is available and compatible with all these cargo subcommands
cargo check --help 2>/dev/null | grep -A 2 "locked" || echo "Checking cargo version..."
cargo --versionRepository: moq-dev/moq
Length of output: 346
🏁 Script executed:
# Check if there are other examples of --locked in justfile or if there's a pattern we should follow
grep -B 2 -A 2 "cargo" rs/justfile | head -40Repository: moq-dev/moq
Length of output: 1922
🏁 Script executed:
# Verify all cargo subcommands (check, clippy, doc, test) support --locked
web_search
cargo --locked flag support for check clippy doc test subcommandsRepository: moq-dev/moq
Length of output: 351
🌐 Web query:
cargo --locked flag support for check clippy doc test subcommands
💡 Result:
The --locked flag is officially supported and functions as expected for the standard Cargo subcommands cargo check, cargo doc, and cargo test [1][2]. When used, this flag asserts that the exact same dependencies and versions recorded in the Cargo.lock file are used; Cargo will exit with an error if the lock file is missing or requires updates [1][2][3]. For cargo clippy, the situation is nuanced. While Clippy is a Cargo subcommand and supports the standard dependency resolution process, there is a known behavioral distinction regarding how the --locked flag is parsed. Specifically, using the flag as a subcommand argument (e.g., cargo clippy --locked) correctly applies the constraint [4]. However, placing it as a global flag (e.g., cargo --locked clippy) may result in the flag being ignored or not passed correctly to the external subcommand, which can lead to unexpected behavior [4]. To ensure the --locked flag is correctly applied, it should always be placed after the subcommand (e.g., cargo check --locked, cargo clippy --locked, cargo doc --locked, or cargo test --locked) [4]. Top results: [1][4][2]
Citations:
- 1: https://doc.rust-lang.org/stable/cargo/commands/cargo-check.html
- 2: https://doc.rust-lang.org/stable/cargo/commands/cargo-doc.html
- 3: https://manpages.ubuntu.com/manpages/plucky/man1/cargo-1.83-doc.1.html
- 4:
cargo --locked clippybehaves differently thancargo clippy --lockedrust-lang/cargo#11390
Add --locked to the new CI cargo commands to enforce reproducibility.
The cargo commands at lines 53–57 lack the --locked flag, which permits lockfile drift during CI resolution. Since these commands replace crane-based checks (which provided deterministic builds), this weakens dependency reproducibility and allows missing Cargo.lock updates to slip through undetected.
Suggested patch
- cargo check --workspace --all-targets
- cargo check --workspace --no-default-features
- cargo clippy --workspace --all-targets --all-features -- -D warnings
- RUSTDOCFLAGS="-D warnings" cargo doc --workspace --all-features --no-deps
- cargo test --workspace --all-targets --all-features
+ cargo check --workspace --all-targets --locked
+ cargo check --workspace --no-default-features --locked
+ cargo clippy --workspace --all-targets --all-features --locked -- -D warnings
+ RUSTDOCFLAGS="-D warnings" cargo doc --workspace --all-features --locked --no-deps
+ cargo test --workspace --all-targets --all-features --locked📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cargo check --workspace --all-targets | |
| cargo check --workspace --no-default-features | |
| cargo deny check --show-stats | |
| cargo clippy --workspace --all-targets --all-features -- -D warnings | |
| RUSTDOCFLAGS="-D warnings" cargo doc --workspace --all-features --no-deps | |
| cargo test --workspace --all-targets --all-features | |
| cargo check --workspace --all-targets --locked | |
| cargo check --workspace --no-default-features --locked | |
| cargo clippy --workspace --all-targets --all-features --locked -- -D warnings | |
| RUSTDOCFLAGS="-D warnings" cargo doc --workspace --all-features --locked --no-deps | |
| cargo test --workspace --all-targets --all-features --locked |
🤖 Prompt for 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.
In `@rs/justfile` around lines 53 - 57, Add the `--locked` flag to all the cargo
commands in the justfile task: specifically to `cargo check`, `cargo clippy`,
`cargo doc`, and `cargo test`. This flag enforces that the exact versions from
Cargo.lock are used during CI resolution, preventing lockfile drift and ensuring
reproducible deterministic builds. Append `--locked` to each cargo invocation to
maintain dependency stability.
Effectively reverts the crane apparatus from #1801 while keeping the problem #1801 actually fixed: the heavy checks no longer compile into a persistent CARGO_TARGET_DIR (which had grown unbounded to 161G and filled the disk). Instead of crane's whole-deps blob, the runner caches cargo compilation per-crate with sccache, so a Cargo.lock change recompiles only the changed crate + its reverse-deps. ./target stays ephemeral (wiped per job) -- no persistent target dir to bound. The crane checks rebuilt the entire third-party graph on any Cargo.lock change (e.g. adding serde_with = ~tens of minutes when the box is busy), because buildDepsOnly is keyed on the whole lockfile. - nix/overlay.nix: remove the crane check apparatus (moqChecks, checkDeps, checkCommonArgs, checkSrc). - flake.nix: `checks = { }`. `nix flake check` still validates flake eval + dev shell, just without compiling the workspace. - rs/justfile: run all-features clippy/doc/test + the feature-edge cargo checks here, as plain cargo. - justfile: refresh the `nix flake check` gate comment. sccache is wired into the runner environment (see the oci repo), not the repo: jobs just run cargo, and the cache is invisible infra that benefits every repo's Rust jobs. Release artifacts are unchanged -- still built via crane buildPackage, hermetically (the sandbox scrubs the runner env). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kixelated
force-pushed
the
prototype/sccache-checks
branch
from
715986f to
56a8816
Compare
dev advanced 3 commits while this PR was open (#1845 PTS-exposing TS Export + PCR-paced SRT egress, #1847 moq-lite-05 wire sync, #1819 always-on hardware encoders). Re-merged dev and reconciled two conflicts: - nix/overlay.nix: kept this PR's cargo+sccache CI direction (dev's tip still carries the crane `moqChecks`; the cargo switch lives on main, #1821). - rs/moq-mux/src/container/ts/export.rs: combined dev's #1845 Frame-returning `Export::next` (PTS/keyframe-stamped) with this PR's generic `catalog::Catalog` trait (main's #1815 mpegts rename). Updated the moq-cli drain helper to read `frame.payload` and refreshed two stale `scte35::Ext` comments. Verified: moq-mux (279), moq-cli, moq-srt tests pass; workspace clippy -D warnings and fmt clean (excluding the crates that need libva/gstreamer system libs unavailable in this sandbox: moq-video/libmoq/moq-boy/moq-gst); JS type-checks and biome clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BQ9o9paZnRLpYRgbyjUwFV
1 participant
Prototype — not for merge as-is. A/B test for replacing the crane
nix flake checkRust CI with plaincargo, accelerated transparently by sccache on the self-hosted runner.Why
The crane checks build one
buildDepsOnlyblob keyed onCargo.lock. Any dependency add/bump (e.g. addingserde_with) re-hashes that derivation and recompiles the entire third-party graph before clippy/doc/test can start — ~tens of minutes when the box is busy.What this does
flake.nix: unwirechecks({ }).nix flake checkstill validates flake eval + dev shell, but no longer compiles the workspace.rs/justfile: run the all-features clippy/doc/test + feature-edgecargo checks here as plain cargo../targetstays ephemeral (wiped per job).justfile: also gatesnix flake checkto Nix/Rust diffs, and (separately) cancels in-flight runs on PR close.Where sccache lives
Not in this repo. rustc is wrapped by sccache via the runner environment (see the
ociinfra repo):RUSTC_WRAPPER+SCCACHE_DIRin the runner.env. The repo just runscargo; aCargo.lockchange then recompiles only the changed crate + its reverse-deps. Builds inside anix flake checksandbox are unaffected, so release artifacts still build hermetically via cranebuildPackage.A/B to watch
Cargo.lock→ should recompile only the delta, vs. the ~9 min full crane deps rebuild we measured.🤖 Generated with Claude Code