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

fix: Use "exec" for cargo build scripts #3235

Conversation

mrmeku
Copy link

@mrmeku mrmeku commented Feb 3, 2025

The nuance between when to use exec vs target
is a bit tricky to get right sometimes. You want
to use "exec" for executables that are used at
build time and "target" for runtime dependencies

Since cargo build script are run at build time,
they need to be set to exec.

This fixes circumstances where a user is using
RBE on a platform different than their host machine

For example, I found this bug when I enabled my
linux based RBE cluster when trying to do a
bazel run of a rust binary on my mac. When the
build script executed it threw up because the RBE
cluster was using the mac version of the script.

The nuance between when to use exec vs target
is a bit tricky to get right sometimes. You want
to use "exec" for executables that are used at
build time and "target" for runtime dependencies

Since cargo build script are run at build time,
they need to be set to exec.

This fixes circumstances where a user is using
RBE on a platform different than their host machine

For example, I found this bug when I enabled my
linux based RBE cluster when trying to do a
bazel run of a rust binary on my mac. When the
build script executed it threw up because the RBE
cluster was using the mac version of the script.
@mrmeku mrmeku force-pushed the mrmeku-fix_use_exec_for_cargo_build_scripts branch from f6b8b0b to 671ec50 Compare February 3, 2025 18:55
@UebelAndre
Copy link
Collaborator

Thanks!

Isn't the build script binary already in exec? There is an intermediate rule called cargo_build_script_runfiles which consumes the rust_binary as exec:

"script": attr.label(
doc = "The binary script to run, generally a `rust_binary` target.",
executable = True,
mandatory = True,
providers = [rust_common.crate_info],
cfg = "exec",

And then this rule creates a symlink from that cfg = "exec" binary:

# Avoid the following issue on Windows when using builds-without-the-bytes.
# https://github.com/bazelbuild/bazel/issues/21747
if is_windows:
args = ctx.actions.args()
args.add(script)
args.add(exe)
ctx.actions.run(
executable = ctx.executable._copy_file,
arguments = [args],
inputs = [script],
outputs = [exe],
)
else:
ctx.actions.symlink(
output = exe,
target_file = script,
is_executable = True,
)

And returns it as the DefaultInfo.executable:

This way when cargo_build_script consumes the cargo_build_script_runfiles target, it is still running a binary built for cfg = "exec" even though this attribute consumes it as cfg = "target":

"script": attr.label(
doc = "The binary script to run, generally a `rust_binary` target.",
executable = True,
mandatory = True,
cfg = "target",
providers = [CargoBuildScriptRunfilesInfo],
),

Is there something broken about this interaction? I've also not had much success with cross platform RBE builds (bazelbuild/bazel#19587) but assuming the execution platform is described to be linux, in your case, I would expect all binaries to be built for the right platform. Curious to know how this wouldn't be the case and how to fix it.

@mrmeku
Copy link
Author

mrmeku commented Feb 3, 2025

Let me play with this a bit. I have a repo that has RBE so it'll be fast for me to iterate on potential fixes. I'll reply to this when I have more a confirmed fix since this is something that's especially hard to make a test case for with current infrastructure

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

@UebelAndre IIRC you put a lot of careful thought into the transitions here?

@mrmeku
Copy link
Author

mrmeku commented Feb 3, 2025

Okay, your analysis was right. I'm going to close this for now and reopen when I've resolved the error. There is still a bug (unless my RBE setup is at fault).

ERROR: /private/var/tmp/_bazel_mrmeku/f426e18877a0b787c58d9aeaf02e075f/external/rules_rust_tinyjson/BUILD.bazel:4:37: Compiling Rust (without process_wrapper) rlib tinyjson (4 files) [for tool] failed: (Exit 1): bootstrap_process_wrapper.sh failed: error executing Rustc command (from target @@rules_rust_tinyjson//:tinyjson) bazel-out/darwin_arm64-opt-exec-ST-4c0668394d32/bin/external/rules_rust/util/process_wrapper/bootstrap_process_wrapper.sh -- ... (remaining 21 arguments skipped)
bazel-out/darwin_arm64-opt-exec-ST-4c0668394d32/bin/external/rules_rust/util/process_wrapper/bootstrap_process_wrapper.sh: line 18: /private/var/tmp/_bazel_mrmeku/f426e18877a0b787c58d9aeaf02e075f/execroot/_main/bazel-out/darwin_arm64-opt-exec-ST-4c0668394d32/bin/external/rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools/rust_toolchain/bin/rustc: cannot execute binary file
bazel-out/darwin_arm64-opt-exec-ST-4c0668394d32/bin/external/rules_rust/util/process_wrapper/bootstrap_process_wrapper.sh: line 18: /private/var/tmp/_bazel_mrmeku/f426e18877a0b787c58d9aeaf02e075f/execroot/_main/bazel-out/darwin_arm64-opt-exec-ST-4c0668394d32/bin/external/rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools/rust_toolchain/bin/rustc: Undefined error: 0

@mrmeku mrmeku closed this Feb 3, 2025
@mrmeku
Copy link
Author

mrmeku commented Feb 3, 2025

The key to what I'm seeing is

bazel-out/darwin_arm64-opt-exec-ST-4c0668394d32/bin/external/rules_rust/util/process_wrapper/bootstrap_process_wrapper.sh: line 18: /private/var/tmp/_bazel_mrmeku/f426e18877a0b787c58d9aeaf02e075f/execroot/_main/bazel-out/darwin_arm64-opt-exec-ST-4c0668394d32/bin/external/rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools/rust_toolchain/bin/rustc: Undefined error: 0

The process wrapper is clearly mismatched to the rustc toolchain. Looking into why that is

@UebelAndre
Copy link
Collaborator

The key to what I'm seeing is

bazel-out/darwin_arm64-opt-exec-ST-4c0668394d32/bin/external/rules_rust/util/process_wrapper/bootstrap_process_wrapper.sh: line 18: /private/var/tmp/_bazel_mrmeku/f426e18877a0b787c58d9aeaf02e075f/execroot/_main/bazel-out/darwin_arm64-opt-exec-ST-4c0668394d32/bin/external/rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools/rust_toolchain/bin/rustc: Undefined error: 0

The process wrapper is clearly mismatched to the rustc toolchain. Looking into why that is

Very interesting. I can believe there’s a bug like this somewhere in the process wrapper bootstrapped. Do you know if toolchain resolutions matches what you expect? I’m wondering if there’s a mix up there or something started referring to the rust_host_tools repository inappropriately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants