Stream: git-wasmtime

Topic: wasmtime / PR #8228 Add documentation and refactor `wasmt...

view this post on Zulip Wasmtime GitHub notifications bot (Mar 24 2024 at 16:13):

alexcrichton opened PR #8228 from alexcrichton:doc-wasi to bytecodealliance:main:

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 https://github.com/bytecodealliance/wasmtime/issues/8187 (they're now at the same level)
Closes https://github.com/bytecodealliance/wasmtime/issues/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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 24 2024 at 16:13):

alexcrichton requested fitzgen for a review on PR #8228.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 24 2024 at 16:13):

alexcrichton requested wasmtime-core-reviewers for a review on PR #8228.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 25 2024 at 15:28):

rylev submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 25 2024 at 15:28):

rylev submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 25 2024 at 15:28):

rylev created PR review comment:

Not related to this PR exactly, but one pattern that can be very useful when a builder is required to build a type is to a builder function to that type. So in this case:

WasiCtx::builder();

view this post on Zulip Wasmtime GitHub notifications bot (Mar 25 2024 at 15:28):

rylev created PR review comment:

This always bothered me that this is called perms instead of dir_perms to contrast with file_perms. What do we think about renaming that arg?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 25 2024 at 15:28):

rylev created PR review comment:

Should guest_path be second so that file paths are first and permissions second?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 25 2024 at 16:54):

alexcrichton updated PR #8228.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 25 2024 at 16:55):

alexcrichton commented on PR #8228:

All good suggestions :+1:

view this post on Zulip Wasmtime GitHub notifications bot (Mar 25 2024 at 20:20):

fitzgen requested sunfishcode for a review on PR #8228.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 25 2024 at 20:21):

fitzgen commented on PR #8228:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 25 2024 at 21:19):

alexcrichton commented on PR #8228:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 25 2024 at 21:19):

alexcrichton requested elliottt for a review on PR #8228.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 11:55):

rylev submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 11:55):

rylev created PR review comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 11:55):

rylev submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 11:55):

rylev created PR review comment:

    /// asynchronous version see [`bindings::Command`](super::Command).

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 11:55):

rylev created PR review comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 11:56):

sunfishcode submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 11:56):

sunfishcode submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 11:56):

sunfishcode created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 14:21):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 14:21):

alexcrichton created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 14:21):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 14:21):

alexcrichton created PR review comment:

This is the old builder from wasi-common

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 14:22):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 14:22):

alexcrichton created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 14:23):

alexcrichton updated PR #8228.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 23:57):

elliottt submitted PR review:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 23:57):

elliottt submitted PR review:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 23:57):

elliottt created PR review comment:

Why don't we consume self here?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 23:57):

elliottt created PR review comment:

    /// Directories can be limited to being readonly. This will restrict what
    /// can be done with them, for example preventing creation of new files.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 23:57):

elliottt created PR review comment:

                // These interfaces come from the outer module, as it's

view this post on Zulip Wasmtime GitHub notifications bot (Mar 27 2024 at 14:29):

alexcrichton updated PR #8228.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 27 2024 at 14:30):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 27 2024 at 14:30):

alexcrichton created PR review comment:

You can find more discussion of that here

view this post on Zulip Wasmtime GitHub notifications bot (Mar 28 2024 at 01:14):

alexcrichton requested elliottt for a review on PR #8228.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 28 2024 at 02:25):

elliottt submitted PR review:

:shipit:

view this post on Zulip Wasmtime GitHub notifications bot (Mar 28 2024 at 03:31):

alexcrichton merged PR #8228.

Last updated: Nov 22 2024 at 16:03 UTC