Skip to content

Emit CrateInfo when building Wasm libraries as executables. #1315

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Emit CrateInfo when building Wasm libraries as executables. #1315

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bdccabd
Emit CrateInfo when building Wasm libraries as executables.
PiotrSikora May 3, 2022
161ed6c
Merge branch 'main' into fix_wasm2
PiotrSikora May 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rust/private/rustc.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -994,7 +994,7 @@ def rustc_compile_action(
),
]

if crate_info.type in ["staticlib", "cdylib"]:
if crate_info.type in ["staticlib", "cdylib"] and not out_binary:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you provide the exact error? Does it happen when you create other targets, eg rust_shared_library?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@PiotrSikora friendly ping 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@UebelAndre @scentini friendly ping, either for this or #1339, since it blocks Rust updates in Envoy and Proxy-Wasm.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

# These rules are not supposed to be depended on by other rust targets, and
# as such they shouldn't provide a CrateInfo. However, one may still want to
# write a rust_test for them, so we provide the CrateInfo wrapped in a provider
Expand Down