feat: automatically resolve zig instead of requiring an exact system version - #231
Open
CptnKirk wants to merge 8 commits into
Open
feat: automatically resolve zig instead of requiring an exact system version#231CptnKirk wants to merge 8 commits into
CptnKirk wants to merge 8 commits into
Conversation
…atch
Burrito.wrap/1 used to exit(1) up front if `zig` wasn't on PATH at all,
or if it didn't match the pinned version exactly. That's a hard blocker
for anyone without zig installed, or whose system zig has moved on (e.g.
a rolling-release distro) since Burrito last bumped its pin.
Add Burrito.Util.ZigResolver, used by a new ResolveZig fetch step, that
resolves a working zig in this order:
1. BURRITO_ZIG_PATH env var, if set (explicit override)
2. a previously downloaded, managed copy at the pinned version
3. system zig on PATH, if its version matches exactly (today's
behavior, unchanged, for anyone already set up correctly)
4. otherwise, download the pinned version for the host OS/CPU from
ziglang.org, verify its sha256 against a hardcoded checksum, cache
it, and use that
Only the exact pinned version is ever used to build -- Burrito's own
build.zig sources have only ever been compatible with one zig version
at a time (see the 0.15 -> 0.16 migration in burrito-elixir#221/burrito-elixir#225), so this isn't
a relaxation of which zig gets used, just of where it has to come from.
The resolved path threads through Context (a new zig_bin field) to both
System.cmd("zig", ...) call sites in Steps.Build.PackAndBuild, which
previously always shelled out to bare "zig" on PATH regardless of what
pre_check had verified.
…e dir File.rename!/2 (like POSIX rename(2)) cannot cross a device boundary. System.tmp_dir!() is frequently a separate filesystem (tmpfs) from the managed cache directory under $XDG_CACHE_HOME -- caught live by a sprite test where /tmp and ~/.cache were on different mounts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRBbtQUQJTce1uASHx4en5
Steps.Patch.RecompileNIFs shells out to `make` with CC/CXX/AR/RANLIB set to bare `zig cc`/`zig c++`/`zig ar`/`zig ranlib` strings, executed by make's own subshell -- a third call site relying on `zig` being on PATH, missed by the first pass since it's reached only when a dependency has a NIF (elixir_make in its compilers list). Caught live: the nozig sprite test got past ResolveZig and the wrapper build, then failed recompiling exqlite's NIF with "zig: not found". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRBbtQUQJTce1uASHx4en5
concurrency-safe managed install, quote zig_bin in shell strings
Four issues from a self-review of the zig-auto-resolve branch, all
introduced or exacerbated by this branch (a fifth finding -- Req.get!
raising instead of returning {:error, _} -- was left alone since it's
an existing FetchMusl convention this branch only followed, not one it
introduced or worsened):
- Extracted the proxy-aware Req.get! logic (previously duplicated
near-verbatim from Fetch.FetchMusl.do_download/2) into a shared
Burrito.Util.Downloader.get!/1, used by both FetchMusl and
ZigResolver. Also routed ZigResolver's downloaded archive bytes
through the existing Burrito.Util.FileCache, same as FetchMusl
already does for the musl runtime, instead of a second bespoke
"does this already exist" mechanism.
- BURRITO_ZIG_PATH now runs through the same zig_version_at/1 check
every other resolution path uses, instead of a bare File.exists?
(which also passed for directories). A wrong-version or non-zig
override now fails clearly at resolution time instead of confusingly
deep inside a build step.
- The managed-zig install path had a real concurrency gap: the
extraction scratch dir was named with :erlang.unique_integer/1
alone, which is unique per-BEAM-instance, not across OS processes,
so two concurrent `mix release` invocations could collide; and the
final install swap (rm_rf + rename) had no protection against a
concurrent installer's directory being deleted out from under it.
Now: the scratch dir also includes System.pid(), and the install
swap is first-writer-wins -- if a concurrent build already finished
installing this exact version, adopt it and discard our own
extraction instead of overwriting.
- CC/CXX/AR/RANLIB are built as shell command strings that make hands
to /bin/sh -c. Before this branch the interpolated value was always
the literal word "zig", which can't contain a space; now it's a
resolved filesystem path (the managed cache dir under the user's
cache home, or a BURRITO_ZIG_PATH override), which can. Added a
small POSIX single-quote helper and applied it to zig_bin in all
four env vars.
Also added a narrow extraction-time check: the one path our own code
touches (the sole top-level archive entry we go on to File.rename!)
must resolve inside the scratch directory. This is a real but partial
mitigation -- it can't retroactively catch a malicious *nested* member
written outside the scratch dir during extraction itself, since a
post-hoc scan can only see what landed inside the directory it's
scanning. The primary defense stays the sha256 checksum verified
strictly before extraction ever runs.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LRBbtQUQJTce1uASHx4en5
A deeper style-fit pass compared zig_resolver.ex against the two direct precedents for this exact failure mode (default_erts_resolver.ex, fetch_musl.ex): both name the helper do_download, and both end the download-failure message with "please file an issue! Thanks!". Matched both here so the new resolver reads as native to this codebase rather than as an outside contribution with its own conventions.
build_examples already covers the already-correctly-configured case across hosts, but nothing exercised the two scenarios ZigResolver exists for: no system zig at all, and a system zig that doesn't match the pinned version. Added build_no_system_zig and build_wrong_system_zig, mirroring build_examples's own job shape (this file already repeats near-identical job blocks per scenario for run_examples_windows/ linux/macos, rather than a shared/parameterized workflow). Verified locally against both real scenarios on isolated Fly Sprites before adding: one with no zig on $PATH, one with zig 0.15.2 present (the prior pin). Both build and run cleanly end-to-end (all 5 targets, exit 0) once p7zip was installed on the sprite -- confirmed separately against a real prior CI run that ubuntu-latest already ships p7zip by default, so this isn't a gap the real runner will hit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JB25iAFxMVFobn96APu8dW
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Burrito currently requires the system
zigon$PATHto match its pinned version exactly (check_zig_version/0inlib/burrito.ex,exit(1)on any mismatch). Every zig version bump — 0.15.2, then 0.16.0 — breaks every downstream project until each one manually reinstalls the new pinned zig. There's also no way to build at all without a system zig present.This is causing me pain since Expert (lsp server) ships as a burrito but can't know what version of zig, if any, will be installed on build hosts. My fix is for Burrito to manage its own zig dependency automatically, the same way it already manages its own ERTS dependency, rather than requiring the build host to have the right one pre-installed.
Fix:
Burrito.Util.ZigResolverresolves zig automatically, in order:BURRITO_ZIG_PATHenv var, if set — an explicit override, still validated (must exist and report the pinned version) rather than trusted blindly.zigon$PATH, if its version matches the pinned version exactly — unchanged behavior for anyone already correctly set up.Only the exact pinned version is ever used to build —
build.zighas historically only compiled against one zig API generation at a time (see the 0.15 → 0.16 migration), so this isn't a relaxation of which zig gets used, just of what has to already be on the system.Wiring:
Burrito.Steps.Fetch.ResolveZigstep, added to the:fetchphase, setscontext.zig_bin(or halts with a logged error).context.zig_binthreaded through both existingSystem.cmd("zig", ...)call sites inpack_and_build.exand through NIF cross-compilation env vars (CC/CXX/AR/RANLIB) inrecompile_nifs.ex, shell-quoted since the resolved path can contain spaces.check_zig_version/0removed fromlib/burrito.ex;pre_check/0still checks forxz, and now warns instead of hard-erroring on missing7z/7zz.fetch_musl.ex,default_erts_resolver.ex, and now this) into a sharedBurrito.Util.Downloader.Also touches CI — calling this out separately since it's not just application code: added two new jobs to
burrito-xcomp-check.yaml,build_no_system_zigandbuild_wrong_system_zig, mirroringbuild_examples's existing shape. Each builds and runs thecli_examplerelease onubuntu-latest— one with no zig on$PATHat all, one with the prior 0.15.2 pin present instead of the current one — so both resolver fallback tiers stay covered automatically on every future PR. Happy to split this into its own PR if you'd rather review the CI change separately from the resolver itself.Verified against both real failure modes, twice over: first by building expert (the official Elixir LSP, whose
justfiledepends on Burrito) end-to-end on two isolated Fly Sprites — one with no zig installed at all, one with zig 0.15.2 present (incompatible with the 0.16.0 pin) — both of which fail onmaintoday and succeed on this branch; then again by the two new CI jobs above, which exercise the same two scenarios automatically going forward.Kept intentionally narrow: this branch is independent of #229/#230 (the unrelated argv/VM-halt regression) and builds cleanly against current
main. Combined end-to-end validation against #230 was done in a separate, throwaway branch, not included here.