wasm: support for wasm32-wasip2 target#12653
Conversation
|
But why? Is there any usecase wasip1 cannot cover? If wasip2 works on web browsers using shim, we should drop wasip1 at the same time. |
|
The main reason to have WASI Preview 2 is because Preview 1 is not being further developed. In wasip1, we would never get full support for streams and socket. With wasip2, on other hand, we will have those as well as support for the WebAssembly Component Model. This documentation might help answering you: https://component-model.bytecodealliance.org/design/why-component-model.html#interoperability Should we drop wasip1? Probably not yet. There are few features that are missing in Preview2:
|
|
Ok. But do you want to publish release binaries too? Or is |
| new_ucmd!() | ||
| .args(&["--from=iec-i", "10M"]) | ||
| .fails_with_code(2) | ||
| .fails_with_code(if cfg!(wasip2_runner) { 1 } else { 2 }) |
There was a problem hiding this comment.
I have written now in docs/src/wasi-test-gaps.md some details on that. The WASI Preview 2, as currently implemented in Rust std, only allows to exit with code 0 (success) or 1 (failure).
There was a problem hiding this comment.
It is limitation for wasip2 instead of ideal behaviour. So we should just guard the test by cfg.
There was a problem hiding this comment.
Is exit 2 not buildable? or buildable with exit 1 fallback?
There was a problem hiding this comment.
It will be done as part of other PR.
|
GNU testsuite comparison: |
For me, I would benefit of having the released binary for wasip2. I understand it adds a bit more complexity to maintain it, but I think the burden to keep wasip2 is basically the same as wasip1. |
5feb45e to
732f48f
Compare
db9d3ff to
712ff58
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Correctness of error handling is unrelated with initial support (cargo build). Please split PR for less diff. |
712ff58 to
e5bfc97
Compare
|
@oech3, done. I only left the changes to run the build successfully. I am skipping the integration tests for wasip2 as it will be part of the PR related to error handling. |
e5bfc97 to
042f481
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
|
@codspeedbot fix this regression |
|
this is probably noise, you can ignore the perf warning |
|
@sylvestre and @oech3, I have rebased it. So let me know if you need me to do anything about those perf warnings, ok? |
042f481 to
749d289
Compare
|
@sylvestre and @oech3, as far I aware there is no other pending issues. If so, this is ready to be approved and merged. |
This comment was marked as resolved.
This comment was marked as resolved.
|
(also I have no priv for merge) |
That was already fixed. I pushed a small change in order to run the integration test of |
484c0af to
02c6ba8
Compare
|
@sylvestre, could I get your approval on this one, please? I just need that into main so I can create the other PR to enable the integration tests for wasip2. |
This comment was marked as resolved.
This comment was marked as resolved.
| cargo test --target wasm32-wasip1 --no-default-features $UTILS | ||
| cargo test --target ${{ matrix.job.target }} --no-default-features $UTILS | ||
| - name: Run integration tests via wasmtime | ||
| if: matrix.job.target == 'wasm32-wasip1' |
There was a problem hiding this comment.
no case for wasm32-wasip2 ?
There was a problem hiding this comment.
Skipping integration tests for now until I fix all the error handling issues. It will be in the next PR.
| #[cfg(unix)] | ||
| use std::os::unix::ffi::{OsStrExt, OsStringExt}; | ||
| #[cfg(target_os = "wasi")] | ||
| #[cfg(all(target_os = "wasi", target_env = "p1"))] |
There was a problem hiding this comment.
sorry, what happens when using p2 ?
There was a problem hiding this comment.
It fails to compile with:
error[E0658]: use of unstable library feature
wasip2
--> src/uucore/src/lib/lib.rs:151:26
|
151 | use std::os::wasi::ffi::{OsStrExt, OsStringExt};
I am avoiding it so not to deal with needing Rust nightly. Additionally, WASI should only expect valid UTF-8 paths (as stated here and in other parts of the WASI specification).
| // TODO(#7542): Re-enable on Android once we figure out why setting limit is broken. | ||
| // #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| #[cfg(target_os = "linux")] | ||
| #[cfg_attr(wasi_runner, ignore = "WASI: resource limits not supported")] |
There was a problem hiding this comment.
allready guarded by linux. Why is this needed? Same for other tests.
There was a problem hiding this comment.
As I have removed those changes per your request, I will address them as part of the next PR, ok?
| new_ucmd!() | ||
| .args(&["--from=iec-i", "10M"]) | ||
| .fails_with_code(2) | ||
| .fails_with_code(if cfg!(wasip2_runner) { 1 } else { 2 }) |
There was a problem hiding this comment.
Is exit 2 not buildable? or buildable with exit 1 fallback?
|
My comments were pending. Sorry for noise |
Co-authored-by: oech3 <79379754+oech3@users.noreply.github.com>
d843910 to
dca4733
Compare
Add support for the wasm32-wasip2 (WASI Preview 2) target, enabling coreutils to compile and be used on sandboxed environments that can run WebAssembly components.
Changes
yesanduucorelibraries)