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

feature: allow wasm-bindgen to work on emscripten targets (INCOMPLETE/RFC) #4014

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

durin42
Copy link

@durin42 durin42 commented Jul 16, 2024

We've got this wired up on our internal build system and wanted to try and get feedback before we get further. With our internal build system we've got this working end-to-end, and we're happy to do work here to make sure the right things happen with cargo including for cargo test. The question right now is: are y'all open to this landing if we're doing the work, or is this completely out of scope for what you want in the project?

(I've seen some other conversations about this in old bugs, but don't know the current direction of the project.)

Thanks!

@durin42
Copy link
Author

durin42 commented Jul 23, 2024

@daxpedda could you let me know if this is a viable direction, or something we wouldn't be able to land? Like I said, we're happy to do the work, but don't want to spend the time if emscripten support will be rejected.

@daxpedda
Copy link
Collaborator

Similar things have been discussed about WASI here as well, see #3454.

In general I'm not against Emscripten support, but I'm not particularly in favor of it either. My understanding is that it should be possible to stick with wasm32-unknown-unknown and consume Emscripten dependencies (e.g. C/C++). Anything that helps that along I'm happy to improve on. But this isn't a reason to block support, just explains the lack of driving force behind it.

On the other hand I have so little knowledge about Emscripten that I honestly would be unable to form a qualified opinion here and I'm not interested in diving into it. So this needs a different maintainer!

@daxpedda daxpedda added the needs discussion Requires further discussion label Jul 23, 2024
@walkingeyerobot
Copy link

Hi! I’m an Emscripten contributor and a co-worker of @durin42’s, and we have a strong need to get wasm-bindgen and Emscripten to play nicely together. More specifically:

  • We have Wasm applications primarily written in C++.
  • These need to be able to depend on Rust libraries.
  • These applications use Emscripten bindings to talk to JS.
  • Third party Rust libraries included by these applications want to directly use JS independently of how it’s being used by the larger application. This is the reason we aren’t just running everything through C++.

My tentative plan is to have Emscripten invoke wasm-bindgen and then incorporate wasm-bindgen’s JS into the Emscripten JS. This will require wasm-bindgen to:

  1. Accept Wasm that has targeted wasm32-unknown-emscripten (which is what this PR does)
  2. Have wasm-bindgen emit an Emscripten-style JS library for direct integration into the rest of Emscripten’s JS. This would be a new option on wasm-bindgen’s existing --target flag.

This will also require a plethora of Emscripten changes. I’ve spoken with Emscripten owners, and they’re on board with this plan. I plan to make changes to both projects in branches, and once I have something that at least partially works, I can open some draft PRs to gather feedback. That also means we don’t immediately need to have this merged if you prefer; we can wait until we’re a bit further along.

@daxpedda
Copy link
Collaborator

That also means we don’t immediately need to have this merged if you prefer; we can wait until we’re a bit further along.

Especially for adding a new target, waiting until there is some consensus is very desirable.


