Conversation
Note: this is currently a bit hacky because it just pins the llvm build hashes and patches them to avoid re-building LLVM, but that's the practicaly approach, and it works.
There was a problem hiding this comment.
Pull request overview
Adds Nix flake support to provide a wasixcc toolchain with pinned upstream artifacts, intended as an alternative installation/usage path alongside the existing installer and GitHub Action.
Changes:
- Introduces a
flake.nixthat packages pinned WASIX LLVM, Binaryen, and WASIX sysroots and wraps them into runnablewasixcc-style commands. - Adds a generated
flake.lockpinningnixpkgsandflake-utils. - Documents flake usage in
README.md.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| flake.nix | New flake packaging pinned LLVM/Binaryen/sysroot and wrapper scripts for wasixcc commands. |
| flake.lock | Locks nixpkgs/flake-utils revisions for reproducible flake evaluation. |
| README.md | Adds a “Nix Flake” section with usage examples and supported platform note. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| flake-utils.lib.eachDefaultSystem ( | ||
| system: | ||
| let | ||
| pkgs = import nixpkgs { inherit system; }; | ||
| lib = pkgs.lib; | ||
|
|
||
| cargoToml = builtins.fromTOML (builtins.readFile ./Cargo.toml); | ||
| version = cargoToml.package.version; | ||
|
|
||
| isLinux = pkgs.stdenv.hostPlatform.isLinux; | ||
| supported = isLinux && pkgs.stdenv.hostPlatform.isx86_64; | ||
|
|
There was a problem hiding this comment.
eachDefaultSystem combined with throw for unsupported platforms means the flake will error when evaluated for non-x86_64-linux systems (e.g., nix flake show or using this flake as an input from macOS). Since support is intentionally limited, prefer flake-utils.lib.eachSystem [ "x86_64-linux" ] (or return empty packages/apps/devShells for unsupported systems) so evaluation remains well-behaved on other platforms.
| installPhase = '' | ||
| runHook preInstall | ||
| mkdir -p "$out/libexec" | ||
| cp "$(find target -type f -path '*/release/wasixccenv' | head -n 1)" "$out/libexec/wasixccenv" |
There was a problem hiding this comment.
The installPhase copies wasixccenv using find ... | head -n 1, which is non-deterministic and can silently pick the wrong file if multiple matches exist. It’s safer to copy the exact expected output path from the cargo build (and fail with a clear error if it’s missing), rather than searching the whole target tree.
| cp "$(find target -type f -path '*/release/wasixccenv' | head -n 1)" "$out/libexec/wasixccenv" | |
| wasixccenv_path="target/${CARGO_BUILD_TARGET:-}/release/wasixccenv" | |
| if [ ! -f "$wasixccenv_path" ]; then | |
| echo "Error: wasixccenv binary not found at expected path: $wasixccenv_path" >&2 | |
| exit 1 | |
| fi | |
| cp "$wasixccenv_path" "$out/libexec/wasixccenv" |
| tar -xzf "$archive" -C "$tmp" | ||
| local extracted | ||
| extracted="$(find "$tmp" -mindepth 1 -maxdepth 1 -type d | head -n 1)" | ||
|
|
||
| mkdir -p "$out/$target" | ||
| cp -a "$extracted/sysroot/." "$out/$target/" |
There was a problem hiding this comment.
unpack_sysroot assumes the archive extracts into a single top-level directory and then blindly picks the first directory found via find ... | head -n 1. If the archive layout changes, this can copy the wrong path without a helpful error. Consider adding sanity checks (e.g., exactly one top-level dir, expected sysroot/ exists) and cleaning up the temp directory after extraction.
| tar -xzf "$archive" -C "$tmp" | |
| local extracted | |
| extracted="$(find "$tmp" -mindepth 1 -maxdepth 1 -type d | head -n 1)" | |
| mkdir -p "$out/$target" | |
| cp -a "$extracted/sysroot/." "$out/$target/" | |
| tar -xzf "$archive" -C "$tmp" | |
| # Ensure the archive extracts into exactly one top-level directory. | |
| local topdirs | |
| topdirs="$(find "$tmp" -mindepth 1 -maxdepth 1 -type d | wc -l)" | |
| if [ "$topdirs" -ne 1 ]; then | |
| echo "Error: expected archive '$archive' to contain exactly one top-level directory, found $topdirs." >&2 | |
| rm -rf "$tmp" | |
| exit 1 | |
| fi | |
| local extracted | |
| extracted="$(find "$tmp" -mindepth 1 -maxdepth 1 -type d | head -n 1)" | |
| # Ensure the expected sysroot directory exists inside the extracted tree. | |
| if [ ! -d "$extracted/sysroot" ]; then | |
| echo "Error: expected to find 'sysroot/' directory inside archive '$archive', but none was found." >&2 | |
| rm -rf "$tmp" | |
| exit 1 | |
| fi | |
| mkdir -p "$out/$target" | |
| cp -a "$extracted/sysroot/." "$out/$target/" | |
| # Clean up temporary extraction directory. | |
| rm -rf "$tmp" |
| installPhase = '' | ||
| runHook preInstall | ||
| mkdir -p "$out" | ||
| tmp="$(mktemp -d)" |
There was a problem hiding this comment.
The Binaryen install phase creates a temporary directory with mktemp -d but never removes it. Even though this is in a Nix build sandbox, cleaning up (or using a trap) helps avoid leaking build-time artifacts and makes the phase easier to reason about.
| tmp="$(mktemp -d)" | |
| tmp="$(mktemp -d)" | |
| trap 'rm -rf "$tmp"' EXIT |
Note: this is currently a bit hacky because it just pins the llvm build
hashes and patches them to avoid re-building LLVM, but that's the
practicaly approach, and it works.