-
Notifications
You must be signed in to change notification settings - Fork 435
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
Add support for wasm-bindgen. #240
Conversation
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.
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!
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. |
@@ -22,6 +22,15 @@ This repository provides rules for building [Rust][rust] projects with [Bazel](h | |||
</ul> | |||
</div> | |||
|
|||
#### 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_library
to 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:
.wasm
file (similar to how_deploy.jar
works). 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.