Otherwise my last comment (#4014 (comment)) still reflects the current stance.

@walkingeyerobot
Copy link

Sounds good, I'm happy to gather consensus. I'll start putting together a proof of concept in the next few weeks so we have something more tangible to discuss.

On the other hand I have so little knowledge about Emscripten that I honestly would be unable to form a qualified opinion here and I'm not interested in diving into it. So this needs a different maintainer!

To clarify, are you looking for another current maintainer to lead the discussion here? Or are you looking for me to commit to maintaining this feature long-term after it's written? If the latter, I'm happy to agree to that. I'll be sure to have good test coverage and make myself available to address any issues / PRs / etc. that pop up down the road.

@daxpedda
Copy link
Collaborator

daxpedda commented Oct 1, 2024

To clarify, are you looking for another current maintainer to lead the discussion here? Or are you looking for me to commit to maintaining this feature long-term after it's written?

I was looking for another current maintainer to take over here, but I'm very happy to accept target-specific maintainers as well.

If the latter, I'm happy to agree to that. I'll be sure to have good test coverage and make myself available to address any issues / PRs / etc. that pop up down the road.

That would be great!

@NickUfer
Copy link

@walkingeyerobot I am currently implementing rust bindings for the C++ library https://github.com/elalish/manifold and people are showing interest using this in a browser via WASM.
In the first message of this PR your colleague mentioned that you guys got it all wired up and working... is there any way to test this? Is there a public branch with minimal instructions somewhere?

@walkingeyerobot
Copy link

We got it working internally with A LOT of hacks. I've been attempting to create a much more sturdy demo that works externally here: https://github.com/walkingeyerobot/cxx-rust-demo. This is still very early stages, but I hope to have more time to poke at this later in the week.

@NickUfer
Copy link

NickUfer commented Nov 20, 2024

So far in my tests I had the problem that I was unable to see any exported functions, because of the cfg macro config this PR removes. When I use this PR's wasm-bindgen version I get function exports:

Cargo.toml

[patch.crates-io]
wasm-bindgen = { git = "https://github.com/rustwasm/wasm-bindgen", package = "wasm-bindgen", rev = "b73711994022f794fd6c930ba3dabc4ecd7614ff" }
#[wasm_bindgen(js_name = newTetrahedron)]
pub fn new_tetrahedron() -> bool {
    true
}
➜  debug git:(main) ✗ wasm-objdump -j export manifold3d.wasm -x

manifold3d.wasm:        file format wasm 0x1
module name: <manifold3d.wasm>

Section Details:

Export[1860]:
 - memory[0] -> "memory"
 - table[0] -> "__indirect_function_table"
 - func[37] <newTetrahedron> -> "newTetrahedron"
 [...]

So far so good, now I'm stuck at the problem that wasm-bindgen throws errors:

➜  debug git:(main) ✗ RUST_BACKTRACE=1 wasm-bindgen --target web manifold3d.wasm --out-dir out
thread 'main' panicked at crates/wasm-interpreter/src/lib.rs:220:18:
can only call locally defined functions
stack backtrace:
  0: std::panicking::begin_panic
  1: wasm_bindgen_wasm_interpreter::Interpreter::call
  2: wasm_bindgen_wasm_interpreter::Interpreter::call
  3: wasm_bindgen_wasm_interpreter::Interpreter::interpret_descriptor
  4: wasm_bindgen_cli_support::descriptors::execute
  5: wasm_bindgen_cli_support::Bindgen::generate_output
  6: wasm_bindgen_cli_support::Bindgen::generate
  7: wasm_bindgen::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

From the sound of it it may got some problems with the exported functions from the C++ library

Edit: log with debug enabled:

➜  debug git:(main) ✗ RUST_LOG=debug RUST_BACKTRACE=1 wasm-bindgen --target web manifold3d.wasm --out-dir out
[...]
[2024-11-20T01:53:29Z DEBUG wasm_bindgen_cli_support::wit] found version specifier {"schema_version":"0.2.93","version":"0.2.92 (b73711994)"}
[2024-11-20T01:53:29Z DEBUG wasm_bindgen_cli_support::wit] found a program of length 20887
[2024-11-20T01:53:29Z DEBUG wasm_bindgen_wasm_interpreter] starting a call of Id { idx: 38 } Some("__wbindgen_describe_newTetrahedron")
[2024-11-20T01:53:29Z DEBUG wasm_bindgen_wasm_interpreter] arguments []
[2024-11-20T01:53:29Z DEBUG wasm_bindgen_wasm_interpreter] starting a call of Id { idx: 11 } Some("invoke_v")
[2024-11-20T01:53:29Z DEBUG wasm_bindgen_wasm_interpreter] arguments [32752]
thread 'main' panicked at crates/wasm-interpreter/src/lib.rs:220:18:
can only call locally defined functions
stack backtrace:
   0: std::panicking::begin_panic
   1: wasm_bindgen_wasm_interpreter::Interpreter::call
   2: wasm_bindgen_wasm_interpreter::Interpreter::call
   3: wasm_bindgen_wasm_interpreter::Interpreter::interpret_descriptor
   4: wasm_bindgen_cli_support::descriptors::execute
   5: wasm_bindgen_cli_support::Bindgen::generate_output
   6: wasm_bindgen_cli_support::Bindgen::generate
   7: wasm_bindgen::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@sbc100
Copy link

sbc100 commented Nov 21, 2024

Once this lands I think we can add some tests to the emscripten CI (Following up on emscripten-core/emscripten#22964).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Requires further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants