Merge builtins.wasm and builtins.wasi into one function#370
Open
Merge builtins.wasm and builtins.wasi into one function#370
Conversation
The new interface is
builtins.wasm {
path = ./path/to/file.wasm;
wasi = false;
function = "fib";
} 33
📝 WalkthroughWalkthroughConsolidates Wasm handling into a single Changes
Sequence Diagram(s)sequenceDiagram
participant Evaluator as Nix Evaluator
participant Parser as Config Parser
participant Instantiator as Wasm Instantiator
participant WasiRuntime as WASI Runtime
participant NonWasiRuntime as Non‑WASI Runtime
participant Module as Wasm Module
Evaluator->>Parser: __wasm(config, arg)
Parser->>Instantiator: validate config (path, function?)
Instantiator->>Module: load & compile module
Instantiator->>Instantiator: inspect imports -> detect `_start` => useWasi?
alt useWasi == true
Instantiator->>WasiRuntime: instantiate with WASI, wire return_to_nix
WasiRuntime->>Module: call _start(argv[1])
Module-->>WasiRuntime: invoke return_to_nix(result)
WasiRuntime-->>Evaluator: deliver result
else useWasi == false
Instantiator->>NonWasiRuntime: instantiate without WASI
NonWasiRuntime->>Module: call nix_wasm_init_v1()
NonWasiRuntime->>Module: call configured function(valueId)
Module-->>NonWasiRuntime: return i32 valueId
NonWasiRuntime-->>Evaluator: deliver result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Member
|
Maybe we should make the toggle not a boolean but an enum, like |
![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)
doc/manual/source/protocols/wasm.md (1)
38-41: Clarifynix_wasm_init_v1()call frequency wording.“called once when the module is instantiated” can read as global/per-module caching behavior. Consider clarifying it is once per instance (i.e., per
builtins.wasminvocation).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/manual/source/protocols/wasm.md` around lines 38 - 41, Clarify the wording for nix_wasm_init_v1() to state it is invoked once per module instance, not globally; update the sentence mentioning `nix_wasm_init_v1()` to read that the function is called once for each instantiation of the module (i.e., once per `builtins.wasm` invocation / per instance), so readers understand it’s per-instance initialization rather than a single global call across the process.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libexpr/primops/wasm.cc`:
- Around line 614-635: The WASI configuration is being applied after
instantiateWasm(), but Wasmtime requires setting the WASI context on the store
before module instantiation; move the WasiConfig creation and calls to
wasi_config_set_stdout_custom/wasi_config_set_stderr_custom,
WasiConfig::argv(...) and the call to instance.wasmStore.context().set_wasi(...)
to occur before calling instantiateWasm(), then call instantiateWasm(state,
wasmPath, useWasi) (and only afterward use instance.addValue and
instance.getExport<Func>().call(...)); ensure the WasiLogger, its trampoline,
and argv are created and passed into the store via set_wasi prior to
instantiation so the module start can safely use WASI.
---
Nitpick comments:
In `@doc/manual/source/protocols/wasm.md`:
- Around line 38-41: Clarify the wording for nix_wasm_init_v1() to state it is
invoked once per module instance, not globally; update the sentence mentioning
`nix_wasm_init_v1()` to read that the function is called once for each
instantiation of the module (i.e., once per `builtins.wasm` invocation / per
instance), so readers understand it’s per-instance initialization rather than a
single global call across the process.
Collaborator
Author
|
@grahamc It now auto-detects whether to use WASI. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/libexpr/primops/wasm.cc (1)
607-642:⚠️ Potential issue | 🔴 CriticalConfigure WASI before instance instantiation.
Line 607 instantiates the module before Line 641 calls
set_wasi. If the module executes start-time code during instantiation, this ordering can fail before WASI is available. This appears to be the same unresolved issue raised earlier.In Wasmtime C++/C APIs, should `Store::Context::set_wasi(...)` be called before `instantiate` when start-time code may run?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/primops/wasm.cc` around lines 607 - 642, The code currently calls instantiateWasm before configuring WASI which can run module start-time code without WASI available; move the WASI setup so that when instance.pre->useWasi is true you create a WasiConfig, set the custom stdout/stderr trampolines and argv, call instance.wasmStore.context().set_wasi(std::move(wasiConfig)) before calling instantiateWasm, and then call instantiateWasm (and remove the later set_wasi call); look for instantiateWasm, instance.pre->useWasi, WasiConfig, wasi_config_set_stdout_custom/wasi_config_set_stderr_custom, and instance.wasmStore.context().set_wasi to make these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/manual/source/protocols/wasm.md`:
- Line 355: Update the stale wording that says "wasi = true" to instead
reference WASI mode detection: change the sentence "This function is only
available when `wasi = true`" to something like "This function is only available
in WASI mode (module exports `_start`)" so it matches the document's
auto-detection behavior and references the `_start` export.
In `@src/libexpr/primops/wasm.cc`:
- Around line 98-103: The current heuristic that marks useWasi = true by
scanning module.exports() for the "_start" export is incorrect; instead detect
WASI by checking module.imports() for a WASI import module (e.g.,
"wasi_snapshot_preview1", "wasi_unstable", or imports whose module name starts
with "wasi_") and set useWasi based on that. Replace the export-based loop that
inspects module.exports() and the "_start" name with a loop over
module.imports() (or equivalent API) that looks at import.module() (or
import.module_name()) for known WASI module names and sets useWasi = true if
found, leaving all other logic (including the code that validates the "function"
attribute) untouched. Ensure you reference the same variables (useWasi, module)
and remove reliance on "_start" so WASI reactors and command modules are both
detected correctly.
---
Duplicate comments:
In `@src/libexpr/primops/wasm.cc`:
- Around line 607-642: The code currently calls instantiateWasm before
configuring WASI which can run module start-time code without WASI available;
move the WASI setup so that when instance.pre->useWasi is true you create a
WasiConfig, set the custom stdout/stderr trampolines and argv, call
instance.wasmStore.context().set_wasi(std::move(wasiConfig)) before calling
instantiateWasm, and then call instantiateWasm (and remove the later set_wasi
call); look for instantiateWasm, instance.pre->useWasi, WasiConfig,
wasi_config_set_stdout_custom/wasi_config_set_stderr_custom, and
instance.wasmStore.context().set_wasi to make these changes.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/libexpr/primops/wasm.cc (1)
135-144:⚠️ Potential issue | 🟠 MajorWASI context is still configured after module instantiation.
The
NixWasmInstanceconstructor instantiates the module at line 140 viapre->instancePre.instantiate(wasmCtx), butset_wasi()is called much later at line 641 inprim_wasm. While usinginstantiate_prepre-compiles the module, the actual instantiation still happens in the constructor before WASI is configured. If the Wasm module has a start section (distinct from the_startexport), it will execute during instantiation and may call WASI functions before the context is set, causing an abort.Consider restructuring so that
set_wasi()is called on the store context beforeNixWasmInstanceis constructed, or defer instantiation until after WASI configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/primops/wasm.cc` around lines 135 - 144, The constructor NixWasmInstance currently calls pre->instancePre.instantiate(wasmCtx) (in NixWasmInstance(...)) before WASI is configured via set_wasi (done later in prim_wasm), which allows a module start function to run without a proper WASI context; fix by moving instantiation to after WASI is set: either (A) call set_wasi on the store/context before constructing NixWasmInstance so the wasmCtx passed into NixWasmInstance already has WASI, or (B) change NixWasmInstance to defer instantiate (remove/replace instance = unwrap(pre->instancePre.instantiate(wasmCtx)) from the constructor and add an explicit instantiate() method invoked from prim_wasm after set_wasi completes); update all call sites in prim_wasm to construct NixWasmInstance first only when using approach A or to call the new instantiate() method after set_wasi when using approach B, ensuring wasmCtx.set_data(this) is retained on the correct store/context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/manual/source/protocols/wasm.md`:
- Around line 43-52: The "WASI Mode" section currently says WASI is detected by
the module exporting `_start`, which contradicts the overview and non-WASI
section; update the "WASI Mode" text to state that WASI is detected by the
presence of `wasi_snapshot_preview1` imports, and optionally note that an
exported `_start` is just the conventional entrypoint used by WASI runtimes (but
not the detection mechanism). Adjust any example or wording in the "WASI Mode"
heading to reference `wasi_snapshot_preview1` as the detection mechanism and
keep `_start` described only as the entry point.
In `@src/libexpr/primops/wasm.cc`:
- Around line 671-701: The docstring for the "__wasm" primop (registered via
primop_wasm with implementation prim_wasm) inaccurately says WASI mode is
detected by checking for an exported "_start"; update the documentation to match
the implementation: state that WASI mode is detected by the presence of wasi
imports (e.g. "wasi_snapshot_preview1") rather than an exported "_start", and
clarify the rule about when the `function` attribute is required/forbidden
accordingly so the text aligns with how prim_wasm actually detects WASI.
- Around line 612-622: The error text thrown when instance.pre->useWasi is true
is misleading because WASI is detected via imports (wasi_snapshot_preview1) not
the presence of a '_start' export; update the Exception message thrown in the
builtins.wasm handling (the throw inside the if (instance.pre->useWasi) path
where functionAttr is provided) to reference WASI imports (e.g., "WASI modules
(modules that import 'wasi_snapshot_preview1') do not accept a 'function'
attribute") or similar wording that mentions imports rather than a '_start'
export; keep the same throw location and type (throw Error(...)) and ensure any
other related messages around functionAttr handling in this block (and the
non-WASI branch) remain consistent.
---
Duplicate comments:
In `@src/libexpr/primops/wasm.cc`:
- Around line 135-144: The constructor NixWasmInstance currently calls
pre->instancePre.instantiate(wasmCtx) (in NixWasmInstance(...)) before WASI is
configured via set_wasi (done later in prim_wasm), which allows a module start
function to run without a proper WASI context; fix by moving instantiation to
after WASI is set: either (A) call set_wasi on the store/context before
constructing NixWasmInstance so the wasmCtx passed into NixWasmInstance already
has WASI, or (B) change NixWasmInstance to defer instantiate (remove/replace
instance = unwrap(pre->instancePre.instantiate(wasmCtx)) from the constructor
and add an explicit instantiate() method invoked from prim_wasm after set_wasi
completes); update all call sites in prim_wasm to construct NixWasmInstance
first only when using approach A or to call the new instantiate() method after
set_wasi when using approach B, ensuring wasmCtx.set_data(this) is retained on
the correct store/context.
Comment on lines
+43
to
+52
| ### WASI Mode | ||
|
|
||
| Usage: `builtins.wasi <module> <arg>` | ||
| WASI mode is automatically used when the module exports a `_start` function. | ||
|
|
||
| Usage: | ||
| ```nix | ||
| builtins.wasm { | ||
| path = <module>; | ||
| } <arg> | ||
| ``` |
There was a problem hiding this comment.
WASI mode section contradicts the overview regarding detection mechanism.
Line 45 states "WASI mode is automatically used when the module exports a _start function" but the overview (line 15) and non-WASI section (line 28) correctly state that WASI is detected by checking for wasi_snapshot_preview1 imports. This inconsistency could confuse users.
✏️ Suggested fix
-WASI mode is automatically used when the module exports a `_start` function.
+WASI mode is automatically used when the module imports from `wasi_snapshot_preview1`.📝 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.
Suggested change
| ### WASI Mode | |
| Usage: `builtins.wasi <module> <arg>` | |
| WASI mode is automatically used when the module exports a `_start` function. | |
| Usage: | |
| ```nix | |
| builtins.wasm { | |
| path = <module>; | |
| } <arg> | |
| ``` | |
| ### WASI Mode | |
| WASI mode is automatically used when the module imports from `wasi_snapshot_preview1`. | |
| Usage: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/manual/source/protocols/wasm.md` around lines 43 - 52, The "WASI Mode"
section currently says WASI is detected by the module exporting `_start`, which
contradicts the overview and non-WASI section; update the "WASI Mode" text to
state that WASI is detected by the presence of `wasi_snapshot_preview1` imports,
and optionally note that an exported `_start` is just the conventional
entrypoint used by WASI runtimes (but not the detection mechanism). Adjust any
example or wording in the "WASI Mode" heading to reference
`wasi_snapshot_preview1` as the detection mechanism and keep `_start` described
only as the entry point.
Comment on lines
+612
to
+622
| if (instance.pre->useWasi) { | ||
| functionName = "_start"; | ||
| if (functionAttr) | ||
| throw Error("'function' attribute is not allowed for WASI modules (modules with a '_start' export)"); | ||
| } else { | ||
| if (!functionAttr) | ||
| throw Error( | ||
| "missing required 'function' attribute in first argument to `builtins.wasm` for non-WASI modules"); | ||
| functionName = std::string( | ||
| state.forceStringNoCtx(*functionAttr->value, pos, "while evaluating the 'function' attribute")); | ||
| } |
There was a problem hiding this comment.
Error message references _start export but WASI is detected via imports.
The error message at line 615 states "modules with a '_start' export" but WASI detection actually checks for wasi_snapshot_preview1 imports (lines 98-103). A WASI reactor module (which imports WASI but doesn't export _start) would trigger useWasi = true but this error message would be misleading.
✏️ Suggested fix
if (functionAttr)
- throw Error("'function' attribute is not allowed for WASI modules (modules with a '_start' export)");
+ throw Error("'function' attribute is not allowed for WASI modules (modules that import from wasi_snapshot_preview1)");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libexpr/primops/wasm.cc` around lines 612 - 622, The error text thrown
when instance.pre->useWasi is true is misleading because WASI is detected via
imports (wasi_snapshot_preview1) not the presence of a '_start' export; update
the Exception message thrown in the builtins.wasm handling (the throw inside the
if (instance.pre->useWasi) path where functionAttr is provided) to reference
WASI imports (e.g., "WASI modules (modules that import 'wasi_snapshot_preview1')
do not accept a 'function' attribute") or similar wording that mentions imports
rather than a '_start' export; keep the same throw location and type (throw
Error(...)) and ensure any other related messages around functionAttr handling
in this block (and the non-WASI branch) remain consistent.
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
2 participants
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.
Motivation
The new interface is
This will allow other Wasm-related configuration to be added in the future (e.g. WASI features, memory limits, caching info, etc).
The argument is separate from the configuration because typically you want to write something like
It now also auto-detects whether a module uses WASI.
Context
Summary by CodeRabbit
Documentation
Refactor