Conversation
📝 WalkthroughWalkthroughAdds project editor and direnv configs, a CI step that verifies generated Wasm artifacts are unchanged, broadens .gitignore patterns, restructures flake.nix to expose wasm plugin entries and per-system outputs (lib, packages, devShells, checks, formatter), and reformats many Nix test expressions to multi-line style. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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
🧹 Nitpick comments (1)
flake.nix (1)
96-104: Consider parameterizing the hardcoded clang version.The clang version
19is hardcoded in multiple paths (lines 96, 103, 161-162). If nixpkgs updates to a different LLVM version, these paths will silently break.♻️ Suggested approach
+ clangVersion = builtins.toString llvmPackages.clang.version or "19"; wasiSdk = runCommand "wasi-sdk-compat" { } '' - mkdir -p $out/bin $out/lib/clang/19 $out/share + mkdir -p $out/bin $out/lib/clang/${clangVersion} $out/share ... - ln -s ${wasiCc}/resource-root/include $out/lib/clang/19/include + ln -s ${wasiCc}/resource-root/include $out/lib/clang/${clangVersion}/includeAnd similarly update the
CFLAGS_wasm32_wasip1andBINDGEN_EXTRA_CLANG_ARGS_wasm32_wasip1references.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 96 - 104, Replace the hardcoded "19" clang version with a single variable and use that variable wherever the version is referenced: define a CLANG_VERSION (or clangVersion) near the top of the flake and use it in the symlink targets (e.g. $out/lib/clang/${CLANG_VERSION}/include) and any other paths; also update the CFLAGS_wasm32_wasip1 and BINDGEN_EXTRA_CLANG_ARGS_wasm32_wasip1 occurrences to interpolate the same CLANG_VERSION variable so all references (the ln -s lines and the CFLAGS/BINDGEN args) stay in sync when LLVM/Clang is bumped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 34-39: The CI step "Ensure no changes to generated Wasm" uses
GNU-only `cp --no-preserve=all` which breaks on macOS; replace the `cp -rL
--no-preserve=all ./result ./wasm` line with a portable copy using `rsync`
(e.g., `rsync -aL --delete ./result/ ./wasm/`) so ownership/permission
differences are normalized across runners, keep the preceding `rm -rf ./wasm`
and subsequent `git diff --exit-code` unchanged, and ensure you use trailing
slashes on source/dest to copy contents rather than the directory itself.
---
Nitpick comments:
In `@flake.nix`:
- Around line 96-104: Replace the hardcoded "19" clang version with a single
variable and use that variable wherever the version is referenced: define a
CLANG_VERSION (or clangVersion) near the top of the flake and use it in the
symlink targets (e.g. $out/lib/clang/${CLANG_VERSION}/include) and any other
paths; also update the CFLAGS_wasm32_wasip1 and
BINDGEN_EXTRA_CLANG_ARGS_wasm32_wasip1 occurrences to interpolate the same
CLANG_VERSION variable so all references (the ln -s lines and the CFLAGS/BINDGEN
args) stay in sync when LLVM/Clang is bumped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 358c2804-fb00-4cbf-b40a-41a27ad1bdc8
⛔ Files ignored due to path filters (8)
wasm/nix_wasm_plugin_fib.wasmis excluded by!**/*.wasmwasm/nix_wasm_plugin_fib_wasi.wasmis excluded by!**/*.wasmwasm/nix_wasm_plugin_grep.wasmis excluded by!**/*.wasmwasm/nix_wasm_plugin_ini.wasmis excluded by!**/*.wasmwasm/nix_wasm_plugin_mandelbrot.wasmis excluded by!**/*.wasmwasm/nix_wasm_plugin_quickjs.wasmis excluded by!**/*.wasmwasm/nix_wasm_plugin_test.wasmis excluded by!**/*.wasmwasm/nix_wasm_plugin_yaml.wasmis excluded by!**/*.wasm
📒 Files selected for processing (15)
.editorconfig.envrc.github/workflows/ci.yml.gitignoreflake.nixnix-wasm-plugin-fib/tests/fib.nixnix-wasm-plugin-grep/tests/grep.nixnix-wasm-plugin-ini/tests/test1.nixnix-wasm-plugin-mandelbrot/tests/test1.nixnix-wasm-plugin-test/tests/call.nixnix-wasm-plugin-test/tests/lazy.nixnix-wasm-plugin-test/tests/pure.nixnix-wasm-plugin-yaml/tests/fromYAML.nixnix-wasm-plugin-yaml/tests/toYAML.nixnix-wasm-plugin-yaml/tests/toYAML2.nix
| - name: Ensure no changes to generated Wasm | ||
| run: | | ||
| rm -rf ./wasm | ||
| nix build | ||
| cp -rL --no-preserve=all ./result ./wasm | ||
| git diff --exit-code |
There was a problem hiding this comment.
cp --no-preserve is not portable to macOS.
The --no-preserve=all flag is GNU coreutils-specific. BSD cp on macOS doesn't support this option, causing the workflow to fail on macos-latest runners.
🐛 Proposed fix using a portable approach
- name: Ensure no changes to generated Wasm
run: |
rm -rf ./wasm
nix build
- cp -rL --no-preserve=all ./result ./wasm
+ mkdir -p ./wasm
+ cp -rL ./result/* ./wasm/
git diff --exit-codeAlternatively, if you need to reset ownership/permissions explicitly, use rsync which is available on both platforms:
- name: Ensure no changes to generated Wasm
run: |
rm -rf ./wasm
nix build
- cp -rL --no-preserve=all ./result ./wasm
+ rsync -rL ./result/ ./wasm/
git diff --exit-code📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Ensure no changes to generated Wasm | |
| run: | | |
| rm -rf ./wasm | |
| nix build | |
| cp -rL --no-preserve=all ./result ./wasm | |
| git diff --exit-code | |
| - name: Ensure no changes to generated Wasm | |
| run: | | |
| rm -rf ./wasm | |
| nix build | |
| mkdir -p ./wasm | |
| cp -rL ./result/* ./wasm/ | |
| git diff --exit-code |
| - name: Ensure no changes to generated Wasm | |
| run: | | |
| rm -rf ./wasm | |
| nix build | |
| cp -rL --no-preserve=all ./result ./wasm | |
| git diff --exit-code | |
| - name: Ensure no changes to generated Wasm | |
| run: | | |
| rm -rf ./wasm | |
| nix build | |
| rsync -rL ./result/ ./wasm/ | |
| git diff --exit-code |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 34 - 39, The CI step "Ensure no
changes to generated Wasm" uses GNU-only `cp --no-preserve=all` which breaks on
macOS; replace the `cp -rL --no-preserve=all ./result ./wasm` line with a
portable copy using `rsync` (e.g., `rsync -aL --delete ./result/ ./wasm/`) so
ownership/permission differences are normalized across runners, keep the
preceding `rm -rf ./wasm` and subsequent `git diff --exit-code` unchanged, and
ensure you use trailing slashes on source/dest to copy contents rather than the
directory itself.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
flake.nix (2)
133-139: Keep the WASI crate list in one place.
nix-wasi-pluginshard-codes the WASI allowlist, whilenix-wasm-pluginshard-codes the inverse denylist. The first new WASI crate will need edits in both places, and missing one side will either fail the build or target the wrong backend. Hoisting that list into one binding would remove the drift.If helpful, I can sketch the shared helper attrset.
Also applies to: 181-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 133 - 139, The WASI crate list is duplicated between the nix-wasi-plugins and nix-wasm-plugins logic (one uses an allowlist and the other a denylist), causing drift; instead create a single shared binding (e.g., wasiCrates or wasiAllowedCrates) that enumerates the WASI crate names and replace the hard-coded allowlist/denylist uses in both modules to reference that binding so additions only need to be made in one place; update any places that currently invert the list (denylist logic) to consult the shared binding and compute the complement there if needed (keeping names like nix-wasi-plugins and nix-wasm-plugins to locate where to change).
70-71: Avoid hard-coding LLVM 19 into the WASI SDK shim.This block mixes
rustPackages.rustc.llvmPackageswithpkgs.llvmPackagesand repeatslib/clang/19in the SDK layout and clang args. That makes the build brittle across nixpkgs bumps and easy to skew betweenlld,clang, andlibclang.♻️ Possible cleanup
rustPackages = pkgs.rustPackages_1_89; rustPlatform = rustPackages.rustPlatform; + llvmPkgs = rustPackages.rustc.llvmPackages; + clangMajor = pkgs.lib.versions.major llvmPkgs.libclang.version; rustSysroot = runCommand "rust-sysroot" { } '' mkdir -p $out/lib/rustlib cp -r ${rustPackages."rustc-unwrapped"}/lib/rustlib/* $out/lib/rustlib/ mkdir -p $out/lib/rustlib/src ln -s ${rustPlatform.rustcSrc} $out/lib/rustlib/src/rust @@ ''; wasiSdk = runCommand "wasi-sdk-compat" { } '' - mkdir -p $out/bin $out/lib/clang/19 $out/share + mkdir -p $out/bin $out/lib/clang/${clangMajor} $out/share ln -s ${wasiCc}/bin/wasm32-unknown-wasi-clang $out/bin/clang ln -s ${wasiCc}/bin/wasm32-unknown-wasi-clang++ $out/bin/clang++ ln -s ${wasiCc}/bin/wasm32-unknown-wasi-ar $out/bin/ar ln -s ${wasiCc}/bin/wasm32-unknown-wasi-ld.lld $out/bin/ld.lld - ln -s ${wasiCc}/resource-root/include $out/lib/clang/19/include + ln -s ${wasiCc}/resource-root/include $out/lib/clang/${clangMajor}/include ln -s ${wasiSysroot} $out/share/wasi-sysroot ''; @@ nativeBuildInputs = [ - rustPackages.rustc.llvmPackages.lld + llvmPkgs.lld binaryen - llvmPackages.clang - llvmPackages.libclang + llvmPkgs.clang + llvmPkgs.libclang ]; WASI_SDK = "${wasiSdk}"; - LIBCLANG_PATH = "${llvmPackages.libclang.lib}/lib"; + LIBCLANG_PATH = "${llvmPkgs.libclang.lib}/lib"; RUSTC = "${rustcWithSysroot}/bin/rustc"; CC_wasm32_wasip1 = "${wasiSdk}/bin/clang"; AR_wasm32_wasip1 = "${wasiSdk}/bin/ar"; - CFLAGS_wasm32_wasip1 = "--sysroot=${wasiSdk}/share/wasi-sysroot -isystem ${wasiSdk}/lib/clang/19/include"; - BINDGEN_EXTRA_CLANG_ARGS_wasm32_wasip1 = "-fvisibility=default --sysroot=${wasiSdk}/share/wasi-sysroot -isystem ${wasiSdk}/lib/clang/19/include -resource-dir ${wasiSdk}/lib/clang/19"; + CFLAGS_wasm32_wasip1 = "--sysroot=${wasiSdk}/share/wasi-sysroot -isystem ${wasiSdk}/lib/clang/${clangMajor}/include"; + BINDGEN_EXTRA_CLANG_ARGS_wasm32_wasip1 = "-fvisibility=default --sysroot=${wasiSdk}/share/wasi-sysroot -isystem ${wasiSdk}/lib/clang/${clangMajor}/include -resource-dir ${wasiSdk}/lib/clang/${clangMajor}";Also applies to: 95-104, 149-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 70 - 71, The WASI SDK shim is hard-coding LLVM 19 (e.g., using lib/clang/19 and mixing rustPackages.rustc.llvmPackages with pkgs.llvmPackages); update the shim to consistently source LLVM from the selected llvmPackages instead of a literal "19": use rustPackages.rustc.llvmPackages (or pkgs.llvmPackages chosen earlier) for lld/clang/libclang, derive the clang include path dynamically from that llvmPackages (instead of lib/clang/19), and adjust clang/ld flags to reference the derived paths so they track nixpkgs bumps and avoid mismatches between lld/clang/libclang; apply the same change to the other similar blocks (lines referenced around 95-104 and 149-163).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flake.nix`:
- Line 210: The flake must declare the wasm experimental feature at
flake-evaluation time: add a top-level nixConfig attribute to the flake that
enables "experimental-features = nix-command flakes wasm-builtin" (or at minimum
"wasm-builtin") so callers don't need to set NIX_CONFIG themselves; update the
flake's top-level export (where the flake attributes are defined) to include
nixConfig = { extra-experimental-features = "wasm-builtin"; } (referencing the
flake's top-level attribute block and the fact that self.lib.* uses
builtins.wasm) so evaluation will succeed without requiring per-derivation
NIX_CONFIG.
---
Nitpick comments:
In `@flake.nix`:
- Around line 133-139: The WASI crate list is duplicated between the
nix-wasi-plugins and nix-wasm-plugins logic (one uses an allowlist and the other
a denylist), causing drift; instead create a single shared binding (e.g.,
wasiCrates or wasiAllowedCrates) that enumerates the WASI crate names and
replace the hard-coded allowlist/denylist uses in both modules to reference that
binding so additions only need to be made in one place; update any places that
currently invert the list (denylist logic) to consult the shared binding and
compute the complement there if needed (keeping names like nix-wasi-plugins and
nix-wasm-plugins to locate where to change).
- Around line 70-71: The WASI SDK shim is hard-coding LLVM 19 (e.g., using
lib/clang/19 and mixing rustPackages.rustc.llvmPackages with pkgs.llvmPackages);
update the shim to consistently source LLVM from the selected llvmPackages
instead of a literal "19": use rustPackages.rustc.llvmPackages (or
pkgs.llvmPackages chosen earlier) for lld/clang/libclang, derive the clang
include path dynamically from that llvmPackages (instead of lib/clang/19), and
adjust clang/ld flags to reference the derived paths so they track nixpkgs bumps
and avoid mismatches between lld/clang/libclang; apply the same change to the
other similar blocks (lines referenced around 95-104 and 149-163).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cb25c35-b681-49d3-ac7a-e52426cd6731
⛔ Files ignored due to path filters (4)
flake.lockis excluded by!**/*.lockwasm/nix_wasm_plugin_ini.wasmis excluded by!**/*.wasmwasm/nix_wasm_plugin_quickjs.wasmis excluded by!**/*.wasmwasm/nix_wasm_plugin_yaml.wasmis excluded by!**/*.wasm
📒 Files selected for processing (1)
flake.nix
| inputs.nix.packages.${system}.nix-cli | ||
| ]; | ||
|
|
||
| NIX_CONFIG = "extra-experimental-features = wasm-builtin"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the file structure and locate flake.nix
git ls-files | grep -E "^flake\.nix$"Repository: DeterminateSystems/nix-wasm-rust
Length of output: 85
🏁 Script executed:
# Get the flake.nix file, focusing on the area around line 210
wc -l flake.nixRepository: DeterminateSystems/nix-wasm-rust
Length of output: 89
🏁 Script executed:
# Read the flake.nix file to understand the context
cat -n flake.nix | head -250 | tail -100Repository: DeterminateSystems/nix-wasm-rust
Length of output: 4394
🏁 Script executed:
# Let's also see the beginning of the file to understand the structure
cat -n flake.nix | head -50Repository: DeterminateSystems/nix-wasm-rust
Length of output: 1863
🏁 Script executed:
# Check for nixConfig in the flake
rg "nixConfig" flake.nixRepository: DeterminateSystems/nix-wasm-rust
Length of output: 58
🏁 Script executed:
# See the complete lib output definition
cat -n flake.nix | sed -n '29,80p'Repository: DeterminateSystems/nix-wasm-rust
Length of output: 2214
🏁 Script executed:
# Check if there's any documentation files
git ls-files | grep -iE "(README|DOCS?|.*\.md)" | head -10Repository: DeterminateSystems/nix-wasm-rust
Length of output: 85
🏁 Script executed:
# Read the README to see if there's documentation about the experimental feature requirement
cat README.mdRepository: DeterminateSystems/nix-wasm-rust
Length of output: 1117
Add top-level nixConfig to surface the experimental feature requirement.
self.lib.* uses builtins.wasm, which requires the wasm-builtin experimental feature to be enabled at flake-evaluation time. The NIX_CONFIG at line 210 only applies to the nix-wasm-plugins derivation build context and doesn't affect flake evaluation, leaving callers to discover this requirement through trial and error. Add a top-level nixConfig in the flake to explicitly declare the requirement:
Suggested addition
{
description = "Nix WebAssembly plugin example";
+
+ nixConfig = {
+ extra-experimental-features = [ "wasm-builtin" ];
+ };
inputs = {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| NIX_CONFIG = "extra-experimental-features = wasm-builtin"; | |
| { | |
| description = "Nix WebAssembly plugin example"; | |
| nixConfig = { | |
| extra-experimental-features = [ "wasm-builtin" ]; | |
| }; | |
| inputs = { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` at line 210, The flake must declare the wasm experimental feature
at flake-evaluation time: add a top-level nixConfig attribute to the flake that
enables "experimental-features = nix-command flakes wasm-builtin" (or at minimum
"wasm-builtin") so callers don't need to set NIX_CONFIG themselves; update the
flake's top-level export (where the flake attributes are defined) to include
nixConfig = { extra-experimental-features = "wasm-builtin"; } (referencing the
flake's top-level attribute block and the fact that self.lib.* uses
builtins.wasm) so evaluation will succeed without requiring per-derivation
NIX_CONFIG.
1 participant
Summary by CodeRabbit
Chores
Style