Skip to content

fix: set zigbuild for ubuntu #355

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

Open
jay009id wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: set zigbuild for ubuntu #355

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eee49a6
fix: set zigbuild for ubuntu
jay009id Apr 19, 2026
864209a
fix: set zigbuild for ubuntu
jay009id Apr 27, 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
4 changes: 1 addition & 3 deletions scripts/host-aware-build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,8 @@ shift || true
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

@gemini-code-assist gemini-code-assist Bot Apr 27, 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

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.

Comment on lines 24 to 28

@coderabbitai coderabbitai Bot May 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

os_name/arch are now unused, and curl_harness.rs diverges from the new always-zigbuild strategy.

Two related points:

  1. Unused variables — os_name (line 24) and arch (line 25) are set via uname but are no longer referenced anywhere. They can be removed.

  2. Test harness divergence (higher impact) — tests/curl_harness.rs lines 348-358 still use a #[cfg(not(target_os = "macos"))] branch that invokes cargo build with musl-gcc on Linux x86_64, while this script now always calls cargo 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.rs line 3-4 explicitly warns that incorrect musl cross-compilation can corrupt the binary and cause SIGSEGV (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.

if [[ ${os_name} == Linux && ${arch} == x86_64 ]]; then
use_zigbuild=false
fi

if [[ ${target} == "brr" ]]; then
if [[ ${use_zigbuild} == true ]]; then
Comment on lines +27 to 31

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep Linux musl-gcc fallback for host-aware builds

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 👍 / 👎.

Expand Down