refactor: move wasip2 implementation to wasmtime_wasi::p2#10073
refactor: move wasip2 implementation to wasmtime_wasi::p2#10073pchickey merged 3 commits intobytecodealliance:mainfrom
wasmtime_wasi::p2#10073Conversation
815b81a to
29b9cad
Compare
cec5af9 to
94e95e6
Compare
|
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'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. |
|
I agree with all of Alex's broad strokes here. I also want to point out that landing PRs to support wasip3 in wasmtime
Agree, I don't like
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.
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. |
|
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 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
Each of the crates contains From the embedder's perspective:
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 With the above in mind, I feel like it may be premature to break all downstream 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. |
|
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 I'm also not personally sold on an |
058218c to
6c5d7a2
Compare
5e2ae49 to
963210f
Compare
963210f to
1554ff1
Compare
|
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 |
alexcrichton
left a comment
There was a problem hiding this comment.
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 | |||
There was a problem hiding this 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.
| @@ -0,0 +1,573 @@ | |||
| use crate::p2::bindings::filesystem::types; | |||
There was a problem hiding this 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.
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
1554ff1 to
c0ed81a
Compare
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
c0ed81a to
de3803b
Compare
|
Closing and reopening to hopefully get CI to work... |
Pull request was closed
`IOView` is split off of `WasiView`. bytecodealliance/wasmtime#10016 `static_memory_maximum_size` is now `memory_reservation`. bytecodealliance/wasmtime#9545 `detect_precompiled` is now associated function. bytecodealliance/wasmtime#10405 Moved WASIp2 related things to `wasmtime_wasi::p2` module. bytecodealliance/wasmtime#10073
`IOView` is split off of `WasiView`. bytecodealliance/wasmtime#10016 `static_memory_maximum_size` is now `memory_reservation`. bytecodealliance/wasmtime#9545 `detect_precompiled` is now associated function. bytecodealliance/wasmtime#10405 Moved WASIp2 related things to `wasmtime_wasi::p2` module. bytecodealliance/wasmtime#10073
`IOView` is split off of `WasiView`. bytecodealliance/wasmtime#10016 `static_memory_maximum_size` is now `memory_reservation`. bytecodealliance/wasmtime#9545 `detect_precompiled` is now associated function. bytecodealliance/wasmtime#10405 Moved WASIp2 related things to `wasmtime_wasi::p2` module. bytecodealliance/wasmtime#10073
wasi interfaces are moved to a separate p2 module in preparation for p3 introduction, see bytecodealliance/wasmtime#10073 cache config can now either be specified in a file or in code, see bytecodealliance/wasmtime#10665
wasi interfaces are moved to a separate p2 module in preparation for p3 introduction, see bytecodealliance/wasmtime#10073 cache config can now either be specified in a file or in code, see bytecodealliance/wasmtime#10665
wasi interfaces are moved to a separate p2 module in preparation for p3 introduction, see bytecodealliance/wasmtime#10073 cache config can now either be specified in a file or in code, see bytecodealliance/wasmtime#10665
`IOView` is split off of `WasiView`. bytecodealliance/wasmtime#10016 `static_memory_maximum_size` is now `memory_reservation`. bytecodealliance/wasmtime#9545 `detect_precompiled` is now associated function. bytecodealliance/wasmtime#10405 Moved WASIp2 related things to `wasmtime_wasi::p2` module. bytecodealliance/wasmtime#10073
`IOView` is split off of `WasiView`. bytecodealliance/wasmtime#10016 `static_memory_maximum_size` is now `memory_reservation`. bytecodealliance/wasmtime#9545 `detect_precompiled` is now associated function. bytecodealliance/wasmtime#10405 Moved WASIp2 related things to `wasmtime_wasi::p2` module. bytecodealliance/wasmtime#10073
`IOView` is split off of `WasiView`. bytecodealliance/wasmtime#10016 `static_memory_maximum_size` is now `memory_reservation`. bytecodealliance/wasmtime#9545 `detect_precompiled` is now associated function. bytecodealliance/wasmtime#10405 Moved WASIp2 related things to `wasmtime_wasi::p2` module. bytecodealliance/wasmtime#10073
Introduce a
p2module in WASI crates as suggested in #10061 (review)I've moved to
wasmtime_wasi::p2submodule:I've left a few non-p2 specific things in
wasmtime_wasitop-level, which I was able to reuse for p3 impl in bytecodealliance/wasip3-prototyping#115I currently do not see a way to reuse
WasiCtxfor 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)WasiP3CtxandWasiP2Ctx, but for now I've just moved the context intop2submodule, since I think we will always have a wasip{N}-specificWasiP{N}Ctx, particularly because set of interfaces included in WASI, as well as their semantics, might change across versionsI've added 3 more commits on top of the move, which seemed to make sense for consistency:
WasiCtx->WasiP2CtxWasiView->WasiP2ViewWasiImpl->WasiP2ImplI opted not to rename existing
preview1andpreview0modules/features to avoid breaking changes.cc @pchickey