Skip to content
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

wasip2 target should not conditionally feature gate stdlib APIs #130323

Open
Manishearth opened this issue Sep 13, 2024 · 5 comments
Open

wasip2 target should not conditionally feature gate stdlib APIs #130323

Manishearth opened this issue Sep 13, 2024 · 5 comments
Labels
C-bug Category: This is a bug. F-wasip2 `#[feature(wasip2)]` O-wasi Operating system: Wasi, Webassembly System Interface O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Manishearth
Copy link
Member

This was introduced in #119616 (cc @rylev)

Currently, any crate using std::os::wasi stuff needs to add a #![feature(wasip2)] if it is to be compiled with --target=wasm32-wasip2. This is due to this line:

#![cfg_attr(target_env = "p2", unstable(feature = "wasip2", issue = "none"))]

This puts library authors targeting stable wasm stuff in an annoying situation of needing to add a feature gate to support downstream users who wish to use unstable wasm stuff (see servo/rust-url#960). This isn't really how feature gates should work: the entity using the feature should be the one opting in. I don't think any of the benefits of feature gates apply if every wasi-using dep in the ecosystem just slaps on a #![cfg_attr(all(target_os = "wasi", target_env = "p2"), feature(wasip2))], which seems likely here.

A somewhat legitimate reason I could see for having such a feature is if the wasip2 target is stable, but certain stdlib things are unstable under that target. It still has the same problems, and I'd argue a target with specially unstable stdlib APIs should not be considered stable, but at least there appears to be a meaningful thing that the stdlib feature gate is gating.

However, it appears to me that wasip2 is just unstable: what's the purpose of gating the stdlib API? If the goal is to gate the target, why not require -Zunstable-options as we do for other CLI-level unstable things?

@Manishearth Manishearth added the C-bug Category: This is a bug. label Sep 13, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 13, 2024
@Noratrieb
Copy link
Member

@alexcrichton

@alexcrichton
Copy link
Member

Thanks for the cc here, and thanks for the issue! This is an intentional de-stabilization because the bindings in std::os::wasi are tailored to exactly WASIp1, not WASIp2. I recommended @rylev make this module unstable on the p2 target to provide freedom for the standard library to change its implementation over time. The module will likely be removed and/or entirely changed on the WASIp2 target, notably using std::os::wasip2 instead of std::os::wasi.

It's worth noting though that this is a very coarse de-stabilization. It looks like rust-url is using std::os::wasi::prelude::OsStrExt which is indeed the same on wasip2 and wasip1. In such a situation it would make sense to make that stable on the p2 target. (this is the sort of thing though where I wanted to make sure stabilization was explicitly opted-in-to instead of accidentally happening)

I'll try to work on a PR to flag some bits that make sense as stable.

@Manishearth do you know of other crates that are adding this feature? If so could you point me to them so I can review what bits are being used?

@Manishearth
Copy link
Member Author

@alexcrichton I believe @brooksmtownsend has been going around makiing this work across reqwest deps. I appreciate the work to stabilize some of those bits!

Honestly, I suspect a cleaner path forward would be to make std::os::wasi _unreachable) in wasip2 and instead expose an unstable std::os::wasip2 that's actually just reexports for now. This is similar to how support for other new platforms works for ecosystem crates, at the moment it feels like it's just elbowing in on the existing wasm code in a way that's confusing.

@alexcrichton
Copy link
Member

Yeah I'm thinking similarly. For rust-url in the meantime I might recommend using .to_str() on WASI platforms instead of .as_bytes() since it's practically infallible as all WASI paths are valid utf-8.

@lolbinarycat lolbinarycat added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ O-wasi Operating system: Wasi, Webassembly System Interface F-wasip2 `#[feature(wasip2)]` and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 14, 2024
@brooksmtownsend
Copy link

Thanks @alexcrichton , that's the path I took in the above mentioned PR and that worked pretty well.

Stebalien added a commit to Stebalien/tempfile that referenced this issue Nov 8, 2024
- wasip2 will require +nightly until rust-lang/rust#130323 is resolved and/or std::os::wasip2 is available in stable.
- Support was added to rustix for version 0.38.39 bytecodealliance/rustix#1205

Signed-off-by: Colin Murphy <[email protected]>
Co-authored-by: Steven Allen <[email protected]>
cdmurph32 added a commit to cdmurph32/c2pa-rs that referenced this issue Nov 12, 2024
- wasip2 will require +nightly until rust-lang/rust#130323 is resolved and/or std::os::wasip2 is available in stable.
- Support was added to rustix for version 0.38.39 bytecodealliance/rustix#1205
- Support was added to tempfile for version 3.14 Stebalien/tempfile#305
cdmurph32 added a commit to cdmurph32/c2pa-rs that referenced this issue Nov 12, 2024
- wasip2 will require +nightly until rust-lang/rust#130323 is resolved and/or std::os::wasip2 is available in stable.
- Support was added to rustix for version 0.38.39 bytecodealliance/rustix#1205
- Support was added to tempfile for version 3.14 Stebalien/tempfile#305
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. F-wasip2 `#[feature(wasip2)]` O-wasi Operating system: Wasi, Webassembly System Interface O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants