fix: add musl targets for Linux installer fallback#1013
fix: add musl targets for Linux installer fallback#1013ilblackdragon merged 3 commits intonearai:stagingfrom
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
zmanian
left a comment
There was a problem hiding this comment.
Good change overall -- the musl target additions are correct and the rig-core TLS switch is the right call. Two issues to address before merging:
1. Missing Cargo.lock update (blocking)
The diff only touches Cargo.toml but does not include an updated Cargo.lock. The rig-core feature change from default (reqwest-tls -> native-tls -> openssl-sys) to reqwest-rustls will not take effect until the lock file is regenerated. I confirmed that on the current lock file, native-tls and openssl-sys are still present, pulled in exclusively through the rig-core -> reqwest chain. Please run cargo generate-lockfile (or cargo update -p rig-core) and include the updated lock file. Without this, the musl builds will fail trying to link OpenSSL.
2. No custom runner entries for musl targets (non-blocking, worth considering)
The [workspace.metadata.dist.github-custom-runners] section has entries for all existing targets but none for the two new musl targets. cargo-dist v0.30.3 will fall back to its default Ubuntu runner, which should work since it auto-installs musl-tools, but you may want to pin specific runners for reproducibility (e.g., x86_64-unknown-linux-musl = "ubuntu-22.04" and aarch64-unknown-linux-musl = "ubuntu-24.04-arm"). This is optional but worth noting.
Everything else looks correct:
- Target triple names are right (
x86_64-unknown-linux-musl,aarch64-unknown-linux-musl) - The rig-core feature switch to
reqwest-rustlsis the clean fix -- eliminates the only path to openssl-sys in the tree - Existing glibc targets are untouched, no regression risk there
- cargo-dist's installer already has the fallback logic to prefer glibc and fall back to musl, so no installer script changes are needed
0b28b3f to
54c41c0
Compare
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
zmanian
left a comment
There was a problem hiding this comment.
The approach is correct -- adding musl targets for glibc fallback and switching rig-core to rustls to eliminate the OpenSSL dependency that would break static builds.
However, there are issues:
1. CI is not running full checks (blocking)
Only classify and scope CI jobs ran. The main build, clippy, and test jobs did not execute. This is a significant concern because:
- The Cargo.lock changes are substantial (clap, anstream version bumps, OpenSSL removal)
- We need to verify that removing
native-tls/openssl-sysdoesn't break anything downstream - The musl target configuration needs validation
Please rebase and ensure full CI runs before this can be reviewed properly.
2. Verify cargo-dist musl toolchain support
The PR mentions cargo-dist v0.30.3 handles musl toolchains automatically, but the CI runners are set to ubuntu-22.04 and ubuntu-24.04-arm. Confirm that musl-tools / musl-gcc are available on these runners or that cargo-dist installs them.
3. Unrelated dependency bumps in Cargo.lock
The lock file includes version bumps for clap (4.5.60 -> 4.6.0), anstream (0.6.21 -> 1.0.0), once_cell (1.21.3 -> 1.21.4) that appear unrelated to the rig-core TLS change. Were these intentional or a side effect of cargo update? Unrelated dep changes should ideally be in a separate PR to keep the diff reviewable.
4. PR description is good
The root cause analysis and reviewer note are well-written. The rig-core feature flag change (default-features = false, features = ["reqwest-rustls"]) is the right approach.
Please get full CI green and address the dep bump question, then this should be ready.
The installer fails on systems with glibc < 2.35 (e.g. Amazon Linux 2023) because only gnu targets are built and there is no static fallback. - Add x86_64-unknown-linux-musl and aarch64-unknown-linux-musl to the cargo-dist target list so the installer can fall back to statically linked binaries when glibc is too old. - Switch rig-core from reqwest-tls (OpenSSL) to reqwest-rustls (pure Rust TLS) to avoid a system OpenSSL dependency that breaks musl builds. Closes nearai#1008
Address review feedback: - Regenerate Cargo.lock to reflect rig-core reqwest-rustls switch, removing openssl-sys and native-tls from the dependency tree - Add github-custom-runners entries for musl targets
54c41c0 to
bca8bbc
Compare
Addressing review feedback (rebased, v3)Changes in this update:
Feedback items:
Local CI results (matches
|
zmanian
left a comment
There was a problem hiding this comment.
Clean infrastructure fix. Adding musl targets with rustls (pure Rust TLS) eliminates the OpenSSL cross-compilation headaches for statically-linked Linux binaries.
Strengths:
- Removes openssl, native-tls, hyper-tls dependencies (pure Rust TLS stack now)
- Adds both x86_64 and aarch64 musl targets as fallback installers
- All CI checks pass
Post-merge suggestion: Verify in CI logs that musl-tools gets installed on Linux runners and both musl builds succeed. Also validate no regressions from the socket2 version change (0.6.3 -> 0.5.10).
Approving.
ilblackdragon
left a comment
There was a problem hiding this comment.
Code Review
Clean, well-motivated fix for the glibc version mismatch on systems like Amazon Linux 2023.
Changes are sound:
- musl targets added with correct CI runner mappings
rig-coreswitched fromnative-tlstoreqwest-rustls, cleanly removingopenssl-sysand all transitive OpenSSL dependencieswebpki-rootscorrectly added to provide bundled Mozilla root CAs for static builds
One note for release notes: The switch from native-tls to rustls means TLS verification now uses a bundled Mozilla root CA set instead of the OS certificate store. Environments with custom/corporate CA certificates not in Mozilla's bundle will fail TLS verification. Worth calling out for users behind corporate proxies with MITM certs.
Minor: The Cargo.lock includes some transitive version bumps (windows-sys, socket2, getrandom) — these are from the rig-core feature switch resolution, not manually introduced.
Verify the first release build after merge installs musl-tools on the CI runners as expected.
LGTM.
…-targets fix: add musl targets for Linux installer fallback
Summary
x86_64-unknown-linux-muslandaarch64-unknown-linux-muslto the cargo-dist target list so the installer can fall back to statically linked binaries when the system glibc is too oldrig-corefromreqwest-tls(default, pulls in OpenSSL vianative-tls) toreqwest-rustls(pure Rust TLS) — this eliminates theopenssl-systransitive dependency that would break musl/static buildsRoot cause
The installer requires glibc >= 2.35 (x86_64) / >= 2.39 (aarch64) because the release binaries are built on Ubuntu 22.04/24.04. Systems like Amazon Linux 2023 (glibc 2.34) get:
The cargo-dist installer already has fallback logic for musl builds, but no musl targets were configured.
Note for reviewers
After merging, the release workflow will need musl toolchains on CI runners. cargo-dist v0.30.3 handles this automatically when musl targets are present, but verify the generated CI installs
musl-toolson the Linux runners.Closes #1008