Add documentation and refactor `wasmtime-wasi` by alexcrichton · Pull Request #8228 · bytecodealliance/wasmtime

alexcrichton · 2024-03-24T16:13:03Z

Currently the wasmtime-wasi crate is pretty sparse on documentation and additionally had some papercuts in the API. I've attempted to document all the major bits with words and examples here in addition to performing some quality-of-life refactors for API usage in the future. I'd like to refactor the WASIp2 bits a bit more to make them a bit more similar to the WASIp1 bits too, but that's a larger refactoring for a future PR.

Closes #8187 (they're now at the same level)
Closes #8188 (I've synchronized interfaces added)

I'll note that wasmtime-wasi-http could still use some love as well, which I haven't touched in this PR.

This commit adds lots of missing documentation and examples to top-level types in `wasmtime-wasi`, mostly related to WASIp2. I've additionally made a number of small refactorings here to try to make the APIs a bit more straightforward and symmetric and simplify where I can. * Remove `bindings::wasi` (reexports are still present under `bindings`) * Rename `bindings::sync_io` to `bindings::sync` * Generate fewer bindings in `bindings::sync` that can be pulled in from the `bindings` module. * Change `WasiCtxBuilder::preopened_dir` to take a path instead of a `Dir` argument to avoid the need for another import. * Synchronize `wasmtime_wasi_http::{add_to_linker, sync::add_to_linker}` in terms of interfaces added. * Remove `wasmtime_wasi::command` and move the generated types to the `bindings` module. * Move top-level add-to-linker functions to `wasmtime_wasi::add_to_linker_sync` and `wasmtime_wasi::add_to_linker_async`. Closes bytecodealliance#8187 Closes bytecodealliance#8188

This commit adds documentation for the `wasmtime_wasi::preview1` module and additionally refactors it as well. Previously this was based on a similar design as WASIp2 with a "view trait" and various bits and pieces, but the design constraints of WASIp1 lends itself to a simpler solution of "just" passing around `WasiP1Ctx` instead. This goes back to what `wasi-common` did of sorts where the `add_to_linker_*` functions only need a projection from `&mut T` to `&mut WasiP1Ctx`, a concrete type, which simplifies the module and usage.

crates/wasi/src/ctx.rs

alexcrichton · 2024-03-25T16:54:59Z

All good suggestions 👍

fitzgen · 2024-03-25T20:21:10Z

Redirecting review to @sunfishcode since I haven't ever really touched this crate.

alexcrichton · 2024-03-25T21:19:38Z

I'll also tag @elliottt since he's worked on these crates as well

crates/wasi/src/bindings.rs

rylev · 2024-03-26T11:35:58Z

crates/wasi/src/bindings.rs

 // have some functions in `only_imports` below for being async.
