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

Disable support for wasm32-wasi #3233

Merged
merged 5 commits into from Aug 7, 2023
Merged

Disable support for wasm32-wasi #3233

merged 5 commits into from Aug 7, 2023

Conversation

gbj
Copy link
Contributor

@gbj gbj commented Jan 12, 2023

At present, wasm-bindgen inserts bindings on any target that has a) the target architecture wasm32 and b) not the target OS emscripten. This means that wasm-bindgen does insert bindings for the target wasm32-wasi, which causes wasm32-wasi binaries to behavior differently from binaries on all other targets. See further discussion here bytecodealliance/wasmtime#5557

This PR simply updates the "is this Emscripten?" check to add an "is this WASI?" check, leading to a panic if a wasm-bindgen function is called but not preventing WASI runtimes like wasmtime from loading and running the binary.

I've tested this against the very example I gave in my issue in the wasmtime repo (linked above), and it resolved that issue, allowing you to load and run a binary with wasm-bindgen dependencies (like web-sys) as long as you don't actually run them — as is the behavior on other native targets.

@bajtos
Copy link

bajtos commented Jan 12, 2023

AFAIK, there are browser-like JS environments that support WASI too - Node.js and Deno.

Ideally, we should be able to compile Rust code for the wasm32-wasi target, while using wasm-bindgen for interfacing with JS APIs.

Isn't this pull request breaking support for such scenario?

@gbj
Copy link
Contributor Author

gbj commented Jan 12, 2023

Oh, interesting point. I’m not familiar enough with Node or Deno WASI support — is there any distinction in compile target between “WASI within JS environment” and “WASI without JS environment”? If not, that’s… unfortunate.

@bajtos
Copy link

bajtos commented Jan 16, 2023

Oh, interesting point. I’m not familiar enough with Node or Deno WASI support — is there any distinction in compile target between “WASI within JS environment” and “WASI without JS environment”? If not, that’s… unfortunate.

I am afraid I am not an expert in this area either :(

I think there are very few people that are running WASI in Node.js & Deno context, so maybe it's not that bad if wasm-bindgen stops working for wasm32-wasi after your change.

I have two suggestions if I may:

  1. Could you please change the title of your pull request to explain what is the effect of your change on the users, instead of describing the implementation details? For example: "Don't inject bindings when targeting wasm32-wasi" or "Disable support for wasm32-wasi".

  2. I think this should be treated as a breaking change and clearly described in the release notes.

@gbj gbj changed the title Add WASI check to Emscripten check Disable support for wasm32-wasi Jan 17, 2023
@gbj
Copy link
Contributor Author

gbj commented Feb 15, 2023

I think this should probably either be behind a feature, or is the wrong approach, due to the possible Node/Deno compatibility issues.

@daxpedda
Copy link
Collaborator

daxpedda commented May 10, 2023

I honestly don't know much about WASI, but it seems a bit counterintuitive to me to use WASI with Node.js or Deno if you intend to access JS, because WASI wouldn't offer anything in that case.
I think the intention of WASI support on these platforms is to support C/C++ that don't have a wasm-bindgen equivalent, or Rust for that matter that doesn't need JS access but now has an easy way to write cross-platform code.

If you want to use parts of the WASI API nevertheless, you can just import that through JS.

But yeah, input from somebody actually knowledgeable about this would be appreciated.

@daxpedda daxpedda added the help wanted We could use some help fixing this issue! label May 10, 2023
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to get this merged @gbj, got notified from #3421 (comment).

Would you mind rebasing and adding a comment to the changelog?

@daxpedda daxpedda added waiting for author Waiting for author to respond and removed help wanted We could use some help fixing this issue! labels Aug 4, 2023
@daxpedda daxpedda self-assigned this Aug 4, 2023
@daxpedda daxpedda merged commit cd04b27 into rustwasm:main Aug 7, 2023
23 checks passed
@daxpedda
Copy link
Collaborator

daxpedda commented Aug 7, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author Waiting for author to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants