Stream: git-wasmtime

Topic: wasmtime / PR #10073 refactor: move wasip2 implementation...

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2025 at 14:54):

rvolosatovs edited PR #10073.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2025 at 15:05):

rvolosatovs edited PR #10073:

Introduce a p2 module in WASI crates as suggested in https://github.com/bytecodealliance/wasmtime/pull/10061#pullrequestreview-2565638530

I've moved to wasmtime_wasi::p2 submodule:

I've left a few non-p2 specific things in wasmtime_wasi top-level, which I was able to reuse for p3 impl in https://github.com/bytecodealliance/wasip3-prototyping/pull/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:

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

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2025 at 15:09):

rvolosatovs has marked PR #10073 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2025 at 15:12):

rvolosatovs commented on PR #10073:

@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 https://github.com/bytecodealliance/wasip3-prototyping/pull/115 for both p2 and p3.

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

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2025 at 16:11):

pchickey submitted PR review:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2025 at 16:11):

pchickey created PR review comment:

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?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2025 at 16:11):

pchickey created PR review comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2025 at 17:15):

rvolosatovs commented on PR #10073:

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?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2025 at 17:30):

pchickey commented on PR #10073:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2025 at 17:57):

rvolosatovs updated PR #10073.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2025 at 18:00):

rvolosatovs commented on PR #10073:

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

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2025 at 19:36):

alexcrichton submitted PR review:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2025 at 19:36):

alexcrichton created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2025 at 19:36):

alexcrichton created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2025 at 15:04):

rvolosatovs updated PR #10073.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2025 at 15:06):

rvolosatovs submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2025 at 15:06):

rvolosatovs created PR review comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2025 at 15:09):

rvolosatovs updated PR #10073.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2025 at 17:04):

pchickey submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2025 at 17:04):

pchickey has enabled auto merge for PR #10073.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2025 at 18:14):

pchickey commented on PR #10073:

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

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2025 at 18:14):

pchickey closed without merge PR #10073.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2025 at 18:14):

pchickey has disabled auto merge for PR #10073.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2025 at 18:14):

pchickey reopened PR #10073.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2025 at 18:15):

pchickey has enabled auto merge for PR #10073.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2025 at 18:58):

pchickey merged PR #10073.

Last updated: Dec 06 2025 at 07:03 UTC