Skip to content

refactor: move wasip2 implementation to wasmtime_wasi::p2 #10073

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

Merged
merged 3 commits into from
Apr 15, 2025

Conversation

rvolosatovs
Copy link
Member

@rvolosatovs rvolosatovs commented Jan 22, 2025

Introduce a p2 module in WASI crates as suggested in #10061 (review)

I've moved to wasmtime_wasi::p2 submodule:

  • wasip2 WIT files
  • generated wasip2 bindings
  • wasip2 host implementations
  • wasip2 context, view and impl structs

I've left a few non-p2 specific things in wasmtime_wasi top-level, which I was able to reuse for p3 impl in bytecodealliance/wasip3-prototyping#115

I currently do not see a way to reuse WasiCtx for wasip3, specifically due to the difference in I/O handling, which would introduce the incompatibility for stdio config between wasip2 and wasip3. It is possible that we can introduce some shared abstraction, which could be reused by both (future) WasiP3Ctx and WasiP2Ctx, but for now I've just moved the context into p2 submodule, since I think we will always have a wasip{N}-specific WasiP{N}Ctx, particularly because set of interfaces included in WASI, as well as their semantics, might change across versions

I've added 3 more commits on top of the move, which seemed to make sense for consistency:

  • rename WasiCtx -> WasiP2Ctx
  • rename WasiView -> WasiP2View
  • rename WasiImpl -> WasiP2Impl

I opted not to rename existing preview1 and preview0 modules/features to avoid breaking changes.
cc @pchickey

@rvolosatovs rvolosatovs force-pushed the feat/p2-module branch 4 times, most recently from 815b81a to 29b9cad Compare January 22, 2025 11:07
@rvolosatovs rvolosatovs marked this pull request as ready for review January 22, 2025 11:08
@rvolosatovs rvolosatovs requested review from a team as code owners January 22, 2025 11:08
@rvolosatovs rvolosatovs requested review from fitzgen and removed request for a team January 22, 2025 11:08
@alexcrichton alexcrichton requested review from alexcrichton and removed request for a team and fitzgen January 22, 2025 17:34
@alexcrichton
Copy link
Member

Thanks for this! I'm going to continue the discussion from #10061 over here since this looks like it's going to be first. I'll note that whatever we end up doing here for p3 is a relatively big change to consider depending on how it ends up which sort of borders along the lines of "maybe this should have an RFC". For example upon reading reading Pat's comment I initially reacted thinking that we should instead do something else. After thinking more though this seems like a more reasonable approach.

That being said though I'd at least personally still have thoughts on this, for example:

  • I'm not sure if we want to keep a pub use p2::* reexport myself.
  • This probably wants to (eventually, not necessarily here), come with a rename of the preview0 and preview1 modules to p0 and p1.
  • We might want to hold off on changing other wasmtime-wasi-* crates for now until APIs are ready for those. Basically pave a path with the core wasmtime-wasi crate but otherwise defer the actual changes to future crates to when we've shaken out all the issues here.

I'll also note though that I'm not necessarily saying this requires an RFC. I find though that it's not always the greatest medium to have a design discussion when there's a PR because it's easy to get into the mindset of "well the PR does it this way so I guess we'll just go with that". RFCs have their own downsides of course though.

In the abstract though I think we should ideally design for where we want to end up a year or two from now. At that point WASIp3 is stable and will be the "primary" APIs that folks use. Given our destination end point first then I think we can work backwards and consider things like breaking changes, refactorings, migration paths, etc. I've historically found that only designing in incremental steps from where we are today, for example trying to minimize breaking changes, doesn't always result in the best design.

@pchickey
Copy link
Contributor

pchickey commented Jan 22, 2025

I agree with all of Alex's broad strokes here.

I also want to point out that landing PRs to support wasip3 in wasmtime main doesn't feel urgent to me. In the corresponding point in the p2 development process, we forked off a prototyping repo where we could thrash things around a bunch and not worry about compatibility as we iterated on the specs and implementations towards working code. So, aside from benefiting from more discussion of our desired end state and working backwards to figure out the changes and migrations to get there, I personally would benefit from seeing a more fleshed out implementation of p3 looks like. Currently it is spread among several different authors, repos, and PRs. Additionally, right now my employer is asking me to solve a totally different set of problems, so I don't have the bandwidth to engage with the p3 implementation process deep enough to collect all that context.

I'm not sure if we want to keep a pub use p2::* reexport myself.

Agree, I don't like use *. Please list all of the identifiers re-exported. (Is that the aspect @alexcrichton objects to?)

This probably wants to (eventually, not necessarily here), come with a rename of the preview0 and preview1 modules to p0 and p1.

Eventually to never would also be my prioritization. Consistency is nice in the abstract, but in this case I don't think its very important, since the interfaces being exposed by preview0 and 1 are for witx, and the rest are for components anyway. And its definitely not urgent to change this, since it would break existing users.

We might want to hold off on changing other wasmtime-wasi-* crates for now until APIs are ready for those. Basically pave a path with the core wasmtime-wasi crate but otherwise defer the actual changes to future crates to when we've shaken out all the issues here.

Agree - lets come up with a repeatable pattern that can be applied to other crates as needed, but lets start by only applying it to wasmtime-wasi now and apply it to others immediately before landing a 0.3-draft impl in those. That way, if we discover in the buildout of wasmtime-wasi that the pattern isnt quite right, we can course-correct with a minimum of thrash.

@rvolosatovs rvolosatovs marked this pull request as draft January 23, 2025 13:55
@alexcrichton
Copy link
Member

Oh to clarify for the pub use personally I prefer names to only be in one location as opposed to multiple, so I would advocate for moving most of the preexisting crate to pub mod p2 and leaving out the top level re-export.

Roman would you be up for making that change and reverting other crates to their original state? That I think should create enough space to start experimenting with p3 in the main crate would be my hope.

@rvolosatovs
Copy link
Member Author

Oh to clarify for the pub use personally I prefer names to only be in one location as opposed to multiple, so I would advocate for moving most of the preexisting crate to pub mod p2 and leaving out the top level re-export.

Roman would you be up for making that change and reverting other crates to their original state? That I think should create enough space to start experimenting with p3 in the main crate would be my hope.

For my third attempt starting from main, I've went with a slightly different approach, which may eventually converge with the approach we're discussing here. In particular, I've split out each WASI package implementation into a separate crate:

  • wasmtime-wasi-clocks
  • wasmtime-wasi-random
  • wasmtime-wasi-filesystem
  • wasmtime-wasi-sockets
  • wasmtime-wasi-cli

Each of the crates contains p3 module with generated bindings and implementation. Eventually, p2 modules could be introduced to these crates as well.

From the embedder's perspective:

  • wasmtime_wasi_cli::p2::add_to_linker would function analogous to existing wasmtime_wasi::add_to_linker, linking in all wasi:cli/[email protected] (clocks, random etc.), it's gated behind p2 feature flag
  • wasmtime_wasi_cli::p3::add_to_linker would link in all wasi:cli/[email protected] (behind p3 feature flag)
  • wasmtime_wasi_cli::add_to_linker would link in both wasi:cli/[email protected] and wasi:cli/[email protected] (behavior can be configured with crate feature flags)

The exact same strategy is taken for other proposals, like clocks, random etc.

The benefit is that embedders can select which WASI interface implementations to link in as opposed to all-or-nothing approach (or, all/nothing/wasi:io now that #10036 is merged).

This approach also insulates the p3 work from changes in wasmtime_wasi (like #10036) and, in fact, avoids any breaking changes for users.

With the above in mind, I feel like it may be premature to break all downstream wasmtime_wasi users at this point by moving (as opposed to duplicating) current bindings into a p2 namespace.
I feel like wasmtime-wasi-cli crate could instead serve as a replacement for wasmtime_wasi crate in it's current shape. In that scenario wasmtime_wasi crate may still serve a purpose of a minimal "utility" crate (however I'd rather find a way to deprecate it altogether) and most downstream users would still only need to import a single crate (like wasmtime_wasi_cli)

I'll add an agenda item to next Wasmtime bi-weekly meeting to discuss this proposal in more detail.

For now I'll continue with crate-per-package approach.

@alexcrichton
Copy link
Member

I'm not sure I'm sold on the idea of multiple crates here, but if you'd prefer to discuss in a meeting I think that's reasonable too. The wasmtime-wasi-io split was some work @pchickey did for no_std compat which I think is still ongoing. In that sense I wouldn't necessarily consider it "final" in the sense of all other crates should look like that. In retrospect one other alternative design would be for a std feature that gates most of the crate except the wasmtime-wasi-io bits. In general I'm not sure if there's much use case for slicing and dicing WASI impls so much given how tightly coupled everything is right now (e.g. if you have cap-std you have the entirety of all these crates)

I'm also not personally sold on an add_to_linker which adds 0.2 and 0.3 functionality just yet. In the future I could see 0.2 becoming legacy and not desired by default, so it'd need a way to opt-out anyway.

@rvolosatovs rvolosatovs force-pushed the feat/p2-module branch 2 times, most recently from 058218c to 6c5d7a2 Compare April 9, 2025 17:20
@github-actions github-actions bot added wasi Issues pertaining to WASI wasmtime:c-api Issues pertaining to the C API. labels Apr 9, 2025
@rvolosatovs rvolosatovs force-pushed the feat/p2-module branch 3 times, most recently from 5e2ae49 to 963210f Compare April 10, 2025 14:46
@rvolosatovs rvolosatovs changed the title refactor: introduce p2 module refactor: move wasip2 implementation to wasmtime_wasi::p2 Apr 10, 2025
@rvolosatovs rvolosatovs marked this pull request as ready for review April 10, 2025 15:09
@rvolosatovs
Copy link
Member Author

@alexcrichton @pchickey since majority of wasip3 implementation work is done, I've came back to this PR to hopefully integrate it in https://github.com/bytecodealliance/wasip3-prototyping.
The p3 module split pattern works pretty well in that repository for wasmtime_wasi and wasmtime_wasi_http. I've also been able to reuse quite a bit of wasmtime_wasi in bytecodealliance/wasip3-prototyping#115 for both p2 and p3.

I've reworked this PR and marking it as ready for review now, please take a look!

Copy link
Contributor

@pchickey pchickey left a comment

Choose a reason for hiding this comment

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

I'm 50/50 on renaming WasiCtxBuilder, WasiCtx, WasiView to all have P2 in their name. They are already in the p2:: namespace which should be enough. On the other hand it feels like bikeshedding. @alexcrichton wdyt? I'm basically fine either way but not renaming them makes the diffs simpler in this crate and in users.

@@ -0,0 +1,573 @@
use crate::p2::bindings::filesystem::types;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this got tracked by git as a delete and re-creation, can you change that into a rename so that we retain history prior to the rename?

Copy link
Member

Choose a reason for hiding this comment

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

You're right @rvolosatovs that with the squash-and-merge strategy we have the history here won't get preserved even if the PR preserves the history. That being said could you try renaming src/filesystem.rs to something like src/fs.rs? That might tip heuristics the other direction to see the rename here. I think the issue is that some of src/filesystem.rs was retained while most was moved here, and by having the original copy still preserved it might be confusing the rename detection.

We could then follow-up with a commit to rename src/fs.rs to src/filesystem.rs if we really wanted too which I think would fixup the git history here.

@@ -0,0 +1,44 @@
use crate::p2::bindings::sockets::network::{ErrorCode, Ipv4Address, Ipv6Address};
Copy link
Contributor

Choose a reason for hiding this comment

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

This also should be a rename, rather than delete and creation

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, that can't be done with the squash merge strategy.

@rvolosatovs
Copy link
Member Author

I'm 50/50 on renaming WasiCtxBuilder, WasiCtx, WasiView to all have P2 in their name. They are already in the p2:: namespace which should be enough. On the other hand it feels like bikeshedding. @alexcrichton wdyt? I'm basically fine either way but not renaming them makes the diffs simpler in this crate and in users.

FWIW, I'm actually in favor of not including the P2 in the name, IMO the p2 namespace is enough, I only opted for doing that for consistency with e.g. preview1::WasiP1Ctx

Should I drop the top 3 commits?

@pchickey
Copy link
Contributor

OK, if you're not in favor of putting P2 in the name its easy to just say, lets not do that, drop those commits.

@rvolosatovs
Copy link
Member Author

I've used the "merge" trick I've adapted from https://dev.to/deckstar/how-to-git-copy-copying-files-while-keeping-git-history-1c9j to duplicate network.rs and filesystem.rs to preserve their history after the split, however I'm pretty sure that will be lost if this PR is merged as a squash, since when converted to a single commit git treats this as a modification of an existing file and creation of a new one

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Agreed leaving "P2" out of the names, and some follow-ups we can do after this are to remove "P1" from names and rename preview{0,1} to p{0,1} as well I think. In the future it might make sense to lift more of p2/*.rs to the root level as well, things like tcp/udp sockets etc. That's purely for internal organization though and can be deferred to later as-needed.

@@ -1,272 +1,23 @@
//! # Wasmtime's WASI Implementation
Copy link
Member

Choose a reason for hiding this comment

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

Mind keeping some of the crate comment here? My rough goal has been to make the landing page relatively informative. No need to duplicate p1/p2 docs, but having pointers/links to those modules and an outline of the high-level organization here I think would be good.

@@ -0,0 +1,573 @@
use crate::p2::bindings::filesystem::types;
Copy link
Member

Choose a reason for hiding this comment

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

You're right @rvolosatovs that with the squash-and-merge strategy we have the history here won't get preserved even if the PR preserves the history. That being said could you try renaming src/filesystem.rs to something like src/fs.rs? That might tip heuristics the other direction to see the rename here. I think the issue is that some of src/filesystem.rs was retained while most was moved here, and by having the original copy still preserved it might be confusing the rename detection.

We could then follow-up with a commit to rename src/fs.rs to src/filesystem.rs if we really wanted too which I think would fixup the git history here.

@pchickey pchickey enabled auto-merge April 15, 2025 17:04
@pchickey
Copy link
Contributor

Closing and reopening to hopefully get CI to work...

@pchickey pchickey closed this Apr 15, 2025
auto-merge was automatically disabled April 15, 2025 18:14

Pull request was closed

@github-project-automation github-project-automation bot moved this from In progress to Done in Ship WASIp3 Apr 15, 2025
@pchickey pchickey reopened this Apr 15, 2025
@pchickey pchickey enabled auto-merge April 15, 2025 18:15
@pchickey pchickey added this pull request to the merge queue Apr 15, 2025
Merged via the queue into bytecodealliance:main with commit 7399206 Apr 15, 2025
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI wasmtime:c-api Issues pertaining to the C API.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants