Skip to content

fix: build --lib only for .rlib targets to skip harness binaries #1397

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shunkakinoki merged 3 commits into from
Apr 6, 2026
Merged

fix: build --lib only for .rlib targets to skip harness binaries #1397

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4a34076
fix: build --lib only for .rlib targets to skip harness binaries
shunkakinoki Apr 6, 2026
1bd0516
fix: use array for cargo extra flags to satisfy shellcheck and shell-…
shunkakinoki Apr 6, 2026
fea4628
chore: apply formatter
shunkakinoki Apr 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion scripts/update-local-binaries.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,14 @@ build_repo() {
fi

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

@gemini-code-assist gemini-code-assist Bot Apr 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

@gemini-code-assist gemini-code-assist Bot Apr 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested 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
# 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

Copilot AI Apr 6, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

Copilot uses AI. Check for mistakes.

@cubic-dev-ai cubic-dev-ai Bot Apr 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Fix with Cubic

return 0
Comment on lines 174 to 183

Copilot AI Apr 6, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
else
return 1
Expand Down
Loading