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

Split off wasmtime-wasi-io crate from wasmtime-wasi #10036

Merged
merged 28 commits into from
Jan 22, 2025
Merged

Conversation

pchickey
Copy link
Contributor

@pchickey pchickey commented Jan 16, 2025

Follow-on to #10016

The big motivator of this change is to provide a common definition of wasi-io to be reusable by alternative (to wasmtime-wasi) implementations of the rest of WASI. In particular, this crate is compatible with !#[no_std] Rust.

Additionally, it contains renames for things that have been bugging me for a year or two.

New wasmtime-wasi-io crate

This takes the bindings generation for the wasi-io crate, the definitions of the Subscribe, HostInputStream, and HostOutputStream traits, and the freshly separated IoView machinery, and puts it into its own crate wasmtime-wasi-io.

Users of wasmtime-wasi and others never have to know or care about the change to the crate, the wasmtime-wasi crate re-exports everything now defined inwasmtime-wasi-io prior to the PR. However, names have changed!

Optionally, crates which depend on wasmtime-wasi for just the wasi-io primitives May depend directly on wasmtime-wasi-io directly. I made changes in this PR so that wasmtime-wasi-http changes its imports to be from wasmtime-wasi-io.

wasmtime-wasi-http can't break its dep on wasmtime-wasi for two reasons: it depends on wasmtime_wasi::runtime to provide common code for in_tokio sync wrappers of async and task spawning, and its linker needs to provide the std{in,out,err} interfaces from the wasi-cli implementation in wasmtime-wasi.

Trait and type renaming

Programmers using the direct bindings got to see the wit Pollable, InputStream, and OutputStream names inside Resource<...>. However, mapping from the resources to the traits is a boring bit of boilerplate that most programmers only ever encounter very briefly, and then spend most of their time interacting with the traits. The traits are the inconsistently-named Subscribe, HostInputStream, and HostOutputStream. Host... prefix these days means bindgen-created, while back in wasi-common meant not-bindgen-created. And Subscribe doesn't describe that trait at all!

So, I renamed according to the following:

    resource type Pollable -> DynPollable
    resource type InputStream -> DynInputStream
    resource type OutputStream -> DynOutputStream

    trait Subscribe -> Pollable
    trait HostInputStream -> InputStream
    trait HostOutputStream -> OutputStream

    type alias PollableFuture -> DynFuture (little-used)
    type alias ClosureFuture deleted (never used)

etc

I had to make one minor functional change to ResourceTable::iter_entries and impl poll::Host { fn poll(...) {...} } to use a BTreeMap in place of a HashMap in order to enable building wasmtime-wasi-io without std enabled in the wasmtime crate features.

@pchickey pchickey requested review from a team as code owners January 16, 2025 21:03
@pchickey pchickey requested review from fitzgen and removed request for a team January 16, 2025 21:03
…i-http

wasmtime-wasi-http is part of the public API where we guarantee semver
is obeyed
@pchickey pchickey requested review from alexcrichton and removed request for fitzgen January 16, 2025 23:24
resource type Pollable -> DynPollable
resource type InputStream -> DynInputStream
resource type OutputStream -> DynOutputStream

trait Subscribe -> Pollable
trait HostInputStream -> InputStream
trait HostOutputStream -> OutputStream

type alias PollableFuture -> DynFuture (little-used)
@github-actions github-actions bot added the wasi Issues pertaining to WASI label Jan 17, 2025
@@ -27,6 +27,7 @@ http-body = { workspace = true }
http-body-util = { workspace = true }
tracing = { workspace = true }
wasmtime-wasi = { workspace = true }
wasmtime-wasi-io = { workspace = true }
Copy link
Member

Choose a reason for hiding this comment

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

As a proof-of-concept would it be possible to not add the dependency here? Or is it required that those using wasmtime-wasi have to also depend on wasmtime-wasi-io?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, its possible to not add the dependency and use everything through wasmtime-wasi. I'm happy to convert it over to that if you think that is valuable, I could go either way.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah if you wouldn't mind doing that I think it'd be good to have an example and basically a test for that. In Spin for example I'd hope to ideally only need to refactor a bit and not have to explicitly pick up wasmtime-wasi-io (otherwise we might need to extend docs more to mention it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - hitting merge.

@pchickey pchickey enabled auto-merge January 22, 2025 00:14
@pchickey pchickey added this pull request to the merge queue Jan 22, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 22, 2025
@pchickey pchickey enabled auto-merge January 22, 2025 05:28
@pchickey pchickey added this pull request to the merge queue Jan 22, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 22, 2025
@pchickey pchickey enabled auto-merge January 22, 2025 17:52
@pchickey pchickey added this pull request to the merge queue Jan 22, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 22, 2025
@pchickey pchickey enabled auto-merge January 22, 2025 19:22
@pchickey pchickey added this pull request to the merge queue Jan 22, 2025
Merged via the queue into main with commit ca95576 Jan 22, 2025
41 checks passed
@pchickey pchickey deleted the pch/wasi_io_crate branch January 22, 2025 19:47
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