Repository navigation
Emit CrateInfo when building Wasm libraries as executables. - #1315
PiotrSikora wants to merge 2 commits into
Conversation
Broken in bazelbuild#1298. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
| ] | ||
|
|
||
| if crate_info.type in ["staticlib", "cdylib"]: | ||
| if crate_info.type in ["staticlib", "cdylib"] and not out_binary: |
There was a problem hiding this comment.
I'm really not a fan of this attribute. Isn't #771 something that would solve for this? Perhaps I'm still missing some context but the attribute feels like a toggle for some super niche functionality that alone adds unaccounted edge cases but is not currently an issue because there's a small group of users who use this and all do the same thing. I'd love to either have my understanding of this problem space be updated or delete the thing that seems like a hole we punched in the rules to support a few things.
There was a problem hiding this comment.
AFAIK, the only use case for out_binary is building Wasm modules, so #771 would definitely fix it, but I don't have bandwidth to implement rust_wasm_{binary,library} myself, so a quick fix it is.
There was a problem hiding this comment.
I just learned about this attribute and I too am unhappy about it. @UebelAndre do we have a maintainer who supports wasm builds? While I don't want to block this PR on it, I think we should either have maintainers/contributors that can pick up #771 and clean up the codebase or discuss whether this is something that we want to support.
There was a problem hiding this comment.
To my knowledge there isn't a maintainer who supports wasm builds. Unless we can find a consumer of this functionality that's willing to cleanly implement the rules and add test then I don't think we can officially say we support this. I'd be reluctantly ok with letting this one through but can't say I'm currently capable of understanding whether or not future changes will cause regressions.
There was a problem hiding this comment.
Could we get this in? It looks that v0.4.0 was shipped without the fix.
I agree that we should have rust_wasm_{binary,library} (and I advocated for this on the PR that added out_binary), at which point we could remove out_binary, but since it's been added and worked for the past few years, breaking it when the fix is so trivial doesn't seem very productive.
There was a problem hiding this comment.
Could you provide the exact error? Does it happen when you create other targets, eg rust_shared_library?
There was a problem hiding this comment.
ERROR: .../rules_rust/test/test_env/BUILD.bazel:4:12: While resolving toolchains for target //test/test_env:hello-world: No matching toolchains found for types @bazel_tools//tools/cpp:toolchain_type. Maybe --
incompatible_use_cc_configure_from_rules_cc has been flipped and there is no default C++ toolchain added in the WORKSPACE file? See https://github.com/bazelbuild/bazel/issues/10134 for details and migration instructions.
It happens only for rust_binary(crate_type = "cdylib", out_binary = True).
There was a problem hiding this comment.
@UebelAndre @scentini friendly ping, either for this or #1339, since it blocks Rust updates in Envoy and Proxy-Wasm.
There was a problem hiding this comment.
Hey @PiotrSikora, I don't believe I'll have time in the near future to explore how rust+wasm builds work. Your best bet would be to either explore how the envoy rules work so that you can provide testing for this use case, or find someone willing to do so. In the meantime I consider this use case unsupported.
Re #1339 - did you confirm that it fixes your issue? That one I can work on soon.
scentini
left a comment
scentini
left a comment
There was a problem hiding this comment.
Could you please add tests for this?
| ] | ||
|
|
||
| if crate_info.type in ["staticlib", "cdylib"]: | ||
| if crate_info.type in ["staticlib", "cdylib"] and not out_binary: |
There was a problem hiding this comment.
I just learned about this attribute and I too am unhappy about it. @UebelAndre do we have a maintainer who supports wasm builds? While I don't want to block this PR on it, I think we should either have maintainers/contributors that can pick up #771 and clean up the codebase or discuss whether this is something that we want to support.
3 participants
Broken in #1298.
Fixes #1330.
Signed-off-by: Piotr Sikora piotrsikora@google.com