Skip to content
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

wasmtime-wasi: Split a new IoView trait off of WasiView #10016

Merged
merged 7 commits into from
Jan 16, 2025

Conversation

pchickey
Copy link
Contributor

@pchickey pchickey commented Jan 14, 2025

This PR prepares the wasi-io aspects of the wasmtime-wasi crate to be split off into its own independent crate, in order for the common base of wasi-io functionality to be reusable in a variety of host implementations which do not intend to reuse all of wasmtime-wasi.

Summary for users

You're an embedder using wasmtime-wasi, and maybe even wasmtime-wasi-http. You defined a struct like:

use wasmtime::component::ResourceTable;
use wasmtime_wasi::WasiCtx;
use wasmtime_wasi_http::WasiHttpCtx;
struct MyCtx {
    table: ResourceTable,
    wasi: WasiCtx,
    http: WasiHttpCtx,
}

Prior to this PR, you'd hook your MyCtx up to the WasiView and WasiHttpView traits like:

impl WasiView for MyCtx {
    fn table(&mut self) -> &mut ResourceTable { &mut self.table }
    fn ctx(&mut self) -> &mut WasiCtx { &mut self.wasi }
}
impl WasiHttpView for MyCtx {
    fn table(&mut self) -> &mut ResourceTable { &mut self.table }
    fn ctx(&mut self) -> &mut WasiHttpCtx { &mut self.http }
}

After this PR, you rewrite that to:

impl IoView for MyCtx {
    fn table(&mut self) -> &mut ResourceTable { &mut self.table }
}
impl WasiView for MyCtx {
    fn ctx(&mut self) -> &mut WasiCtx { &mut self.wasi }
}
impl WasiHttpView for MyCtx {
    fn ctx(&mut self) -> &mut WasiHttpCtx { &mut self.http }
}

Thats it. For almost all users, you don't have to worry about anything else. All of the other aspects of wasmtime_wasi{,_http}::add_to_linker_{sync,async} keep working exactly like before. Semantically, nothing changed. The WasiView and WasiHttpView traits grew a common implies on the IoView trait, i.e. trait WasiView: IoView {...}, trait WasiHttpView: IoView {...}.

More details

You probably don't care about these, unless you are doing low level linker stuff and probably can just read the source code to figure it out.

There's a new IoImpl struct that the wasi-io Host trait impls, instead of WasiImpl. Like WasiImpl this struct is #[repr(transparent)]. Anytime you were constructing a WasiImpl(t), you now construct WasiImpl(IoImpl(t)), e.g. https://github.com/bytecodealliance/wasmtime/pull/10016/files#diff-a6ec073ba5d074b65f0bcbf8df48af2b126d65034398905bf74d8f10a5c5f8b6R288

If you're using the wasmtime-wit-bindgen generated add_to_linker_ funcs directly, you'll need a variant on the type_annotate hack to get rustc to infer IoView in the right places, e.g. https://github.com/bytecodealliance/wasmtime/pull/10016/files#diff-a6ec073ba5d074b65f0bcbf8df48af2b126d65034398905bf74d8f10a5c5f8b6R287.

@github-actions github-actions bot added the wasi Issues pertaining to WASI label Jan 14, 2025
@pchickey pchickey force-pushed the pch/wasi_view_traits branch from 413dd23 to afb6228 Compare January 15, 2025 22:06
@pchickey pchickey marked this pull request as ready for review January 16, 2025 20:53
@pchickey pchickey requested a review from a team as a code owner January 16, 2025 20:53
@pchickey pchickey requested review from alexcrichton and removed request for a team January 16, 2025 20:53
T::ctx(&mut self.0)
}
}

/// Per-[`Store`] state which holds state necessary to implement WASI from this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These docs are now appended to the ones above, so should these words move above the example perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made lots of fixes to the docs during the code motion for #10036 - can we put off the docs review and check it all over there?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable to me 👍

fn table(&mut self) -> &mut ResourceTable;
}

pub trait WasiView: IoView {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this retain some of the documentation from before as well? Ideally with an example or links to other examples throughout the crate.

use crate::ctx::WasiCtx;
use wasmtime::component::ResourceTable;

pub trait IoView: Send {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To perhaps bikeshed a bit, maybe TableView?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also mind ensuring there's documentation for this trait as well? (similar to the WasiView one below)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I named it Io instead of Table view in anticipation of factoring it out for the wasmtime-wasi-io crate. I don't mind renaming it, but thats why I was thinking Io over Table.

@alexcrichton alexcrichton added this pull request to the merge queue Jan 16, 2025
Merged via the queue into main with commit e7f43b8 Jan 16, 2025
41 checks passed
@alexcrichton alexcrichton deleted the pch/wasi_view_traits branch January 16, 2025 22:27
pchickey added a commit that referenced this pull request Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants