Add a build setting to disable the hash prefix on `rust_test` outputs. #1600

jmillikin · 2022-10-21T09:26:54Z

The hash prefix for test outputs was originally added to work around potential filename conflicts between rust_test and rust_binary on platforms without sandboxed builds (Windows).

However, on platforms that do have sandboxed builds, the prefix can be unwanted because it makes path of Rust test binaries different from what would be produced using other language rules such as cc_test.

This commit adds a Bazel build setting so that the output prefix can be disabled in cases where it's unwanted.

$ bazel build //test/rust:hello_lib_test
Target //test/rust:hello_lib_test up-to-date:
  bazel-bin/test/rust/test-365930506/hello_lib_test
$ bazel build --@rules_rust//rust/settings:rust_test_prefix_output_files=false //test/rust:hello_lib_test
Target //test/rust:hello_lib_test up-to-date:
  bazel-bin/test/rust/hello_lib_test

krasimirgg · 2022-11-02T15:42:26Z

This is nice! Just to make sure @jmillikin : is this just for consistency with cc_test on supported platforms, and you don't have a workflow that relies on the specifics of the output files layout?

jmillikin · 2022-11-03T11:05:09Z

A little of column A, and a little of column B.

I have a workflow for integration testing using cross-compiled test binaries running in QEMU. To support this without depending on the output file layout would require ctx.expand_location() to support generating "short" paths as used in runfiles, but Bazel doesn't support this[0]. Therefore I am hardcoding the output path of test binaries, which works because cc_test has predictable output paths.

Migrating these tests to Rust has required me to patch my local version of rules_rust in a couple ways, one of which is the removal of the test binary's hash prefix. This PR, if merged, would let that patch be dropped in favor of a .bazelrc option.

[0] That function has a short_paths= parameter, but Bazel considers it "Private API" and setting it to true raises an exception.

krasimirgg · 2022-12-12T15:09:32Z

could you refresh this PR so the buildkites get reran

krasimirgg · 2022-12-12T15:10:30Z

And could you add a test

jmillikin · 2023-02-03T04:36:26Z

@krasimirgg OK, I rebased on current main HEAD and added a test.

UebelAndre · 2023-04-11T13:25:40Z

If rust_binary and rust_test are conflicting, wouldn't it make sense to have the hash account for the name of the rule or something so they're always different?

jmillikin · 2023-04-11T14:08:03Z

If rust_binary and rust_test are conflicting, wouldn't it make sense to have the hash account for the name of the rule or something so they're always different?

The current behavior (introduced in PR #1434, which you reviewed) is for rust_test to have a hash. Per this PR's description, the hash exists as a workaround for non-sandboxed platforms (Windows).

This PR adds the option to remove the hash from outputs, because (again, per PR description) it is unnecessary on platforms with sandboxing and interferes with incremental migration from codebases in other languages (C++).

jmillikin · 2023-05-02T02:36:30Z

Rebased to current main HEAD. CI failures appear to be unrelated (caused by bazelci infra issues).

@UebelAndre , it's been about six months since I sent this PR in. Would it be possible to get it reviewed?

UebelAndre · 2023-05-02T02:50:16Z

@scentini @krasimirgg do you know why we have this hash output? Why can't rules_rust just write an output matching the name of the label?

UebelAndre

Sorry for the delay! Been a busy year!

rust/private/rust.bzl

test/test_prefix_setting/BUILD.bazel

jmillikin · 2023-05-02T03:29:07Z

@scentini @krasimirgg do you know why we have this hash output? Why can't rules_rust just write an output matching the name of the label?

There's a fairly comprehensive writeup in the PR I linked: #1434

UebelAndre · 2023-05-02T03:39:21Z

@scentini @krasimirgg do you know why we have this hash output? Why can't rules_rust just write an output matching the name of the label?

There's a fairly comprehensive writeup in the PR I linked: #1434

Thanks! I'll take a look in a but but in the mean time, could you add some documentation to the new flag that contains a writeup? I think the most ergonomic way to document this would be to use a custom rule instead of the bazel_skylib build settings.

jmillikin · 2023-05-02T03:48:58Z

I'm not sure I'm the correct person to write those docs because I don't use Windows, don't have any need for the hash prefix added by @scentini in that PR, and all I want to do here is have an escape hatch to turn it off. I would be perfectly happy with a PR that removed the whole thing.

Would copying chunks of the original PR into a docstring be sufficient?

UebelAndre · 2023-05-06T23:39:27Z

Would copying chunks of the original PR into a docstring be sufficient?

Yeah probably 😅. Anything we can do to communicate this clearly would be good. I know I've had folks ask me about this directly and it was definitely a challenge to remember and dig up. Having a breakdown in the repo would be awesome!

jmillikin · 2023-05-07T11:53:21Z

@UebelAndre I've added a file docs/rust_test_output_path_uniqueness.md that summarizes the issue and links to PR #1434 for folks who want to dig deeper.

The hash prefix for test outputs was originally added to work around potential filename conflicts between `rust_test` and `rust_binary` on platforms without sandboxed builds (Windows). However, on platforms that *do* have sandboxed builds, the prefix can be unwanted because it makes path of Rust test binaries different from what would be produced using other language rules such as `cc_test`. This commit adds a Bazel build setting so that the output prefix can be disabled in cases where it's unwanted.

jmillikin · 2023-05-11T02:38:12Z

Rebased to current main.

@UebelAndre gentle ping. Any remaining blockers?

UebelAndre · 2023-05-11T16:35:18Z

Hey! Thanks for the ping and sorry for the delay. I will take a look tonight and if you don’t see activity feel free to ping me again :)

UebelAndre

Digging into this closer, I don't actually see a reason why rust_binary and rust_test need to have hashes in the outputs of their names. Why not just delete this behavior and have them always write consistent outputs? In the links below we see the output is based on ctx.label.name which should always produce unique outputs, right?

https://github.com/bazelbuild/rules_rust/blob/0.21.1/rust/private/rust.bzl#L329
https://github.com/bazelbuild/rules_rust/blob/0.21.1/rust/private/rust.bzl#L385-L392
https://github.com/bazelbuild/rules_rust/blob/0.21.1/rust/private/rust.bzl#L431-L438

jmillikin · 2023-05-12T15:40:18Z

I believe that @scentini provided detailed steps to reproduce the issue in the original PR, which I linked above, and which you reviewed.

If you want to revert PR #1434 then that would solve my problem, but it would leave Windows users out in the cold, and you may want to consider carefully given that in #1427 (comment) you determined the inter-action file corruption to be a release blocker.

I don't have a Windows dev machine available at the moment, but I can verify that on Linux, with the hashes disabled, build actions for rust_test and rust_binary rules do generate temporary files with different content that are written to the same path. These are .d files -- presumably on Windows, the .o files get written to disk instead of piped.

rust_binary:

$ bazel build -s --@rules_rust//rust/settings:rust_test_prefix_output_files=false //test/unit/rustdoc:bin_with_transitive_proc_macro --sandbox_debug --spawn_strategy=sandboxed
[...]
1683905179.521713278: src/main/tools/linux-sandbox-pid1.cc:285: working dir: /home/john/.cache/bazel/_bazel_john/bfc02a8f286d82b1e327c569081a7f63/sandbox/linux-sandbox/13/execroot/rules_rust

$ cd /home/john/.cache/bazel/_bazel_john/bfc02a8f286d82b1e327c569081a7f63/sandbox/linux-sandbox/13/execroot/rules_rust
$ shasum bazel-out/k8-fastbuild/bin/test/unit/rustdoc/*.d
800703ec95caef394b403af3da96536ecee58a12  bazel-out/k8-fastbuild/bin/test/unit/rustdoc/bin_with_transitive_proc_macro.d

rust_test:

$ bazel build -s --@rules_rust//rust/settings:rust_test_prefix_output_files=false //test/unit/rustdoc:bin_with_transitive_proc_macro_test --sandbox_debug --spawn_strategy=sandboxed
[...]
1683905191.591318251: src/main/tools/linux-sandbox-pid1.cc:285: working dir: /home/john/.cache/bazel/_bazel_john/bfc02a8f286d82b1e327c569081a7f63/sandbox/linux-sandbox/14/execroot/rules_rust

$ cd /home/john/.cache/bazel/_bazel_john/bfc02a8f286d82b1e327c569081a7f63/sandbox/linux-sandbox/14/execroot/rules_rust
$ shasum bazel-out/k8-fastbuild/bin/test/unit/rustdoc/*.d
0e3bf6f7752fcdc643bd06a5106d5e4613c48214  bazel-out/k8-fastbuild/bin/test/unit/rustdoc/bin_with_transitive_proc_macro.d

UebelAndre · 2023-05-12T16:24:01Z

Instead of a feature that disables this, why not continue to build with hashes but for tests and binaries, use ctx.actions.symlink to create a file with a clean name? This way the *.o files won't stomp on each other and the executable has a predictable name?

jmillikin · 2023-05-12T16:40:17Z

It seems like keeping a perma-fork of rules_rust will be a more practical approach moving forward.

UebelAndre · 2023-05-12T17:02:22Z

It seems like keeping a perma-fork of rules_rust will be a more practical approach moving forward.

Sorry, It's definitely not my intention to string you or anyone else along in a PR. Coming into this I lacked both the time to properly review and history with the problem which makes the review stressful for me since I don't wanna have something merge that ends up causing more problems later, which unfortunately happens 😞.

I've opened #1968 since I think it would be a good change to have predictable outputs. Does that work for your use case?

UebelAndre · 2024-09-11T14:02:37Z

A similar change was implemented in #2828

scentini requested review from krasimirgg and removed request for krasimirgg November 2, 2022 09:58

jmillikin force-pushed the config-setting-test-prefixes branch 5 times, most recently from 1a95907 to 7476c49 Compare February 3, 2023 04:30

jmillikin force-pushed the config-setting-test-prefixes branch from 7476c49 to 3a70b90 Compare May 2, 2023 02:29

UebelAndre requested changes May 2, 2023

View reviewed changes

rust/private/rust.bzl Outdated Show resolved Hide resolved

test/test_prefix_setting/BUILD.bazel Outdated Show resolved Hide resolved

jmillikin force-pushed the config-setting-test-prefixes branch from 3a70b90 to aed941e Compare May 2, 2023 03:27

jmillikin force-pushed the config-setting-test-prefixes branch 2 times, most recently from 157cffa to 1034632 Compare May 2, 2023 03:34

jmillikin force-pushed the config-setting-test-prefixes branch 2 times, most recently from 20eb8c0 to 53aab35 Compare May 7, 2023 11:47

jmillikin added 2 commits May 11, 2023 11:14

Test for @rules_rust//rust/settings:rust_test_prefix_output_files.

7207aff

Add documentation about why rust_test output prefixing exists.

8d16d34

jmillikin force-pushed the config-setting-test-prefixes branch from 53aab35 to 8d16d34 Compare May 11, 2023 02:14

UebelAndre requested changes May 12, 2023

View reviewed changes

jmillikin closed this May 12, 2023

jmillikin deleted the config-setting-test-prefixes branch May 12, 2023 16:40

UebelAndre mentioned this pull request May 12, 2023

Update rust_test to produce outputs with predictable names. #1968

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a build setting to disable the hash prefix on `rust_test` outputs. #1600

Add a build setting to disable the hash prefix on `rust_test` outputs. #1600

jmillikin commented Oct 21, 2022 •

edited

Loading

krasimirgg commented Nov 2, 2022

jmillikin commented Nov 3, 2022 •

edited

Loading

krasimirgg commented Dec 12, 2022 •

edited

Loading

krasimirgg commented Dec 12, 2022

jmillikin commented Feb 3, 2023

UebelAndre commented Apr 11, 2023

jmillikin commented Apr 11, 2023

jmillikin commented May 2, 2023

UebelAndre commented May 2, 2023

UebelAndre left a comment

jmillikin commented May 2, 2023

UebelAndre commented May 2, 2023

jmillikin commented May 2, 2023

UebelAndre commented May 6, 2023

jmillikin commented May 7, 2023

jmillikin commented May 11, 2023

UebelAndre commented May 11, 2023

UebelAndre left a comment

jmillikin commented May 12, 2023

UebelAndre commented May 12, 2023

jmillikin commented May 12, 2023

UebelAndre commented May 12, 2023

UebelAndre commented Sep 11, 2024

Add a build setting to disable the hash prefix on rust_test outputs. #1600

Add a build setting to disable the hash prefix on rust_test outputs. #1600

Conversation

jmillikin commented Oct 21, 2022 • edited Loading

krasimirgg commented Nov 2, 2022

jmillikin commented Nov 3, 2022 • edited Loading

krasimirgg commented Dec 12, 2022 • edited Loading

krasimirgg commented Dec 12, 2022

jmillikin commented Feb 3, 2023

UebelAndre commented Apr 11, 2023

jmillikin commented Apr 11, 2023

jmillikin commented May 2, 2023

UebelAndre commented May 2, 2023

UebelAndre left a comment

Choose a reason for hiding this comment

jmillikin commented May 2, 2023

UebelAndre commented May 2, 2023

jmillikin commented May 2, 2023

UebelAndre commented May 6, 2023

jmillikin commented May 7, 2023

jmillikin commented May 11, 2023

UebelAndre commented May 11, 2023

UebelAndre left a comment

Choose a reason for hiding this comment

jmillikin commented May 12, 2023

UebelAndre commented May 12, 2023

jmillikin commented May 12, 2023

UebelAndre commented May 12, 2023

UebelAndre commented Sep 11, 2024

Add a build setting to disable the hash prefix on `rust_test` outputs. #1600

Add a build setting to disable the hash prefix on `rust_test` outputs. #1600

jmillikin commented Oct 21, 2022 •

edited

Loading

jmillikin commented Nov 3, 2022 •

edited

Loading

krasimirgg commented Dec 12, 2022 •

edited

Loading