fix: remove cargo update to preserve Cargo.lock versions - #1320
Conversation
cargo update overwrites Cargo.lock with latest semver-compatible versions, which broke cass by bumping digest/sha2 beyond what the repo code supports. Just use cargo +nightly with the locked deps.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughA Cargo build step in the local binaries update script was simplified by removing the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Mesa DescriptionTL;DRRemove What changed?
Description generated by Mesa. Update settings |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of ff076dd...2ca3d7c
Analysis
• Stale Repository Vulnerability: The PR respects upstream Cargo.lock files for reproducibility, but there's no mechanism to update local repository checkouts before building. This could perpetuate outdated source code and dependency versions indefinitely, potentially including known security vulnerabilities.
• Missing Repository Update Logic: The build script operates on local repositories without visible pre-build update steps (e.g., git pull). Users must manually update repositories or remember to run separate refresh commands, creating a gap between intended reproducibility and actual security maintenance.
• Incomplete Documentation of Build Prerequisites: The change assumes users understand they need to manually update repository checkouts, but this dependency isn't documented or enforced in the script itself, risking stale builds without clear guidance.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
0 files reviewed | 1 comments | Edit Agent Settings • Read Docs
| fi | ||
| elif [ -f "$build_dir/Cargo.toml" ]; then | ||
| if (cd "$build_dir" && cargo update 2>&1 && cargo +nightly build --release 2>&1); then | ||
| if (cd "$build_dir" && cargo +nightly build --release 2>&1); then |
There was a problem hiding this comment.
Removing cargo update preserves upstream dependency versions, which is good for reproducibility. However, this script doesn't appear to pull/update the git repositories before building. Consider adding a git pull or similar mechanism to ensure you're building the latest upstream code, otherwise you may be building stale versions with stale dependencies. This would be particularly important if these repositories receive security updates.
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#1320
File: scripts/update-local-binaries.sh#L161
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Removing `cargo update` preserves upstream dependency versions, which is good for reproducibility. However, this script doesn't appear to pull/update the git repositories before building. Consider adding a `git pull` or similar mechanism to ensure you're building the latest upstream code, otherwise you may be building stale versions with stale dependencies. This would be particularly important if these repositories receive security updates.
![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 modifies the scripts/update-local-binaries.sh script to remove the cargo update step when building Rust projects, which helps in maintaining consistent dependency versions. A review comment suggests further improving this by adding the --locked flag to the cargo build command to ensure the build strictly adheres to the Cargo.lock file and fails if it is out of sync.
| fi | ||
| elif [ -f "$build_dir/Cargo.toml" ]; then | ||
| if (cd "$build_dir" && cargo update 2>&1 && cargo +nightly build --release 2>&1); then | ||
| if (cd "$build_dir" && cargo +nightly build --release 2>&1); then |
There was a problem hiding this comment.
To fully adhere to the goal of preserving Cargo.lock versions as stated in the PR title and description, consider adding the --locked flag. This ensures that the build will use the exact dependency versions specified in the lockfile and will fail if the lockfile is out of sync with Cargo.toml, preventing any unintended automatic updates during the build process.
| if (cd "$build_dir" && cargo +nightly build --release 2>&1); then | |
| if (cd "$build_dir" && cargo +nightly build --release --locked 2>&1); then |
There was a problem hiding this comment.
Pull request overview
Removes cargo update from the local binary build script so it doesn’t overwrite upstream Cargo.lock, while keeping the existing nightly release build behavior.
Changes:
- Drop
cargo updatefrom the Rust build path inupdate-local-binaries.sh - Continue using
cargo +nightly build --releasefor Rust repos
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fi | ||
| elif [ -f "$build_dir/Cargo.toml" ]; then | ||
| if (cd "$build_dir" && cargo update 2>&1 && cargo +nightly build --release 2>&1); then | ||
| if (cd "$build_dir" && cargo +nightly build --release 2>&1); then |
There was a problem hiding this comment.
To fully align with the goal of preserving Cargo.lock, consider adding --locked to the cargo build invocation. Without --locked, Cargo can still rewrite the lockfile in some scenarios (e.g., lockfile format normalization, resolver differences, or when the lock is out-of-date), which can leave the repo dirty and undermines the intent of removing cargo update.
| if (cd "$build_dir" && cargo +nightly build --release 2>&1); then | |
| if (cd "$build_dir" && cargo +nightly build --release --locked 2>&1); then |
Summary
cargo updatefromupdate-local-binaries.shto avoid overwriting upstreamCargo.lockcargo +nightly build --releasefrom fix: use cargo nightly and update deps for local binary builds #1319Test plan
🤖 Generated with Claude Code
Summary by cubic
Remove
cargo updatefromscripts/update-local-binaries.shto respect upstreamCargo.lockand avoid unexpected dependency upgrades that can break builds (e.g.,digest/sha2bumps in cass). The script now only runscargo +nightly build --releasefor reproducible builds with locked deps.Written for commit 2ca3d7c. Summary will update on new commits.