-
Notifications
You must be signed in to change notification settings - Fork 22
Add WASI p2 support #488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add WASI p2 support #488
Conversation
spec/unit/wasi_spec.rb
Outdated
it "prevents panic when Store has the wrong WASI config" do | ||
linker = Linker.new(@engine, wasi: true) | ||
store = Store.new(@engine, wasi_config: WasiConfig.new.use_p2.set_stdin_string("some str")) | ||
expect { linker.instantiate(store, wasi_module).invoke("_start") } | ||
.to raise_error(Wasmtime::Error, /Store is missing WASI configuration/) | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I'm fully convinced about the use_p2
approach, the main reason being the scenario described by this unit test.
As I was glossing over your change, I kept wondering if a viable alternative is to consider moving away from the status quo of:
- Calling
wasmtime_wasi::p<N>::add_to_linker_sync
when callingLinker::new
- Passing
wasi: <true|false>
onLinker::new
For 1, in order to make the usage of the WasiConfig
less error prone (reducing the possibility of erroneous configuration), would it be possible to have dedicate methods / functions that operate on top of a Wasmtime::Linker
and Wasmtime::Component::Linker
and do a runtime check then to ensure that you're passing the right Linker
for the preview that you're trying to use? We still need a runtime check, however, this approach (IMHO) reduces the bloat in the WasiConfig
For 2, couldn't we get rid entirely of the WASI checks performed in instantiate
? If you try to instantiate a module/component and not all imports are satisfied, Wasmtime should report that as an error that we should handle, so I'm not sure if it's worth duplicating that check. I'm aware that such logic was pre-existing, however, with the addition of p2, it's apparent the duplication of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For 1, I'm open to ideas about a better API since I agree what I'm proposing is prone to misconfiguration and relies on a bunch of runtime checks.
Are you suggesting WasiConfig
would have an add_to_linker(linker) -> ()
and an add_to_component_linker(linker) -> ()
method? Or are you suggesting each linker would have a method that would accept a WasiConfig
which would then add the appropriate preview after checking whether the preview is set up correctly?
One drawback that comes to mind with either is a user could call .use_p2
after making the call against the linker and before passing the WasiConfig
to the store.
For example,
linker = Linker.new(engine)
wasi_config = WasiConfig.new
.set_stdin_string("hi!")
.inherit_stdout
.inherit_stderr
.set_argv(ARGV)
.set_env(ENV)
wasi_config.add_to_linker(linker) # runtime checks pass for now since it's a P1 config and a module linker
wasi_config.use_p2
store = Store.new(engine, wasi_config: wasi_config) # store is configured for P2
However, we could get around that by requiring the use of a static constructor method for WasiConfig
to use preview 2. Something like WasiConfig.new_p2
and remove the setter. However, I can imagine a future world where someone might want to use multiple WASI versions that work with components so a static constructor method tied to a particular version might not be the best option.
My instinct would be to use the WasiConfig
to create the linker which would have a method to create the store (or potentially the store creating the linker).
Something along the lines of,
wasi_config = WasiConfig.new
linker = Linker.new(engine, wasi: wasi_config)
store = linker.new_store
But this also diverges from Wasmtime's API.
Anyway, happy to work through options and discuss offline.
For 2, couldn't we get rid entirely of the WASI checks performed in instantiate? If you try to instantiate a module/component and not all imports are satisfied, Wasmtime should report that as an error that we should handle, so I'm not sure if it's worth duplicating that check. I'm aware that such logic was pre-existing, however, with the addition of p2, it's apparent the duplication of it.
When I comment out the runtime check in the module linker's instantiate method, I see panics when running the specs. It's not about missing imports but that wasmtime_wasi
's add_to_linker_sync
functions require a WASI context so we have .expect()
s that get triggered if the WASI context isn't set and we don't have access to the store when creating the linker which call add_to_linker_sync
.
Looks like the clippy issues were not introduced in this PR. I've opened #489 to fix them. |
Adds support for WASI preview 2.
I do have a couple things I'm looking for feedback on:
.use_p2
onWasiConfig
. I could add a static constructor toWasiConfig
for preview 2 instead.