From 0da0a2196d2d7b37bd5e07ce18b03568e2711f39 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Thu, 21 Apr 2022 22:19:36 -0500 Subject: [PATCH 1/2] Pass all paths to `Step::run` at once when using `ShouldRun::krate` This was surprisingly complicated. The main changes are: 1. Invert the order of iteration in `StepDescription::run`. Previously, it did something like: ```python for path in paths: for (step, should_run) in should_runs: if let Some(set) = should_run.pathset_for_path(path): step.run(builder, set) ``` That worked ok for individual paths, but didn't allow passing more than one path at a time to `Step::run` (since `pathset_for_paths` only had one path available to it). Change it to instead look at the intersection of `paths` and `should_run.paths`: ```python for (step, should_run) in should_runs: if let Some(set) = should_run.pathset_for_paths(paths): step.run(builder, set) ``` 2. Change `pathset_for_path` to take multiple pathsets. The goal is to avoid `x test library/alloc` testing *all* library crates, instead of just alloc. The changes here are similarly subtle, to use the intersection between the paths rather than all paths in `should_run.paths`. I added a test for the behavior to try and make it more clear. Note that we use pathsets instead of just paths to allow for sets with multiple aliases (*cough* `all_krates` *cough*). See the documentation added in the next commit for more detail. 3. Change `StepDescription::run` to explicitly handle 0 paths. Before this was implicitly handled by the `for` loop, which just didn't excute when there were no paths. Now it needs a check, to avoid trying to run all steps (this is a problem for steps that use `default_condition`). 4. Change `RunDescription` to have a list of pathsets, rather than a single path. 5. Remove paths as they're matched This allows checking at the end that no invalid paths are left over. Note that if two steps matched the same path, this will no longer run both; but that's a bug anyway. 6. Handle suite paths separately from regular sets. Running multiple suite paths at once instead of in separate `make_run` invocations is both tricky and not particularly useful. The respective test Steps already handle this by introspecting the original paths. Avoid having to deal with it by moving suite handling into a seperate loop than `PathSet::Set` checks. --- src/bootstrap/builder.rs | 170 +++++++++++++++++++++++---------- src/bootstrap/builder/tests.rs | 9 +- src/bootstrap/cache.rs | 4 +- src/bootstrap/lib.rs | 1 + src/bootstrap/test.rs | 35 ++++--- 5 files changed, 153 insertions(+), 66 deletions(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index 38d4f15d3c858..c3ee30b65fe4b 100644 --- a/src/bootstrap/builder.rs +++ b/src/bootstrap/builder.rs @@ -91,7 +91,7 @@ pub trait Step: 'static + Clone + Debug + PartialEq + Eq + Hash { pub struct RunConfig<'a> { pub builder: &'a Builder<'a>, pub target: TargetSelection, - pub path: PathBuf, + pub paths: Vec, } impl RunConfig<'_> { @@ -150,11 +150,16 @@ impl Debug for TaskPath { /// Collection of paths used to match a task rule. #[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq)] pub enum PathSet { - /// A collection of individual paths. + /// A collection of individual paths or aliases. /// /// These are generally matched as a path suffix. For example, a - /// command-line value of `libstd` will match if `src/libstd` is in the + /// command-line value of `std` will match if `library/std` is in the /// set. + /// + /// NOTE: the paths within a set should always be aliases of one another. + /// For example, `src/librustdoc` and `src/tools/rustdoc` should be in the same set, + /// but `library/core` and `library/std` generally should not, unless there's no way (for that Step) + /// to build them separately. Set(BTreeSet), /// A "suite" of paths. /// @@ -177,26 +182,65 @@ impl PathSet { } fn has(&self, needle: &Path, module: Option) -> bool { - let check = |p: &TaskPath| { - if let (Some(p_kind), Some(kind)) = (&p.kind, module) { - p.path.ends_with(needle) && *p_kind == kind - } else { - p.path.ends_with(needle) + match self { + PathSet::Set(set) => set.iter().any(|p| Self::check(p, needle, module)), + PathSet::Suite(suite) => Self::check(suite, needle, module), + } + } + + // internal use only + fn check(p: &TaskPath, needle: &Path, module: Option) -> bool { + if let (Some(p_kind), Some(kind)) = (&p.kind, module) { + p.path.ends_with(needle) && *p_kind == kind + } else { + p.path.ends_with(needle) + } + } + + /// Return all `TaskPath`s in `Self` that contain any of the `needles`, removing the + /// matched needles. + /// + /// This is used for `StepDescription::krate`, which passes all matching crates at once to + /// `Step::make_run`, rather than calling it many times with a single crate. + /// See `tests.rs` for examples. + fn intersection_removing_matches( + &self, + needles: &mut Vec<&Path>, + module: Option, + ) -> PathSet { + let mut check = |p| { + for (i, n) in needles.iter().enumerate() { + let matched = Self::check(p, n, module); + if matched { + needles.remove(i); + return true; + } } + false }; - match self { - PathSet::Set(set) => set.iter().any(check), - PathSet::Suite(suite) => check(suite), + PathSet::Set(set) => PathSet::Set(set.iter().filter(|&p| check(p)).cloned().collect()), + PathSet::Suite(suite) => { + if check(suite) { + self.clone() + } else { + PathSet::empty() + } + } } } - fn path(&self, builder: &Builder<'_>) -> PathBuf { + /// A convenience wrapper for Steps which know they have no aliases and all their sets contain only a single path. + /// + /// This can be used with [`ShouldRun::krate`], [`ShouldRun::path`], or [`ShouldRun::alias`]. + #[track_caller] + pub fn assert_single_path(&self) -> &TaskPath { match self { PathSet::Set(set) => { - set.iter().next().map(|p| &p.path).unwrap_or(&builder.build.src).clone() + assert_eq!(set.len(), 1, "called assert_single_path on multiple paths"); + set.iter().next().unwrap() } - PathSet::Suite(path) => path.path.clone(), + PathSet::Suite(_) => unreachable!("called assert_single_path on a Suite path"), } } } @@ -213,8 +257,8 @@ impl StepDescription { } } - fn maybe_run(&self, builder: &Builder<'_>, pathset: &PathSet) { - if self.is_excluded(builder, pathset) { + fn maybe_run(&self, builder: &Builder<'_>, pathsets: Vec) { + if pathsets.iter().any(|set| self.is_excluded(builder, set)) { return; } @@ -222,7 +266,7 @@ impl StepDescription { let targets = if self.only_hosts { &builder.hosts } else { &builder.targets }; for target in targets { - let run = RunConfig { builder, path: pathset.path(builder), target: *target }; + let run = RunConfig { builder, paths: pathsets.clone(), target: *target }; (self.make_run)(run); } } @@ -261,46 +305,51 @@ impl StepDescription { for (desc, should_run) in v.iter().zip(&should_runs) { if desc.default && should_run.is_really_default() { for pathset in &should_run.paths { - desc.maybe_run(builder, pathset); + desc.maybe_run(builder, vec![pathset.clone()]); } } } } - for path in paths { - // strip CurDir prefix if present - let path = match path.strip_prefix(".") { - Ok(p) => p, - Err(_) => path, - }; + // strip CurDir prefix if present + let mut paths: Vec<_> = + paths.into_iter().map(|p| p.strip_prefix(".").unwrap_or(p)).collect(); - let mut attempted_run = false; + // Handle all test suite paths. + // (This is separate from the loop below to avoid having to handle multiple paths in `is_suite_path` somehow.) + paths.retain(|path| { for (desc, should_run) in v.iter().zip(&should_runs) { - if let Some(suite) = should_run.is_suite_path(path) { - attempted_run = true; - desc.maybe_run(builder, suite); - } else if let Some(pathset) = should_run.pathset_for_path(path, desc.kind) { - attempted_run = true; - desc.maybe_run(builder, pathset); + if let Some(suite) = should_run.is_suite_path(&path) { + desc.maybe_run(builder, vec![suite.clone()]); + return false; } } + true + }); - if !attempted_run { - eprintln!( - "error: no `{}` rules matched '{}'", - builder.kind.as_str(), - path.display() - ); - eprintln!( - "help: run `x.py {} --help --verbose` to show a list of available paths", - builder.kind.as_str() - ); - eprintln!( - "note: if you are adding a new Step to bootstrap itself, make sure you register it with `describe!`" - ); - std::process::exit(1); + if paths.is_empty() { + return; + } + + // Handle all PathSets. + for (desc, should_run) in v.iter().zip(&should_runs) { + let pathsets = should_run.pathset_for_paths_removing_matches(&mut paths, desc.kind); + if !pathsets.is_empty() { + desc.maybe_run(builder, pathsets); } } + + if !paths.is_empty() { + eprintln!("error: no `{}` rules matched {:?}", builder.kind.as_str(), paths,); + eprintln!( + "help: run `x.py {} --help --verbose` to show a list of available paths", + builder.kind.as_str() + ); + eprintln!( + "note: if you are adding a new Step to bootstrap itself, make sure you register it with `describe!`" + ); + std::process::exit(1); + } } } @@ -370,7 +419,7 @@ impl<'a> ShouldRun<'a> { /// Indicates it should run if the command-line selects the given crate or /// any of its (local) dependencies. /// - /// `make_run` will be called separately for each matching command-line path. + /// `make_run` will be called a single time with all matching command-line paths. pub fn krate(mut self, name: &str) -> Self { for krate in self.builder.in_tree_crates(name, None) { let path = krate.local_path(self.builder); @@ -417,9 +466,10 @@ impl<'a> ShouldRun<'a> { self } - pub fn is_suite_path(&self, path: &Path) -> Option<&PathSet> { + /// Handles individual files (not directories) within a test suite. + fn is_suite_path(&self, requested_path: &Path) -> Option<&PathSet> { self.paths.iter().find(|pathset| match pathset { - PathSet::Suite(p) => path.starts_with(&p.path), + PathSet::Suite(suite) => requested_path.starts_with(&suite.path), PathSet::Set(_) => false, }) } @@ -435,8 +485,28 @@ impl<'a> ShouldRun<'a> { self } - fn pathset_for_path(&self, path: &Path, kind: Kind) -> Option<&PathSet> { - self.paths.iter().find(|pathset| pathset.has(path, Some(kind))) + /// Given a set of requested paths, return the subset which match the Step for this `ShouldRun`, + /// removing the matches from `paths`. + /// + /// NOTE: this returns multiple PathSets to allow for the possibility of multiple units of work + /// within the same step. For example, `test::Crate` allows testing multiple crates in the same + /// cargo invocation, which are put into separate sets because they aren't aliases. + /// + /// The reason we return PathSet instead of PathBuf is to allow for aliases that mean the same thing + /// (for now, just `all_krates` and `paths`, but we may want to add an `aliases` function in the future?) + fn pathset_for_paths_removing_matches( + &self, + paths: &mut Vec<&Path>, + kind: Kind, + ) -> Vec { + let mut sets = vec![]; + for pathset in &self.paths { + let subset = pathset.intersection_removing_matches(paths, Some(kind)); + if subset != PathSet::empty() { + sets.push(subset); + } + } + sets } } diff --git a/src/bootstrap/builder/tests.rs b/src/bootstrap/builder/tests.rs index 4ab502e90526f..f030917d7a431 100644 --- a/src/bootstrap/builder/tests.rs +++ b/src/bootstrap/builder/tests.rs @@ -46,6 +46,13 @@ fn run_build(paths: &[PathBuf], config: Config) -> Cache { builder.cache } +#[test] +fn test_intersection() { + let set = PathSet::Set(["library/core", "library/alloc", "library/std"].into_iter().map(TaskPath::parse).collect()); + let subset = set.intersection(&[Path::new("library/core"), Path::new("library/alloc"), Path::new("library/stdarch")], None); + assert_eq!(subset, PathSet::Set(["library/core", "library/alloc"].into_iter().map(TaskPath::parse).collect())); +} + #[test] fn test_exclude() { let mut config = configure("test", &["A"], &["A"]); @@ -539,7 +546,7 @@ mod dist { target: host, mode: Mode::Std, test_kind: test::TestKind::Test, - krate: INTERNER.intern_str("std"), + crates: vec![INTERNER.intern_str("std")], },] ); } diff --git a/src/bootstrap/cache.rs b/src/bootstrap/cache.rs index fac5d8db5119d..97f0bfdc484da 100644 --- a/src/bootstrap/cache.rs +++ b/src/bootstrap/cache.rs @@ -270,7 +270,7 @@ impl Cache { #[cfg(test)] impl Cache { - pub fn all(&mut self) -> Vec<(S, S::Output)> { + pub fn all(&mut self) -> Vec<(S, S::Output)> { let cache = self.0.get_mut(); let type_id = TypeId::of::(); let mut v = cache @@ -278,7 +278,7 @@ impl Cache { .map(|b| b.downcast::>().expect("correct type")) .map(|m| m.into_iter().collect::>()) .unwrap_or_default(); - v.sort_by_key(|&(a, _)| a); + v.sort_by_key(|(s, _)| s.clone()); v } diff --git a/src/bootstrap/lib.rs b/src/bootstrap/lib.rs index b4333566f07d9..2ae49b59fd9ba 100644 --- a/src/bootstrap/lib.rs +++ b/src/bootstrap/lib.rs @@ -171,6 +171,7 @@ mod job { pub unsafe fn setup(_build: &mut crate::Build) {} } +pub use crate::builder::PathSet; use crate::cache::{Interned, INTERNER}; pub use crate::config::Config; pub use crate::flags::Subcommand; diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index fdce078bbedf5..9958306b5765c 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -1856,12 +1856,12 @@ impl Step for RustcGuide { } } -#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct CrateLibrustc { compiler: Compiler, target: TargetSelection, test_kind: TestKind, - krate: Interned, + crates: Vec>, } impl Step for CrateLibrustc { @@ -1877,10 +1877,14 @@ impl Step for CrateLibrustc { let builder = run.builder; let host = run.build_triple(); let compiler = builder.compiler_for(builder.top_stage, host, host); - let krate = builder.crate_paths[&run.path]; + let crates = run + .paths + .iter() + .map(|p| builder.crate_paths[&p.assert_single_path().path].clone()) + .collect(); let test_kind = builder.kind.into(); - builder.ensure(CrateLibrustc { compiler, target: run.target, test_kind, krate }); + builder.ensure(CrateLibrustc { compiler, target: run.target, test_kind, crates }); } fn run(self, builder: &Builder<'_>) { @@ -1889,18 +1893,18 @@ impl Step for CrateLibrustc { target: self.target, mode: Mode::Rustc, test_kind: self.test_kind, - krate: self.krate, + crates: self.crates, }); } } -#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct Crate { pub compiler: Compiler, pub target: TargetSelection, pub mode: Mode, pub test_kind: TestKind, - pub krate: Interned, + pub crates: Vec>, } impl Step for Crate { @@ -1916,9 +1920,13 @@ impl Step for Crate { let host = run.build_triple(); let compiler = builder.compiler_for(builder.top_stage, host, host); let test_kind = builder.kind.into(); - let krate = builder.crate_paths[&run.path]; + let crates = run + .paths + .iter() + .map(|p| builder.crate_paths[&p.assert_single_path().path].clone()) + .collect(); - builder.ensure(Crate { compiler, target: run.target, mode: Mode::Std, test_kind, krate }); + builder.ensure(Crate { compiler, target: run.target, mode: Mode::Std, test_kind, crates }); } /// Runs all unit tests plus documentation tests for a given crate defined @@ -1934,7 +1942,6 @@ impl Step for Crate { let target = self.target; let mode = self.mode; let test_kind = self.test_kind; - let krate = self.krate; builder.ensure(compile::Std { compiler, target }); builder.ensure(RemoteCopyLibs { compiler, target }); @@ -1975,7 +1982,9 @@ impl Step for Crate { DocTests::Yes => {} } - cargo.arg("-p").arg(krate); + for krate in &self.crates { + cargo.arg("-p").arg(krate); + } // The tests are going to run with the *target* libraries, so we need to // ensure that those libraries show up in the LD_LIBRARY_PATH equivalent. @@ -2011,8 +2020,8 @@ impl Step for Crate { } builder.info(&format!( - "{} {} stage{} ({} -> {})", - test_kind, krate, compiler.stage, &compiler.host, target + "{} {:?} stage{} ({} -> {})", + test_kind, self.crates, compiler.stage, &compiler.host, target )); let _time = util::timeit(&builder); try_run(builder, &mut cargo.into()); From fca6dbd9afac228b91749a5ab5c2ba2d8bfcb6ef Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 27 Apr 2022 23:43:33 -0500 Subject: [PATCH 2/2] Add tests for fixed bugs --- src/bootstrap/builder.rs | 4 ++++ src/bootstrap/builder/tests.rs | 40 ++++++++++++++++++++++++++++++---- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index c3ee30b65fe4b..3b20cc4ca3929 100644 --- a/src/bootstrap/builder.rs +++ b/src/bootstrap/builder.rs @@ -348,7 +348,11 @@ impl StepDescription { eprintln!( "note: if you are adding a new Step to bootstrap itself, make sure you register it with `describe!`" ); + #[cfg(not(test))] std::process::exit(1); + #[cfg(test)] + // so we can use #[should_panic] + panic!() } } } diff --git a/src/bootstrap/builder/tests.rs b/src/bootstrap/builder/tests.rs index f030917d7a431..70cb0de7cce04 100644 --- a/src/bootstrap/builder/tests.rs +++ b/src/bootstrap/builder/tests.rs @@ -3,7 +3,11 @@ use crate::config::{Config, TargetSelection}; use std::thread; fn configure(cmd: &str, host: &[&str], target: &[&str]) -> Config { - let mut config = Config::parse(&[cmd.to_owned()]); + configure_with_args(&[cmd.to_owned()], host, target) +} + +fn configure_with_args(cmd: &[String], host: &[&str], target: &[&str]) -> Config { + let mut config = Config::parse(cmd); // don't save toolstates config.save_toolstates = None; config.dry_run = true; @@ -46,11 +50,39 @@ fn run_build(paths: &[PathBuf], config: Config) -> Cache { builder.cache } +fn check_cli(paths: [&str; N]) { + run_build( + &paths.map(PathBuf::from), + configure_with_args(&paths.map(String::from), &["A"], &["A"]), + ); +} + +#[test] +fn test_valid() { + // make sure multi suite paths are accepted + check_cli(["test", "src/test/ui/attr-start.rs", "src/test/ui/attr-shebang.rs"]); +} + +#[test] +#[should_panic] +fn test_invalid() { + // make sure that invalid paths are caught, even when combined with valid paths + check_cli(["test", "library/std", "x"]); +} + #[test] fn test_intersection() { - let set = PathSet::Set(["library/core", "library/alloc", "library/std"].into_iter().map(TaskPath::parse).collect()); - let subset = set.intersection(&[Path::new("library/core"), Path::new("library/alloc"), Path::new("library/stdarch")], None); - assert_eq!(subset, PathSet::Set(["library/core", "library/alloc"].into_iter().map(TaskPath::parse).collect())); + let set = PathSet::Set( + ["library/core", "library/alloc", "library/std"].into_iter().map(TaskPath::parse).collect(), + ); + let mut command_paths = + vec![Path::new("library/core"), Path::new("library/alloc"), Path::new("library/stdarch")]; + let subset = set.intersection_removing_matches(&mut command_paths, None); + assert_eq!( + subset, + PathSet::Set(["library/core", "library/alloc"].into_iter().map(TaskPath::parse).collect()) + ); + assert_eq!(command_paths, vec![Path::new("library/stdarch")]); } #[test]