-
Notifications
You must be signed in to change notification settings - Fork 530
Add support for wasm-bindgen. #240
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
Conversation
acmcarther
left a comment
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 a pretty significant change that cuts across multiple areas of expertise for existing maintainers. Thanks for putting this together!
I'll take an initial look mostly at the toolchain stuff. I'm going to ask a lot of dumb questions, because I don't know much about web assembly.
johnedmonds
left a comment
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 taking a look!
acmcarther
left a comment
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.
Two small additional comments. I'll wait until your revision (per comment on the hybrid output platform usecase aka Bazel transition), or a ping on the PR.
0ebbe86 to
ef0bf46
Compare
|
This is a great PR. Thanks for the work. @acmcarther @johnedmonds : Joining your discussions about building for multiple platforms at the same time. The Our team have an almost identical situation as @johnedmonds . We'd like to use rust-wasm as well. Looking forward to seeing this PR land. |
|
I do have a very small point to bring up. I agree with @0xADD1E mentioned in #186,
I'd like to grab the raw The rationale being that the That said, I think a reasonable tradeoff is to have a place in this PR for us to easily swap in our own vendored What do you think? |
|
Thanks for the kind comments @qzmfranklin. With regards to this:
I think you can already do that with this PR by using |
|
@johnedmonds Thanks for the informative reply! I'll definitely try it out before too long, pending other priorities. BTW, I really like the way this PR landed the To truly solve the cross-compilation problem in rules_rust, I'd believe an overhaul is probably necessary. |
|
|
||
| #### WebAssembly | ||
|
|
||
| To build a `rust_binary` for wasm32-unknown-unknown add the `--platforms=//rust/platform:wasm` flag. |
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 a great start, but I think that the requirement to set target platform is quite limiting.
WebAssembly is used outside of browsers / JavaScript host environments, and Wasm modules can be loaded by other build targets in the same Bazel workspace, and those build targets must be built using "native" platforms and not the //rust:platform:wasm pseudo-platform.
For example, imagine such BUILD file:
cc_library(
name = "runtime",
srcs = "runtime.cc",
data = [
":wasm",
],
)
rust_library(
name = "wasm",
srcs = ["wasm.rs"],
edition = "2018",
crate_type = "cdylib",
rustc_flags = ["--target=wasm32-unknown-unknown"],
)
where Wasm module compiled using rust_library must be loaded by the runtime written in C/C++.
The ability to override --target is something that I have hacked in my local tree, but ideally we would have something like rust_wasm_library that uses --crate-type=cdylib --target=wasm32-unknown-unknown to compile Wasm modules.
Basically, we need the ability to indicate Wasm target on a per build target basis, and not globally.
What do you think?
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.
@PiotrSikora Thanks for the comment! I think I had a similar problem where I wanted to include the compiled wasm (compiled as wasm32-unknown-unknown) with include_bytes and serve it from the binary (compiled as a native Rust binary). We allow it in this PR by using Bazel transitions. See https://github.com/bazelbuild/rules_rust/pull/240/files#diff-9a42a2c125ed40c511198be1665d0faeR346. This allows us to build the entire build with the default target platform (e.g. x86 + Linux) but temporarily transition to //rust:platform:wasm when building the wasm code. We also handle transitioning to the host platform when building proc-macro crates.
The easiest way is to do this in practice is to make a rust_wasm_bindgen target which will force the transition for the dependent rust_library (and transitively for all its dependencies). So I think what you want to do is this:
cc_library(
name = "runtime",
srcs = "runtime.cc",
data = [
":wasm_bindgen_bg.wasm",
],
)
rust_wasm_bindgen(
name = "wasm_bindgen",
wasm_file = ":wasm"
)
rust_library(
name = "wasm",
srcs = ["wasm.rs"],
edition = "2018",
)
With this you should be able to build :runtime as a native library and :wasm will automatically get built as //rust:platform:wasm.
I have a note about this here https://github.com/bazelbuild/rules_rust/pull/240/files/7305a12fd276b0c21432d00d4e2ddd8b15115e13#diff-04c6e90faac2675aa89e2176d2eec7d8R31. As I read my note again I realize that perhaps it's a bit unclear since "transition" is a Bazel-specific word. Do you think it would be clearer if I referenced it in this paragraph? Or did I misunderstand your usecase?
Basically in this paragraph I was trying to explain how you could get the .wasm file if that's the only thing you wanted.
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, the transitions are great, I didn't realize that you could do it this way.
I think that rust_wasm_bindgen is close to what I want, but after reading that paragraph, I simply ignored it, because I don't want anything related to wasm-bindgen, I want pure Rust code that's compiled to wasm32-unknown-unknown.
As such, I'm suggesting that we would add rust_wasm_library that uses the transitions that you've already added for rust_wasm_bindgen, i.e.
cc_library(
name = "runtime",
srcs = "runtime.cc",
data = [
":wasm",
],
)
rust_wasm_library(
name = "wasm",
deps = ":library"
)
rust_library(
name = "library",
srcs = ["library.rs"],
edition = "2018",
)
Alternatively, since you're already doing it for proc-macro, I think that we could use crate_type = "wasm32" and simply transition to wasm32-unknown-unknown in the rust_library, i.e.
cc_library(
name = "runtime",
srcs = "runtime.cc",
data = [
":library",
],
)
rust_library(
name = "library",
srcs = ["library.rs"],
crate_type = "wasm32",
edition = "2018",
)
What do you think?
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.
Friendly ping before this gets merged.
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.
Do you think we should do it in a different PR? There are several ways to do this like:
- The method you proposed of introducing a rule to force a transition
- Adding a setting to the
rust_libraryto enable the transition - Change Bazel to make it easier to do transitions on demand.
- Existing workaround via
rust_wasm_bindgen.
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.
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'm definitely against (4), since this feature has nothing to do with wasm-bindgen.
(3) would still require action from the person building the target (vs person writing BUILD file), so it's not much different than requiring extra --platforms=wasm32 flag, IMHO.
I'm fine with either (1) or (2), with a slight preference for (1), since then you could easily add a target that extends existing rust_library to be also compiled as Wasm module (i.e. you could have both native and Wasm build targets built at the same time) by simply adding few lines with rust_wasm_library (see the example above). You could achieve the same with (2), but then you'd need to duplicate the complete build target, so it would be more error prone, I think.
Also, there is already a precedence for rust_xxx_library with rust_proto_library and rust_grpc_library.
It's fine if you want to add it in a separate PR (i.e. I don't want to block this PR from getting merged), but could you rename it to "Add support for wasm-bindgen", since it's not a complete WebAssembly support?
What do you think?
|
@acmcarther Could you take another look? |
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 including support for replacing the prepackaged wasm_bindgen!
One question on the special casing of wasm32-*-* in toolchain loading, otherwise LGTM.
EDIT: Also, apologies for the delay!
|
@johnedmonds : You might've already known about this. Just want to bring it to your attention. Bazel's support for Android involves the use of In short, the I had another comment about the relatively recent (about a month) redesign of Above and again, this is a great PR. Clear goals, pragmatic approaches, aggressive integrations. Would love to see it merge. |
|
@acmcarther Would you mind merging? I don't have permission to do the merge. |
|
Oops, my bad, done now. |
A few things to note:
.wasmfile (similar to how_deploy.jarworks). I chose to do it this way mainly because bazel doesn't yet support multiple target configs. Our use case is mainly being able to use a single bazel build command to generate a docker image containing the server binary and the client code. And also to let us run all the tests together. If you prefer though we could try changing wasm so it would be more like a traditional platform.