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: rust_doc_test failure to find params file #1418

Merged
merged 4 commits into from
Aug 1, 2022

Conversation

ParkMyCar
Copy link
Contributor

@ParkMyCar ParkMyCar commented Jun 23, 2022

Context
Today rust_doc_test fails when Bazel Args "spill" over to a file. To a user this failure is seemingly random because Bazel will auto-magically spill Args onto a file when there are too many args for the command line, or when it can improve performance. The generated test script, e.g. hellolib.rustdoc_test.sh, is then run from runfiles, which is separate from the Arg file Bazel created.

It's especially tricky because the amount of command line args Windows supports is < macOS < Linux, so there's a "silent" OS dependency here too.

Solution
This PR fixes the issue by manually declaring a params file, e.g. hellolib.rustdoc_opt_params, that is a sibling file to our test runner, hellolib.rustdoc_test.sh. We then pass the path of this optional params file to our test writer. The test writer checks for the presence of the Bazel Args file, and if it finds one, copies the content into our manually declared params file.

Our manually declared params file then gets moved into runfiles with our test script. The test script can then find the file, and rustdoc picks up the args.

Note: Today we detect the params file by matching a prefix of @ and suffix of .rustdoc_test.sh-0.params. I'm not sure if this is accurate for all systems, or all versions of bazel. I tried the use_param_file option on Args to make it easier to detect a params file, but that seem to overwrite (or just generally effect) the other arguments we'd pass to rustdoc_test_writer.rs.

Fixes #1233

@google-cla
Copy link

google-cla bot commented Jun 23, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Comment on lines +22 to +26
#[clap(
name = "cargo-bazel",
about = "crate_universe` is a collection of tools which use Cargo to generate build targets for Bazel.",
version
)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this change, the the other changes to #[clap(author ... are not relevant to the underlying fix, but the changes were necessary to get the rust_doc_test test passing on Windows

ParkMyCar pushed a commit to dropbox/rules_rust that referenced this pull request Jun 23, 2022
@ParkMyCar
Copy link
Contributor Author

Turns out this fix isn't perfect, we still hit character limits on Windows. Specifically if the crate you're adding a doc test for has a large dependency tree you can hit command is longer than CreateProcessW's limit (32767 characters). I thought maybe separating the args in the .bat or .sh file by newlines would help but I don't think it did

@illicitonion
Copy link
Collaborator

Thanks for putting this together!

It may be worth taking a look at #1076 for inspiration in terms of the limits - that PR ended up working out how to always use param files and I think mostly get things working out including for very long arg lists.

/cc @ddeville

@ParkMyCar
Copy link
Contributor Author

Bump on this @illicitonion, I updated the PR if you have a chance to review :)

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.

LGTM - thanks for the contribution!

Just one naming/commenting comment :)

Comment on lines 23 to 24
/// The path where we can copy the params file Bazel might generate
optional_params_file: PathBuf,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// The path where we can copy the params file Bazel might generate
optional_params_file: PathBuf,
/// If Bazel generated a params file, we may need to strip roots from it.
/// This is the path where we will output our stripped params file.
optional_output_params_file: PathBuf,

* Declares our own params file, that we copy the Bazel Args into when necessary
* Updates crate_universe and runs the rust_doc_test tests
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.

Thanks!

@illicitonion illicitonion merged commit 67e204f into bazelbuild:main Aug 1, 2022
@ParkMyCar
Copy link
Contributor Author

Thanks for merging!

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.

rust_doc_test failing with "Failed to load argument file"
2 participants