fix: build --lib only for .rlib targets to skip harness binaries - #1397
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
Mesa DescriptionTL;DROptimized What changed?
Description generated by Mesa. Update settings |
![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 build process in scripts/update-local-binaries.sh to conditionally include the --lib flag when the target is a .rlib file, optimizing builds in large workspaces. Review feedback highlights a regression in the test suite due to the changed command string and suggests using Bash arrays for more robust flag handling along with support for additional library file extensions.
| if [[ "$expanded_bin" == *.rlib ]]; then | ||
| cargo_flags="--release --lib" | ||
| fi | ||
| if (cd "$build_dir" && cargo +nightly build $cargo_flags 2>&1); then |
There was a problem hiding this comment.
This change will cause the test at line 166 of spec/update_local_binaries_spec.sh to fail. The test uses grep to look for the literal string cargo +nightly build --release in the script, which no longer exists in that form. Please update the test suite to accommodate this change.
| # If the target is a .rlib, only build the library (avoids compiling all | ||
| # test/harness binaries in large workspaces like frankensqlite). | ||
| local cargo_flags="--release" | ||
| local expanded_bin="${binary_path/#\~/$HOME}" | ||
| if [[ "$expanded_bin" == *.rlib ]]; then | ||
| cargo_flags="--release --lib" | ||
| fi | ||
| if (cd "$build_dir" && cargo +nightly build $cargo_flags 2>&1); then |
There was a problem hiding this comment.
Using an array for cargo_flags is more robust and maintainable than string concatenation, as it correctly handles argument boundaries and makes it easier to extend with more flags. Additionally, the tilde expansion for expanded_bin is not strictly necessary for the extension check within [[ ... ]] as the pattern match works on the raw string.
You might also consider supporting other library extensions like .a, .so, and .dylib for consistency with sync-local-binaries.sh.
| # If the target is a .rlib, only build the library (avoids compiling all | |
| # test/harness binaries in large workspaces like frankensqlite). | |
| local cargo_flags="--release" | |
| local expanded_bin="${binary_path/#\~/$HOME}" | |
| if [[ "$expanded_bin" == *.rlib ]]; then | |
| cargo_flags="--release --lib" | |
| fi | |
| if (cd "$build_dir" && cargo +nightly build $cargo_flags 2>&1); then | |
| # If the target is a .rlib, only build the library (avoids compiling all | |
| # test/harness binaries in large workspaces like frankensqlite). | |
| local cargo_flags=("--release") | |
| if [[ "$binary_path" == *.rlib ]]; then | |
| cargo_flags+=("--lib") | |
| fi | |
| if (cd "$build_dir" && cargo +nightly build "${cargo_flags[@]}" 2>&1); then |
There was a problem hiding this comment.
Pull request overview
Updates the local binaries updater script to avoid building unnecessary Rust workspace targets when the configured artifact is a .rlib, significantly reducing build time for large workspaces.
Changes:
- Detect
.rlibtargets and add--libto thecargo buildinvocation. - Keep default behavior (
--release) for non-.rlibRust targets.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [ -f "$build_dir/Cargo.toml" ]; then | ||
| if (cd "$build_dir" && cargo +nightly build --release 2>&1); then | ||
| # If the target is a .rlib, only build the library (avoids compiling all | ||
| # test/harness binaries in large workspaces like frankensqlite). | ||
| local cargo_flags="--release" | ||
| local expanded_bin="${binary_path/#\~/$HOME}" | ||
| if [[ "$expanded_bin" == *.rlib ]]; then | ||
| cargo_flags="--release --lib" | ||
| fi | ||
| if (cd "$build_dir" && cargo +nightly build $cargo_flags 2>&1); then | ||
| return 0 |
There was a problem hiding this comment.
This change replaces the literal cargo +nightly build --release command with cargo +nightly build $cargo_flags, which will break the existing shell spec that greps for cargo +nightly build --release (see spec/update_local_binaries_spec.sh). Update the spec to assert the new behavior (including the new --lib case for .rlib targets) rather than matching the old hard-coded command string.
| local cargo_flags="--release" | ||
| local expanded_bin="${binary_path/#\~/$HOME}" | ||
| if [[ "$expanded_bin" == *.rlib ]]; then | ||
| cargo_flags="--release --lib" | ||
| fi | ||
| if (cd "$build_dir" && cargo +nightly build $cargo_flags 2>&1); then |
There was a problem hiding this comment.
Consider storing cargo flags in a bash array (e.g., cargo_flags=(--release) and conditionally appending --lib) and invoking cargo with "${cargo_flags[@]}". This avoids relying on word-splitting of $cargo_flags and makes the command more robust if additional flags are added later.
| local cargo_flags="--release" | |
| local expanded_bin="${binary_path/#\~/$HOME}" | |
| if [[ "$expanded_bin" == *.rlib ]]; then | |
| cargo_flags="--release --lib" | |
| fi | |
| if (cd "$build_dir" && cargo +nightly build $cargo_flags 2>&1); then | |
| local cargo_flags=(--release) | |
| local expanded_bin="${binary_path/#\~/$HOME}" | |
| if [[ "$expanded_bin" == *.rlib ]]; then | |
| cargo_flags+=(--lib) | |
| fi | |
| if (cd "$build_dir" && cargo +nightly build "${cargo_flags[@]}" 2>&1); then |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/update-local-binaries.sh">
<violation number="1" location="scripts/update-local-binaries.sh:182">
P2: This change removes the literal `cargo +nightly build --release` string from the script, which will break the existing test in `spec/update_local_binaries_spec.sh` that uses `grep` to match that exact command. The spec needs to be updated to account for the new `$cargo_flags` variable (and ideally assert the `--lib` behavior for `.rlib` targets).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if [[ "$expanded_bin" == *.rlib ]]; then | ||
| cargo_flags="--release --lib" | ||
| fi | ||
| if (cd "$build_dir" && cargo +nightly build $cargo_flags 2>&1); then |
There was a problem hiding this comment.
P2: This change removes the literal cargo +nightly build --release string from the script, which will break the existing test in spec/update_local_binaries_spec.sh that uses grep to match that exact command. The spec needs to be updated to account for the new $cargo_flags variable (and ideally assert the --lib behavior for .rlib targets).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/update-local-binaries.sh, line 182:
<comment>This change removes the literal `cargo +nightly build --release` string from the script, which will break the existing test in `spec/update_local_binaries_spec.sh` that uses `grep` to match that exact command. The spec needs to be updated to account for the new `$cargo_flags` variable (and ideally assert the `--lib` behavior for `.rlib` targets).</comment>
<file context>
@@ -172,7 +172,14 @@ build_repo() {
+ if [[ "$expanded_bin" == *.rlib ]]; then
+ cargo_flags="--release --lib"
+ fi
+ if (cd "$build_dir" && cargo +nightly build $cargo_flags 2>&1); then
return 0
else
</file context>
Summary
update-local-binaries.shencounters a.rlibtarget (e.g.frankensqlite/target/release/libfsqlite.rlib), it now passes--libtocargo buildinstead of building all targetsTest plan
update-local-binaries.shand confirm frankensqlite lib builds without triggering harness binary compilationSummary by cubic
Use
cargo build --libfor.rlibtargets inupdate-local-binaries.shto skip harness/test binaries, leaving other targets unchanged.Pass extra
cargoflags via an array to satisfy ShellCheck/shell tests and apply formatting;frankensqlitedrops from ~4m to ~30–60s on warm cache.Written for commit fea4628. Summary will update on new commits.