-
Notifications
You must be signed in to change notification settings - Fork 13.7k
std: Start supporting WASIp2 natively #145944
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
base: master
Are you sure you want to change the base?
Conversation
The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
Procedurally this contains a git dependency on the Otherwise though my hope was to open this up a bit earlier to get some feedback and start the review process. |
ed77afb
to
5839c8b
Compare
This comment has been minimized.
This comment has been minimized.
It's also worth noting that these changes are not tested on CI because CI does not test the Also, I started with a few commits around the build system for |
This comment has been minimized.
This comment has been minimized.
} | ||
|
||
pub fn available_parallelism() -> io::Result<NonZero<usize>> { | ||
crate::sys::unsupported() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale for returning an error instead of e.g. constant 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is following the precedent of wasip1's implementation. Following the git-blame trail that goes back to #74480, the original implementation, which had a fallback of an error for all unknown platforms.
Personally I'd say it's somewhat accurate to leave this as unsupported for now since that basically forces callers to assume it's 1 and forces a threaded implementation in the future to come through and implement this as opposed to accidentally forgetting about it
@@ -0,0 +1,9 @@ | |||
pub fn fill_bytes(bytes: &mut [u8]) { | |||
bytes.copy_from_slice(&wasip2::random::random::get_random_bytes( | |||
u64::try_from(bytes.len()).unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this unwrap be fully optimised out since we're on a 32bit target?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5839c8b
to
f5d3e73
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
f5d3e73
to
039681b
Compare
@@ -798,7 +799,7 @@ fn check_runtime_no_duplicate_dependencies(metadata: &Metadata, bad: &mut bool) | |||
continue; | |||
} | |||
|
|||
if !seen_pkgs.insert(&*pkg.name) { | |||
if pkg.name.to_string() != "wasi" && !seen_pkgs.insert(&*pkg.name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to call out these two changes in particular. Notably the standard library on the wasm32-wasip2 target is picking up a new dependency, wit-bindgen
. This is developed at https://github.com/bytecodealliance/wit-bindgen/ and basically follows the same development practices of https://github.com/bytecodealliance/wasi-rs, a preexisting dependency of the standard library.
The "wasi" check here is then to allow duplicate dependencies on the "wasi" crate from the standard library since it's intentional that wasm32-wasip1 depends on 0.11 and wasm32-wasip2 depends on 0.14.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind adding a comment about why wasi is special here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would make sense to keep a pal/wasi
directory that contains shared bits, then have pal/wasi/p1
and pal/wasi/p2
? Similar to how pal/unix
has common code plus some platform-specific modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd personally say that the end state is going to be different enough such that when the wasi
0.11 crate is dropped as a dependency from the wasm32-wasip2
target that it's probably worth it to go ahead and split the implementation. The only file in library/std/src/sys/pal/wasi/*.rs
that'll be shared between the two is os.rs
which is more like a wasi-libc layer than a WASIpN layer.
I'm happy to do either way, though, but my gut is that WASIp{1,2} are different enough it'll be easier in the long run to have things split.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the context, that makes sense 👍. Is the wasip1 target probably eventually going to wind up removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'd be tied to the removal of the wasm32-wasip1
target entirely. So long as there's a wasm32-wasip1
target, however, it's unlikely to get deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I meant, is rustc probably going to keep supporting wasip1 more or less forever? Or is wasip2+ basically expected to absorb its usecases and replace it?
(I'm just not sure what the long term support story looks like being that these are "preview"s)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I missed the keyword "target"... There goes my reading comprehension again...
Regardless though the answer is probably a bit nuanced. From the perspective of the WASI subgroup, the official governing body of WASI, WASIp1 is and has been dead for years as a standard. In that sense it's already in the "people need to migrate" mode.
In a different sense though WASIp1 has seen widespread adoption and some users are currently unwilling to update to WASIp2. There's a variety of reasons for this which range from technical to political.
My personal answer is "yes this target will be removed, but on the timescale of 5+ years". I at least do not plan on being listed as a maintainer after that point and I'd leave it in the hands of others if they choose to maintain it.
// For testing `wasm32-wasip2`-and-beyond it's required to have | ||
// `wasm-component-ld`. This is enabled by default via `tool_enabled` | ||
// but if it's disabled then double-check it's present on the system. | ||
if target.contains("wasip") | ||
&& !target.contains("wasip1") | ||
&& !build.tool_enabled("wasm-component-ld") | ||
{ | ||
cmd_finder.must_have("wasm-component-ld"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cc @rust-lang/bootstrap in case anybody wants to look at this
@@ -798,7 +799,7 @@ fn check_runtime_no_duplicate_dependencies(metadata: &Metadata, bad: &mut bool) | |||
continue; | |||
} | |||
|
|||
if !seen_pkgs.insert(&*pkg.name) { | |||
if pkg.name.to_string() != "wasi" && !seen_pkgs.insert(&*pkg.name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind adding a comment about why wasi is special here?
For `wasm32-wasip2`-and-beyond this tool is required, so in case it's disabled in `config.toml` add a sanity-check that it's present in the environment.
This commit is the start of an effort to support WASIp2 natively in the standard library. Before this commit the `wasm32-wasip2` target behaved exactly like `wasm32-wasip1` target by importing APIs from the core wasm module `wasi_snapshot_preview1`. These APIs are satisfied by the `wasm-component-ld` target by using an [adapter] which implements WASIp1 in terms of WASIp2. This adapter comes at a cost, however, in terms of runtime indirection and instantiation cost, so ideally the adapter would be removed entirely. The purpose of this adapter was to provide a smoother on-ramp from WASIp1 to WASIp2 when it was originally created. The `wasm32-wasip2` target has been around for long enough now that it's much more established. Additionally the only thing historically blocking using WASIp2 directly was implementation effort. Work is now underway to migrate wasi-libc itself to using WASIp2 directly and now seems as good a time as any to migrate the Rust standard library too. Implementation-wise the milestones here are: * The `wasm32-wasip2` target now also depends on the `wasi` crate at version 0.14.* in addition to the preexisting dependency of 0.11.*. The 0.14.* release series binds WASIp2 APIs instead of WASIp1 APIs. * Some preexisting naming around `mod wasi` or `wasi.rs` was renamed to `wasip1` where appropriate. For example `std::sys::pal::wasi` is now called `std::sys::pal::wasip1`. * More platform-specific WASI modules are now split between WASIp1 and WASIp2. For example getting the current time, randomness, and process arguments now use WASIp2 APIs directly instead of using WASIp1 APIs that require an adapter. It's worth pointing out that this PR does not migrate the entire standard library away from using WASIp1 APIs on the `wasm32-wasip2` target. Everything related to file descriptors and filesystem APIs is still using WASIp1. Migrating that is left for a future PR. In the meantime the goal of this change is to lay the groundwork necessary for migrating in the future. Eventually the goal is to drop the `wasi` 0.11.* dependency on the `wasm32-wasip2` target (the `wasm32-wasip1` target will continue to retain this dependency). [adapter]: https://github.com/bytecodealliance/wasmtime/blob/main/crates/wasi-preview1-component-adapter/README.md
039681b
to
5d81f03
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The std part looks good to me, I'll give another day or so if anybody from the relevant teams wants to take a peek at bootstrap or tidy.
This commit is the start of an effort to support WASIp2 natively in the
standard library. Before this commit the
wasm32-wasip2
target behavedexactly like
wasm32-wasip1
target by importing APIs from the core wasmmodule
wasi_snapshot_preview1
. These APIs are satisfied by thewasm-component-ld
target by using an adapter which implements WASIp1in terms of WASIp2. This adapter comes at a cost, however, in terms of
runtime indirection and instantiation cost, so ideally the adapter would
be removed entirely. The purpose of this adapter was to provide a
smoother on-ramp from WASIp1 to WASIp2 when it was originally created.
The
wasm32-wasip2
target has been around for long enough now that it'smuch more established. Additionally the only thing historically blocking
using WASIp2 directly was implementation effort. Work is now underway to
migrate wasi-libc itself to using WASIp2 directly and now seems as good
a time as any to migrate the Rust standard library too.
Implementation-wise the milestones here are:
wasm32-wasip2
target now also depends on thewasi
crate atversion 0.14.* in addition to the preexisting dependency of 0.11..
The 0.14. release series binds WASIp2 APIs instead of WASIp1 APIs.
mod wasi
orwasi.rs
was renamed towasip1
where appropriate. For examplestd::sys::pal::wasi
is nowcalled
std::sys::pal::wasip1
.WASIp2. For example getting the current time, randomness, and
process arguments now use WASIp2 APIs directly instead of using WASIp1
APIs that require an adapter.
It's worth pointing out that this PR does not migrate the entire
standard library away from using WASIp1 APIs on the
wasm32-wasip2
target. Everything related to file descriptors and filesystem APIs is
still using WASIp1. Migrating that is left for a future PR. In the
meantime the goal of this change is to lay the groundwork necessary for
migrating in the future. Eventually the goal is to drop the
wasi
0.11.* dependency on the
wasm32-wasip2
target (thewasm32-wasip1
target will continue to retain this dependency).