github-actions[bot] commented on Issue #1565:
Subscribe to Label Action
cc @kubkon, @peterhuene
<details>
This issue or pull request has been labeled: "wasi", "wasmtime", "wasmtime:api", "wasmtime:docs"Thus the following users have been cc'd because of the following labels:
- kubkon: wasi
- peterhuene: wasmtime:api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
github-actions[bot] commented on Issue #1565:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "wasmtime:c-api"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:c-api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
alexcrichton commented on Issue #1565:
I do like this approach better myself, but I feel like it's still a bit too heavyweight because it makes
NewInstance
almost exclusively WASI-focused. I would expectInstance
to be neutral with respect to WASI-or-not and users have to opt-in to WASI-specific hooks.I realize it can be a small footgun if these methods are added directly to
Instance
, but I feel that the footgun is worth it in terms of ergonomics. For example if you're reading the documentation of thewasmtime
crate all the examples haveinit_reactor(&[])
which is somewhat bizarre if you've never heard of WASI before.
sunfishcode commented on Issue #1565:
They aren't really WASI-specific. I expect that between the linking spec and interface types, the definition of commands and reactors will eventually want to move out of WASI and into one of those other specs. Would it help if we moved the definition into tool-conventions for now? We could then avoid calling these "WASI" functions. Would that help avoid confusion?
I'm also not tied to the name
init_reactor
. Would.initialize(&[])
or something else look less surprising?
github-actions[bot] commented on Issue #1565:
Subscribe to Label Action
cc @fitzgen
<details>
This issue or pull request has been labeled: "fuzzing"Thus the following users have been cc'd because of the following labels:
- fitzgen: fuzzing
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
alexcrichton commented on Issue #1565:
Oh sorry, so I mean to say that the current as-is state of
NewInstance
feels very WASI-specific. The options you have are:
start
- this is actually somewhat unrelated to the wasmstart
function and classifies every "normal" wasm module as a "reactor"run_command
,init_reactor
- WASI-specificThere's not really an easy option for "just give me the
Instance
I don't care about WASI". Even withinit_reactor
you have to pass an array of arguments which can be confusing.This is why I still think it's best to return an work with an
Instance
. The type-level distinction of aNewInstance
I don't think is buying us much here. If we really want we could always probe what anInstance
looks like and dynamically guard invocations of functions. We could say, for example, that you can't call WASI functions before you call the wasi initialization function which does the command/reactor thing.
sunfishcode commented on Issue #1565:
Ok, I've now gone back to
Instance::new
not doing any command/reactor stuff. I keptNewInstance
, with astart
function that now just runs the wasm start function. That also simplifies the C API story, so this patch now has C API updates corresponding to the Rust API updates.The new approach now adds a new function
Linker::module
, which handles instantiating the module, automatically handling command/reactor semantics, which ends up being a lot nicer because we don't need APIs that conditionally consume anInstance
.Linker::module
avoids the user having to deal withInstance
s altogether.
alexcrichton commented on Issue #1565:
Ok this all looks great to me, thanks again for this @sunfishcode! I feel like anything further we can continue to iterate on in-tree if necessary, so I'm gonna merge.
Last updated: Dec 23 2024 at 12:05 UTC