Skip to content

Commit

Permalink
Auto merge of #110576 - jyn514:unify-test-args, r=ozkanonur
Browse files Browse the repository at this point in the history
bootstrap: Unify test argument handling

Fixes #104198. Does *not* help with #80124 because I couldn't figure out a reasonable way to omit `--lib` only for `panic_abort` and not other `std` dependencies.

- Remove unnecessary `test_kind` field and `TestKind` struct. These are just subsets of the existing `builder.kind` / `Kind` struct.
- Add a new `run_cargo_test` function which handles passing arguments to cargo based on `builder.config`
- Switch all Steps in `mod test` to `run_cargo_test` where possible
- Combine several steps into one `CrateBootstrap` step. These tests all do the same thing, just with different crate names.
- Fix `x test --no-doc`. This is much simpler after the refactors mentioned earlier, but I'm happy to split it into a separate PR if desired. Before, this would panic a lot because steps forgot to pass `--lib`.
  • Loading branch information
bors committed Apr 29, 2023
2 parents eb62877 + 78a7093 commit 87b1f89
Show file tree
Hide file tree
Showing 6 changed files with 215 additions and 349 deletions.
13 changes: 9 additions & 4 deletions src/bootstrap/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,14 @@ impl Kind {
Kind::Suggest => "suggest",
}
}

pub fn test_description(&self) -> &'static str {
match self {
Kind::Test => "Testing",
Kind::Bench => "Benchmarking",
_ => panic!("not a test command: {}!", self.as_str()),
}
}
}

impl<'a> Builder<'a> {
Expand Down Expand Up @@ -695,7 +703,6 @@ impl<'a> Builder<'a> {
crate::toolstate::ToolStateCheck,
test::ExpandYamlAnchors,
test::Tidy,
test::TidySelfTest,
test::Ui,
test::RunPassValgrind,
test::MirOpt,
Expand All @@ -711,11 +718,9 @@ impl<'a> Builder<'a> {
test::CrateLibrustc,
test::CrateRustdoc,
test::CrateRustdocJsonTypes,
test::CrateJsonDocLint,
test::SuggestTestsCrate,
test::CrateBootstrap,
test::Linkcheck,
test::TierCheck,
test::ReplacePlaceholderTest,
test::Cargotest,
test::Cargo,
test::RustAnalyzer,
Expand Down
1 change: 0 additions & 1 deletion src/bootstrap/builder/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,6 @@ mod dist {
compiler: Compiler { host, stage: 0 },
target: host,
mode: Mode::Std,
test_kind: test::TestKind::Test,
crates: vec![INTERNER.intern_str("std")],
},]
);
Expand Down
1 change: 1 addition & 0 deletions src/bootstrap/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ struct Crate {
name: Interned<String>,
deps: HashSet<Interned<String>>,
path: PathBuf,
has_lib: bool,
}

impl Crate {
Expand Down
42 changes: 26 additions & 16 deletions src/bootstrap/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use serde_derive::Deserialize;

use crate::cache::INTERNER;
use crate::util::output;
use crate::{Build, Crate};
use crate::{t, Build, Crate};

/// For more information, see the output of
/// <https://doc.rust-lang.org/nightly/cargo/commands/cargo-metadata.html>
Expand All @@ -22,6 +22,7 @@ struct Package {
source: Option<String>,
manifest_path: String,
dependencies: Vec<Dependency>,
targets: Vec<Target>,
}

/// For more information, see the output of
Expand All @@ -32,6 +33,11 @@ struct Dependency {
source: Option<String>,
}

#[derive(Debug, Deserialize)]
struct Target {
kind: Vec<String>,
}

/// Collects and stores package metadata of each workspace members into `build`,
/// by executing `cargo metadata` commands.
pub fn build(build: &mut Build) {
Expand All @@ -46,11 +52,16 @@ pub fn build(build: &mut Build) {
.filter(|dep| dep.source.is_none())
.map(|dep| INTERNER.intern_string(dep.name))
.collect();
let krate = Crate { name, deps, path };
let has_lib = package.targets.iter().any(|t| t.kind.iter().any(|k| k == "lib"));
let krate = Crate { name, deps, path, has_lib };
let relative_path = krate.local_path(build);
build.crates.insert(name, krate);
let existing_path = build.crate_paths.insert(relative_path, name);
assert!(existing_path.is_none(), "multiple crates with the same path");
assert!(
existing_path.is_none(),
"multiple crates with the same path: {}",
existing_path.unwrap()
);
}
}
}
Expand All @@ -60,29 +71,28 @@ pub fn build(build: &mut Build) {
/// Note that `src/tools/cargo` is no longer a workspace member but we still
/// treat it as one here, by invoking an additional `cargo metadata` command.
fn workspace_members(build: &Build) -> impl Iterator<Item = Package> {
let cmd_metadata = |manifest_path| {
let collect_metadata = |manifest_path| {
let mut cargo = Command::new(&build.initial_cargo);
cargo
.arg("metadata")
.arg("--format-version")
.arg("1")
.arg("--no-deps")
.arg("--manifest-path")
.arg(manifest_path);
cargo
.arg(build.src.join(manifest_path));
let metadata_output = output(&mut cargo);
let Output { packages, .. } = t!(serde_json::from_str(&metadata_output));
packages
};

// Collects `metadata.packages` from the root workspace.
let root_manifest_path = build.src.join("Cargo.toml");
let root_output = output(&mut cmd_metadata(&root_manifest_path));
let Output { packages, .. } = serde_json::from_str(&root_output).unwrap();

// Collects `metadata.packages` from src/tools/cargo separately.
let cargo_manifest_path = build.src.join("src/tools/cargo/Cargo.toml");
let cargo_output = output(&mut cmd_metadata(&cargo_manifest_path));
let Output { packages: cargo_packages, .. } = serde_json::from_str(&cargo_output).unwrap();
// Collects `metadata.packages` from all workspaces.
let packages = collect_metadata("Cargo.toml");
let cargo_packages = collect_metadata("src/tools/cargo/Cargo.toml");
let ra_packages = collect_metadata("src/tools/rust-analyzer/Cargo.toml");
let bootstrap_packages = collect_metadata("src/bootstrap/Cargo.toml");

// We only care about the root package from `src/tool/cargo` workspace.
let cargo_package = cargo_packages.into_iter().find(|pkg| pkg.name == "cargo").into_iter();
packages.into_iter().chain(cargo_package)

packages.into_iter().chain(cargo_package).chain(ra_packages).chain(bootstrap_packages)
}
Loading

0 comments on commit 87b1f89

Please sign in to comment.