Skip to content

fix: rust_doc_test failure to find params file #1418

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

Merged
merged 4 commits into from
Aug 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions crate_universe/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,4 @@ rust_doc(
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": [],
}),
)
6 changes: 5 additions & 1 deletion crate_universe/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ pub use splice::splice;
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
)]
Comment on lines +22 to +26
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

pub enum Options {
/// Generate Bazel Build files from a Cargo manifest.
Generate(GenerateOptions),
Expand Down
2 changes: 1 addition & 1 deletion crate_universe/src/cli/generate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use crate::splicing::SplicingManifest;

/// 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")]
Expand Down
2 changes: 1 addition & 1 deletion crate_universe/src/cli/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::splicing::SplicingManifest;

/// 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)]
Expand Down
2 changes: 1 addition & 1 deletion crate_universe/src/cli/splice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::splicing::{generate_lockfile, Splicer, SplicingManifest, WorkspaceMet

/// 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)]
Expand Down
2 changes: 1 addition & 1 deletion crate_universe/src/cli/vendor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use crate::splicing::{generate_lockfile, Splicer, SplicingManifest, WorkspaceMet

/// 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")]
Expand Down
33 changes: 27 additions & 6 deletions rust/private/rustdoc_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ load("//rust/private:providers.bzl", "CrateInfo")
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
Expand All @@ -29,6 +29,7 @@ def _construct_writer_arguments(ctx, test_runner, action, crate_info):
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.

Expand All @@ -43,6 +44,9 @@ def _construct_writer_arguments(ctx, test_runner, action, crate_info):
# 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")
Expand All @@ -56,19 +60,29 @@ def _construct_writer_arguments(ctx, test_runner, action, crate_info):
# 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("--")
Expand Down Expand Up @@ -116,6 +130,12 @@ def _rust_doc_test_impl(ctx):
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",
Expand All @@ -136,6 +156,7 @@ def _rust_doc_test_impl(ctx):
writer_args, env = _construct_writer_arguments(
ctx = ctx,
test_runner = test_runner,
opt_test_params = opt_test_params,
action = action,
crate_info = crate_info,
)
Expand All @@ -151,12 +172,12 @@ def _rust_doc_test_impl(ctx):
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,
)]

Expand Down
76 changes: 76 additions & 0 deletions tools/rustdoc/rustdoc_test_writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::cmp::Reverse;
use std::collections::{BTreeMap, BTreeSet};
use std::env;
use std::fs;
use std::io::{BufRead, BufReader};
use std::path::{Path, PathBuf};

#[derive(Debug)]
Expand All @@ -19,6 +20,10 @@ struct Options {
/// 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>,
}
Expand Down Expand Up @@ -50,6 +55,13 @@ fn parse_args() -> Options {
.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="));
Expand Down Expand Up @@ -85,10 +97,73 @@ fn parse_args() -> Options {
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,
Expand Down Expand Up @@ -195,6 +270,7 @@ fn write_test_runner(

fn main() {
let opt = parse_args();
let opt = expand_params_file(opt);

let env: BTreeMap<String, String> = env::vars()
.into_iter()
Expand Down