-pub mod sync_io {
-    pub(crate) mod _internal {
+pub mod sync {


I'm unsure about sync vs. async_io. It seems like the previous sync_io contrasts more clearly with async_io

Ah the async_io name doesn't show up at all, that's just to contain everything and then reexport only the bits we need. The only modules here are bindings and bindings::sync, but these bindings are relatively rare to access since they're the raw traits/types that most embedders won't be working with.

rylev · 2024-03-26T11:54:32Z

src/commands/run.rs

+            let dir = Dir::open_ambient_dir(host, ambient_authority())
+                .with_context(|| format!("failed to open directory '{}'", host))?;
+            builder.preopened_dir(dir, guest)?;


I'm confused by this. Why does the preopened_dir only take two arguments?

This is the old builder from wasi-common

sunfishcode · 2024-03-25T21:25:03Z

crates/wasi-http/src/proxy.rs

+        wasmtime_wasi::bindings::cli::stdin::add_to_linker(l, |t| t)?;
+        wasmtime_wasi::bindings::cli::stdout::add_to_linker(l, |t| t)?;
+        wasmtime_wasi::bindings::cli::stderr::add_to_linker(l, |t| t)?;
+        wasmtime_wasi::bindings::random::random::add_to_linker(l, |t| t)?;


This might be out of scope for this PR but: is there a reason we need to add all the interfaces manually here? The world in the Wit level groups all these interfaces together, so it seems like the host-side bindings should be able to group all these together as well.

The main reason is that the auto-generated functions all take a closure that's fn(&mut T) -> &mut U but that's switched here to be T: WasiView. Otherwise though, you're right, and it's something I've wanted to clean up for awhile.

elliottt

This looks great to me! Just a few questions/suggestions.

crates/wasi/src/bindings.rs

elliottt · 2024-03-26T18:05:16Z

crates/wasi/src/ctx.rs

+    /// Panics if this method is called twice. Each [`WasiCtxBuilder`] can be
+    /// used to create only a single [`WasiCtx`] or [`WasiP1Ctx`]. Repeated
+    /// usage of this method is not allowed and should use a second builder
+    /// instead.


Why don't we consume self here?

You can find more discussion of that here

crates/wasi/src/filesystem.rs

elliottt

This PR upgrades the version of `wasmtime` and `wasmtime-wasi` in use to v21.0.1. We have to skip v20 because Tree-sitter also skipped it. Here are the changes that had to be made: ### v19 -> v20 After upgrading the `wasmtime` packages to v20, I also had to run `cargo update -p mach2` to pull in [v0.4.2](https://github.com/JohnTitor/mach2/releases/tag/0.4.2) to fix some compile errors. There were a few minor API changes in `wasmtime-wasi` from bytecodealliance/wasmtime#8228 that we needed to account for. ### v20 -> v21 Since there isn't a Tree-sitter version that depends on `wasmtime@v20`, we're jumping straight to v21. The published version of Tree-sitter (v0.22.6) still depends on `wasmtime@v19`, but there was a commit (tree-sitter/tree-sitter@7f4a578) later that month that upgrades the `wasmtime` dependency to v21. We're patching Tree-sitter to that commit so we can get the new `wasmtime` version. The main change in v21 is that imports generated by `bindgen!` are no longer automatically trapped (bytecodealliance/wasmtime#8310), so we need to add `trappable_imports: true` to our `bindgen!` calls. Release Notes: - N/A

alexcrichton added 2 commits March 24, 2024 09:08

alexcrichton requested a review from a team as a code owner March 24, 2024 16:13

alexcrichton requested review from fitzgen and removed request for a team March 24, 2024 16:13

github-actions bot added the wasi Issues pertaining to WASI label Mar 24, 2024

rylev reviewed Mar 25, 2024

View reviewed changes

crates/wasi/src/ctx.rs Outdated Show resolved Hide resolved

crates/wasi/src/ctx.rs Show resolved Hide resolved

crates/wasi/src/ctx.rs Show resolved Hide resolved

alexcrichton added 2 commits March 25, 2024 09:53

Small refactorings to preopened_dir

5c69d93

Add WasiCtx::builder.

c210213

fitzgen requested review from sunfishcode and removed request for fitzgen March 25, 2024 20:20

alexcrichton requested a review from elliottt March 25, 2024 21:19

rylev reviewed Mar 26, 2024

View reviewed changes

sunfishcode reviewed Mar 26, 2024

View reviewed changes

Fix typo

707ce2a

elliottt reviewed Mar 26, 2024

View reviewed changes

Review comments

28fcef2

alexcrichton requested a review from elliottt March 28, 2024 01:14

elliottt approved these changes Mar 28, 2024

View reviewed changes

alexcrichton added this pull request to the merge queue Mar 28, 2024

Merged via the queue into bytecodealliance:main with commit 20660cb Mar 28, 2024

alexcrichton deleted the doc-wasi branch March 28, 2024 03:31

primoly mentioned this pull request May 6, 2024

Since Wasmtime 20, preopens of non-existent directories fail eagerly rather than on first access #8552

Closed

maxdeviant mentioned this pull request Jul 25, 2024

extension: Upgrade wasmtime to v21 zed-industries/zed#15210

Merged

Conversation

alexcrichton commented Mar 24, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexcrichton commented Mar 25, 2024

Uh oh!

fitzgen commented Mar 25, 2024

Uh oh!

alexcrichton commented Mar 25, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elliottt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elliottt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants