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

Added documentation to rust_binary::out_binary #772

Merged
merged 5 commits into from
Jul 15, 2021

Conversation

UebelAndre
Copy link
Collaborator

@UebelAndre UebelAndre commented Jun 17, 2021

closes #604

The description here still feels generic and I'm not really sure how best to communicate what this is for and what it is without just linking it's history. I made #771 since it sounds like a better solution to what seems like an ambiguous issue.

@google-cla google-cla bot added the cla: yes label Jun 17, 2021
@hlopko hlopko requested a review from dfreese June 22, 2021 07:51
@hlopko
Copy link
Member

hlopko commented Jun 22, 2021

As I remember it, this attribute a temporary bandaid to make some part of wasm build and it will eventually be removed. But I think we shouldn't trust me here, and @dfreese will know more :)

My questions:

  • do we need to document the attribute at all?
  • should we mark it as experimental and tell people not to use it?
  • is presence of this attribute blocking any progress, or making any progress harder? Will creating a rust_wasm_binary pay for itself if the only difference is this attribute?

@UebelAndre
Copy link
Collaborator Author

* do we need to document the attribute at all?

All user facing attributes should be documented. I don't think there should be any exception.

* should we mark it as experimental and tell people not to use it?

That's a definite possibility depending on what it's really used for (which is still kinda mysterious to me)

* is presence of this attribute blocking any progress, or making any progress harder? Will creating a `rust_wasm_binary` pay for itself if the only difference is this attribute?

I don't think it's blocking any progress but it's a mysterious bit of functionality that may or may not be what someone needs. I care most that this attribute is identified and that there's a plan for it. Either it's something that's going to be removed and we document it as deprecated with some kind of issue link or it's actually removed or replaced by something we think fits better, like rust_wasm_binary (which I'm not confident in as a solution but seemed to pop up in more than one place so I wanted to track it as a potential solution).

I would love some input on what this is, what it should be, and what to write here. But again, I do think something should be written.

Copy link
Contributor

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

This is a hack that we use to force crate_type=cdylib to be built as an executable when targeting Wasm platforms.

crate_type=cdylib is needed because we're building Wasm reactors (without main() function), and Rust toolchain complains when we're building a binary without it. This is fixed on nightly with RUSTFLAGS="-Z wasi-exec-model=reactor", but it's going to be a while before it's stabilized.

out_binary=True is needed because we need to mark the output as executable, otherwise Bazel complains that it doesn't have any of the allowed extensions for a library:

ERROR: .../envoy/test/extensions/filters/http/wasm/test_data/BUILD:89:17: in rust_binary rule //test/extensions/filters/http/wasm/test_data:_wasm_resume_call_rust_wasm: 
Traceback (most recent call last):
        File ".../external/rules_rust/rust/private/rust.bzl", line 283, column 32, in _rust_binary_impl
                return rustc_compile_action(
        File ".../external/rules_rust/rust/private/rustc.bzl", line 583, column 29, in rustc_compile_action
                return establish_cc_info(ctx, crate_info, toolchain, cc_toolchain, feature_configuration) + [
        File ".../external/rules_rust/rust/private/rustc.bzl", line 640, column 59, in establish_cc_info
                library_to_link = cc_common.create_library_to_link(
Error in create_library_to_link: '_wasm_resume_call_rust_wasm.wasm' does not have any of the allowed extensions .so, .dylib or .dll
ERROR: Analysis of target '//test/extensions/filters/http/wasm:wasm_filter_test' failed; build aborted: Analysis of target '//test/extensions/filters/http/wasm/test_data:_wasm_resume_call_rust_wasm' failed

This could be removed if we had rust_wasm_binary.

rust/private/rust.bzl Outdated Show resolved Hide resolved
Copy link
Member

@hlopko hlopko left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@UebelAndre UebelAndre merged commit a814d85 into bazelbuild:main Jul 15, 2021
@UebelAndre UebelAndre deleted the out branch July 20, 2021 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What is rust_binary::out_binary?
3 participants