fix: set zigbuild for ubuntu#355
Conversation
📝 WalkthroughWalkthroughThe build script now unconditionally enables zigbuild for both targets, removing the prior conditional that disabled it on Linux x86_64. All builds will consistently use ChangesBuild Tool Standardization
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![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 scripts/host-aware-build.sh to always use zigbuild for better portability. The reviewer pointed out that this change leaves os_name and arch variables unused and makes several conditional checks redundant, suggesting a cleanup of the dead code to improve maintainability.
| arch=$(uname -m || echo unknown) | ||
|
|
||
| # Always use zigbuild so binaries are portable between host and container. | ||
| use_zigbuild=true |
There was a problem hiding this comment.
By hardcoding use_zigbuild=true, the variables os_name and arch (defined on lines 24-25) are now unused. Furthermore, the conditional checks if [[ ${use_zigbuild} == true ]]; then on lines 31 and 51 are now redundant, making the corresponding else branches dead code. Consider removing the unused variables and simplifying the control flow to improve the script's maintainability.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 864209aa09
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # Always use zigbuild so binaries are portable between host and container. | ||
| use_zigbuild=true | ||
| if [[ ${os_name} == Linux && ${arch} == x86_64 ]]; then | ||
| use_zigbuild=false | ||
| fi | ||
|
|
||
| if [[ ${target} == "brr" ]]; then | ||
| if [[ ${use_zigbuild} == true ]]; then |
There was a problem hiding this comment.
Setting use_zigbuild=true unconditionally makes scripts/host-aware-build.sh require the cargo-zigbuild plugin on every host, but this script is the default build path from Tiltfile (brr_build_cmd/pet_build_cmd) and the documented just dev-up flow does not install that plugin. On a standard Linux setup, cargo exits with no such command: zigbuild unless cargo-zigbuild is separately installed, so this change turns a previously working Linux x86_64 path into an immediate build failure instead of using the existing musl-gcc fallback.
Useful? React with 👍 / 👎.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/host-aware-build.sh (1)
8-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale header comment no longer reflects the new unconditional-zigbuild strategy.
Lines 10-11 still describe "Linux x86_64: cargo build … with musl-gcc linker", which is the old behaviour. Update the comment to reflect that zigbuild is now used on all platforms.
📝 Proposed fix
-# Selects the correct build strategy based on host OS/arch: -# - macOS: cargo zigbuild --target x86_64-unknown-linux-musl -# - Linux x86_64: cargo build --target x86_64-unknown-linux-musl with musl-gcc linker +# Always uses cargo-zigbuild for portability between host and container binaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/host-aware-build.sh` around lines 8 - 11, The header comment in host-aware-build.sh is stale: update the comment block that lists platform build strategies to state that cargo zigbuild (target x86_64-unknown-linux-musl) is used unconditionally on all supported hosts (including macOS and Linux), remove the old "Linux x86_64: cargo build … with musl-gcc linker" line, and briefly mention that zig is used as the cross-compilation toolchain so the comment accurately describes the current unconditional-zigbuild behavior.
🧹 Nitpick comments (1)
scripts/host-aware-build.sh (1)
30-59: ⚡ Quick winThe
elsebranches are unreachable dead code and can be removed.Because
use_zigbuildis now alwaystrue, theelseblocks at lines 33-37 (brr) and lines 55-59 (pet) can never execute. Removing them reduces maintenance surface and removes the misleading impression that a musl-gcc fallback exists.♻️ Proposed cleanup
if [[ ${target} == "brr" ]]; then - if [[ ${use_zigbuild} == true ]]; then - exec cargo zigbuild --release --features jemalloc --target x86_64-unknown-linux-musl --lib "$@" - else - exec env CC_x86_64_unknown_linux_musl=musl-gcc \ - CARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_LINKER=musl-gcc \ - cargo build --release --features jemalloc --target x86_64-unknown-linux-musl --lib "$@" - fi + exec cargo zigbuild --release --features jemalloc --target x86_64-unknown-linux-musl --lib "$@" elif [[ ${target} == "pet" ]]; then if [[ -n "${SKIP_CROSS_COMPILE:-}" ]]; then exec cargo build -p pet_store - elif [[ ${use_zigbuild} == true ]]; then - exec cargo zigbuild --target x86_64-unknown-linux-musl -p pet_store - else - exec env CC_x86_64_unknown_linux_musl=musl-gcc \ - CARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_LINKER=musl-gcc \ - cargo build --target x86_64-unknown-linux-musl -p pet_store - fi + else + exec cargo zigbuild --target x86_64-unknown-linux-musl -p pet_store + fi elseWith
use_zigbuildand the unusedos_name/archvariables also removed, the top of the script simplifies to:-os_name=$(uname -s || echo unknown) -arch=$(uname -m || echo unknown) - -# Always use zigbuild so binaries are portable between host and container. -use_zigbuild=true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/host-aware-build.sh` around lines 30 - 59, Remove the unreachable musl-gcc fallback branches since use_zigbuild is always true: for the target "brr" keep only the cargo zigbuild invocation (exec cargo zigbuild --release --features jemalloc --target x86_64-unknown-linux-musl --lib "$@") and remove the else block that sets CC_x86_64_unknown_linux_musl / CARGO_TARGET_* and runs cargo build; likewise for the target "pet" keep only the cargo zigbuild path (exec cargo zigbuild --target x86_64-unknown-linux-musl -p pet_store) and remove the SKIP_CROSS_COMPILE/else musl-gcc fallback; ensure you also remove any now-unused comments referring to musl-gcc and the branches referencing use_zigbuild so the script only contains the zigbuild paths for "brr" and "pet".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/host-aware-build.sh`:
- Around line 24-28: Remove the now-unused os_name and arch variables set by
uname in the host-aware-build script (they are unused) and update the test
harness in tests/curl_harness.rs: replace the #[cfg(not(target_os = "macos"))]
branch that invokes cargo build with musl-gcc with a matching zigbuild
invocation so the harness uses the same always-zigbuild toolchain as the script;
ensure the replacement preserves any existing build flags/env setup and the test
still produces a Linux-compatible binary for the curl_harness test.
---
Outside diff comments:
In `@scripts/host-aware-build.sh`:
- Around line 8-11: The header comment in host-aware-build.sh is stale: update
the comment block that lists platform build strategies to state that cargo
zigbuild (target x86_64-unknown-linux-musl) is used unconditionally on all
supported hosts (including macOS and Linux), remove the old "Linux x86_64: cargo
build … with musl-gcc linker" line, and briefly mention that zig is used as the
cross-compilation toolchain so the comment accurately describes the current
unconditional-zigbuild behavior.
---
Nitpick comments:
In `@scripts/host-aware-build.sh`:
- Around line 30-59: Remove the unreachable musl-gcc fallback branches since
use_zigbuild is always true: for the target "brr" keep only the cargo zigbuild
invocation (exec cargo zigbuild --release --features jemalloc --target
x86_64-unknown-linux-musl --lib "$@") and remove the else block that sets
CC_x86_64_unknown_linux_musl / CARGO_TARGET_* and runs cargo build; likewise for
the target "pet" keep only the cargo zigbuild path (exec cargo zigbuild --target
x86_64-unknown-linux-musl -p pet_store) and remove the SKIP_CROSS_COMPILE/else
musl-gcc fallback; ensure you also remove any now-unused comments referring to
musl-gcc and the branches referencing use_zigbuild so the script only contains
the zigbuild paths for "brr" and "pet".
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 117813a6-737f-4fa7-8d08-58e048c00a88
📒 Files selected for processing (1)
scripts/host-aware-build.sh
| os_name=$(uname -s || echo unknown) | ||
| arch=$(uname -m || echo unknown) | ||
|
|
||
| # Always use zigbuild so binaries are portable between host and container. | ||
| use_zigbuild=true |
There was a problem hiding this comment.
os_name/arch are now unused, and curl_harness.rs diverges from the new always-zigbuild strategy.
Two related points:
-
Unused variables —
os_name(line 24) andarch(line 25) are set viaunamebut are no longer referenced anywhere. They can be removed. -
Test harness divergence (higher impact) —
tests/curl_harness.rslines 348-358 still use a#[cfg(not(target_os = "macos"))]branch that invokescargo buildwithmusl-gccon Linux x86_64, while this script now always callscargo zigbuild. The two paths produce binaries through different toolchains, meaning CI runs that exercise the test harness on Linux still validate the old musl-gcc path — the new zigbuild-on-Linux path remains untested.This matters because
curl_harness.rsline 3-4 explicitly warns that incorrect musl cross-compilation can corrupt the binary and causeSIGSEGV (exit 139). The test harness should be updated to match this script so that integration tests actually cover the toolchain now in use:// In tests/curl_harness.rs, replace the #[cfg(not(target_os = "macos"))] block // (lines ~348-358) that uses musl-gcc with a zigbuild invocation, mirroring // the always-zigbuild approach adopted here.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 24-24: os_name appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 25-25: arch appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/host-aware-build.sh` around lines 24 - 28, Remove the now-unused
os_name and arch variables set by uname in the host-aware-build script (they are
unused) and update the test harness in tests/curl_harness.rs: replace the
#[cfg(not(target_os = "macos"))] branch that invokes cargo build with musl-gcc
with a matching zigbuild invocation so the harness uses the same always-zigbuild
toolchain as the script; ensure the replacement preserves any existing build
flags/env setup and the test still produces a Linux-compatible binary for the
curl_harness test.
1 participant
use zigbuild for ubuntu for portability between host & container binary.
Summary by CodeRabbit