fix: `rust_doc_test` failure to find params file (#1418)
**Context**
Today `rust_doc_test` fails when Bazel [Args](https://docs.bazel.build/versions/main/skylark/lib/Args.html) "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`](https://docs.bazel.build/versions/main/skylark/lib/Args.html#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
diff --git a/crate_universe/BUILD.bazel b/crate_universe/BUILD.bazel
index 203a4d2..10ad57c 100644
--- a/crate_universe/BUILD.bazel
+++ b/crate_universe/BUILD.bazel
@@ -124,11 +124,4 @@
rust_doc_test(
name = "rustdoc_test",
crate = ":cargo_bazel",
- # TODO: This target fails on some platforms with the error tracked in:
- # https://github.com/bazelbuild/rules_rust/issues/1233
- target_compatible_with = select({
- "@platforms//os:macos": ["@platforms//:incompatible"],
- "@platforms//os:windows": ["@platforms//:incompatible"],
- "//conditions:default": [],
- }),
)
diff --git a/crate_universe/src/cli.rs b/crate_universe/src/cli.rs
index 2ed27ac..fdff844 100644
--- a/crate_universe/src/cli.rs
+++ b/crate_universe/src/cli.rs
@@ -19,7 +19,11 @@
pub use vendor::vendor;
#[derive(Parser, Debug)]
-#[clap(name = "cargo-bazel", about, version)]
+#[clap(
+ name = "cargo-bazel",
+ about = "crate_universe` is a collection of tools which use Cargo to generate build targets for Bazel.",
+ version
+)]
pub enum Options {
/// Generate Bazel Build files from a Cargo manifest.
Generate(GenerateOptions),
diff --git a/crate_universe/src/cli/generate.rs b/crate_universe/src/cli/generate.rs
index cf5e770..3fdc97e 100644
--- a/crate_universe/src/cli/generate.rs
+++ b/crate_universe/src/cli/generate.rs
@@ -16,7 +16,7 @@
/// Command line options for the `generate` subcommand
#[derive(Parser, Debug)]
-#[clap(about, version)]
+#[clap(about = "Command line options for the `generate` subcommand", version)]
pub struct GenerateOptions {
/// The path to a Cargo binary to use for gathering metadata
#[clap(long, env = "CARGO")]
diff --git a/crate_universe/src/cli/query.rs b/crate_universe/src/cli/query.rs
index 668f64f..19087ab 100644
--- a/crate_universe/src/cli/query.rs
+++ b/crate_universe/src/cli/query.rs
@@ -13,7 +13,7 @@
/// Command line options for the `query` subcommand
#[derive(Parser, Debug)]
-#[clap(about, version)]
+#[clap(about = "Command line options for the `query` subcommand", version)]
pub struct QueryOptions {
/// The lockfile path for reproducible Cargo->Bazel renderings
#[clap(long)]
diff --git a/crate_universe/src/cli/splice.rs b/crate_universe/src/cli/splice.rs
index f5077bf..213ee34 100644
--- a/crate_universe/src/cli/splice.rs
+++ b/crate_universe/src/cli/splice.rs
@@ -11,7 +11,7 @@
/// Command line options for the `splice` subcommand
#[derive(Parser, Debug)]
-#[clap(about, version)]
+#[clap(about = "Command line options for the `splice` subcommand", version)]
pub struct SpliceOptions {
/// A generated manifest of splicing inputs
#[clap(long)]
diff --git a/crate_universe/src/cli/vendor.rs b/crate_universe/src/cli/vendor.rs
index 366bb68..0b90541 100644
--- a/crate_universe/src/cli/vendor.rs
+++ b/crate_universe/src/cli/vendor.rs
@@ -19,7 +19,7 @@
/// Command line options for the `vendor` subcommand
#[derive(Parser, Debug)]
-#[clap(about, version)]
+#[clap(about = "Command line options for the `vendor` subcommand", version)]
pub struct VendorOptions {
/// The path to a Cargo binary to use for gathering metadata
#[clap(long, env = "CARGO")]
diff --git a/rust/private/rustdoc_test.bzl b/rust/private/rustdoc_test.bzl
index c2425b0..a2ee98c 100644
--- a/rust/private/rustdoc_test.bzl
+++ b/rust/private/rustdoc_test.bzl
@@ -19,7 +19,7 @@
load("//rust/private:rustdoc.bzl", "rustdoc_compile_action")
load("//rust/private:utils.bzl", "dedent", "find_toolchain", "transform_deps")
-def _construct_writer_arguments(ctx, test_runner, action, crate_info):
+def _construct_writer_arguments(ctx, test_runner, opt_test_params, action, crate_info):
"""Construct arguments and environment variables specific to `rustdoc_test_writer`.
This is largely solving for the fact that tests run from a runfiles directory
@@ -29,6 +29,7 @@
Args:
ctx (ctx): The rule's context object.
test_runner (File): The test_runner output file declared by `rustdoc_test`.
+ opt_test_params (File): An output file we can optionally use to store params for `rustdoc`.
action (struct): Action arguments generated by `rustdoc_compile_action`.
crate_info (CrateInfo): The provider of the crate who's docs are being tested.
@@ -43,6 +44,9 @@
# Track the output path where the test writer should write the test
writer_args.add("--output={}".format(test_runner.path))
+ # Track where the test writer should move "spilled" Args to
+ writer_args.add("--optional_test_params={}".format(opt_test_params.path))
+
# Track what environment variables should be written to the test runner
writer_args.add("--action_env=DEVELOPER_DIR")
writer_args.add("--action_env=PATHEXT")
@@ -56,19 +60,29 @@
# files. To ensure rustdoc can find the appropriate dependencies, the
# file roots are identified and tracked for each dependency so it can be
# stripped from the test runner.
+
+ # Collect and dedupe all of the file roots in a list before appending
+ # them to args to prevent generating a large amount of identical args
+ roots = []
for dep in crate_info.deps.to_list():
dep_crate_info = getattr(dep, "crate_info", None)
dep_dep_info = getattr(dep, "dep_info", None)
if dep_crate_info:
root = dep_crate_info.output.root.path
- writer_args.add("--strip_substring={}/".format(root))
+ if not root in roots:
+ roots.append(root)
if dep_dep_info:
for direct_dep in dep_dep_info.direct_crates.to_list():
root = direct_dep.dep.output.root.path
- writer_args.add("--strip_substring={}/".format(root))
+ if not root in roots:
+ roots.append(root)
for transitive_dep in dep_dep_info.transitive_crates.to_list():
root = transitive_dep.output.root.path
- writer_args.add("--strip_substring={}/".format(root))
+ if not root in roots:
+ roots.append(root)
+
+ for root in roots:
+ writer_args.add("--strip_substring={}/".format(root))
# Indicate that the rustdoc_test args are over.
writer_args.add("--")
@@ -116,6 +130,12 @@
else:
test_runner = ctx.actions.declare_file(ctx.label.name + ".rustdoc_test.sh")
+ # Bazel will auto-magically spill params to a file, if they are too many for a given OSes shell
+ # (e.g. Windows ~32k, Linux ~2M). The executable script (aka test_runner) that gets generated,
+ # is run from the runfiles, which is separate from the params_file Bazel generates. To handle
+ # this case, we declare our own params file, that the test_writer will populate, if necessary
+ opt_test_params = ctx.actions.declare_file(ctx.label.name + ".rustdoc_opt_params", sibling = test_runner)
+
# Add the current crate as an extern for the compile action
rustdoc_flags = [
"--extern",
@@ -136,6 +156,7 @@
writer_args, env = _construct_writer_arguments(
ctx = ctx,
test_runner = test_runner,
+ opt_test_params = opt_test_params,
action = action,
crate_info = crate_info,
)
@@ -151,12 +172,12 @@
tools = tools,
arguments = [writer_args] + action.arguments,
env = action.env,
- outputs = [test_runner],
+ outputs = [test_runner, opt_test_params],
)
return [DefaultInfo(
files = depset([test_runner]),
- runfiles = ctx.runfiles(files = tools, transitive_files = action.inputs),
+ runfiles = ctx.runfiles(files = tools + [opt_test_params], transitive_files = action.inputs),
executable = test_runner,
)]
diff --git a/tools/rustdoc/rustdoc_test_writer.rs b/tools/rustdoc/rustdoc_test_writer.rs
index 6803e8b..0920c74 100644
--- a/tools/rustdoc/rustdoc_test_writer.rs
+++ b/tools/rustdoc/rustdoc_test_writer.rs
@@ -6,6 +6,7 @@
use std::collections::{BTreeMap, BTreeSet};
use std::env;
use std::fs;
+use std::io::{BufRead, BufReader};
use std::path::{Path, PathBuf};
#[derive(Debug)]
@@ -19,6 +20,10 @@
/// The path where the script should be written.
output: 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,
+
/// The `argv` of the configured rustdoc build action.
action_argv: Vec<String>,
}
@@ -50,6 +55,13 @@
.map(PathBuf::from)
.expect("Missing `--output` argument");
+ let optional_output_params_file = writer_args
+ .iter()
+ .find(|arg| arg.starts_with("--optional_test_params="))
+ .and_then(|arg| arg.splitn(2, '=').last())
+ .map(PathBuf::from)
+ .expect("Missing `--optional_test_params` argument");
+
let (strip_substring_args, writer_args): (Vec<String>, Vec<String>) = writer_args
.into_iter()
.partition(|arg| arg.starts_with("--strip_substring="));
@@ -85,10 +97,73 @@
env_keys,
strip_substrings,
output,
+ optional_output_params_file,
action_argv,
}
}
+/// Expand the Bazel Arg file and write it into our manually defined params file
+fn expand_params_file(mut options: Options) -> Options {
+ let params_extension = if cfg!(target_family = "windows") {
+ ".rustdoc_test.bat-0.params"
+ } else {
+ ".rustdoc_test.sh-0.params"
+ };
+
+ // We always need to produce the params file, we might overwrite this later though
+ fs::write(&options.optional_output_params_file, b"unused")
+ .expect("Failed to write params file");
+
+ // extract the path for the params file, if it exists
+ let params_path = match options.action_argv.pop() {
+ // Found the params file!
+ Some(arg) if arg.starts_with('@') && arg.ends_with(params_extension) => {
+ let path_str = arg
+ .strip_prefix('@')
+ .expect("Checked that there is an @ prefix");
+ PathBuf::from(path_str)
+ }
+ // No params file present, exit early
+ Some(arg) => {
+ options.action_argv.push(arg);
+ return options;
+ }
+ None => return options,
+ };
+
+ // read the params file
+ let params_file = fs::File::open(params_path).expect("Failed to read the rustdoc params file");
+ let content: Vec<_> = BufReader::new(params_file)
+ .lines()
+ .map(|line| line.expect("failed to parse param as String"))
+ // Remove any substrings found in the argument
+ .map(|arg| {
+ let mut stripped_arg = arg;
+ options
+ .strip_substrings
+ .iter()
+ .for_each(|substring| stripped_arg = stripped_arg.replace(substring, ""));
+ stripped_arg
+ })
+ .collect();
+
+ // add all arguments
+ fs::write(&options.optional_output_params_file, content.join("\n"))
+ .expect("Failed to write test runner");
+
+ // append the path of our new params file
+ let formatted_params_path = format!(
+ "@{}",
+ options
+ .optional_output_params_file
+ .to_str()
+ .expect("invalid UTF-8")
+ );
+ options.action_argv.push(formatted_params_path);
+
+ options
+}
+
/// Write a unix compatible test runner
fn write_test_runner_unix(
path: &Path,
@@ -195,6 +270,7 @@
fn main() {
let opt = parse_args();
+ let opt = expand_params_file(opt);
let env: BTreeMap<String, String> = env::vars()
.into_iter()
