Pass all paths to Step::run at once when using ShouldRun::krate

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<PathSet>,
 }
 
 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<TaskPath>),
     /// A "suite" of paths.
     ///
@@ -177,26 +182,65 @@ impl PathSet {
     }
 
     fn has(&self, needle: &Path, module: Option<Kind>) -> 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<Kind>) -> 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<Kind>,
+    ) -> 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,16 +257,16 @@ impl StepDescription {
         }
     }
 
-    fn maybe_run(&self, builder: &Builder<'_>, pathset: &PathSet) {
-        if self.is_excluded(builder, pathset) {
+    fn maybe_run(&self, builder: &Builder<'_>, pathsets: Vec<PathSet>) {
+        if pathsets.iter().any(|set| self.is_excluded(builder, set)) {
             return;
         }
 
         // Determine the targets participating in this rule.
         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,55 @@ 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!`"
+            );
+            #[cfg(not(test))]
+            std::process::exit(1);
+            #[cfg(test)]
+            // so we can use #[should_panic]
+            panic!()
+        }
     }
 }
 
@@ -370,7 +423,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 +470,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 +489,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<PathSet> {
+        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
     }
 }
 

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,6 +50,41 @@ fn run_build(paths: &[PathBuf], config: Config) -> Cache {
     builder.cache
 }
 
+fn check_cli<const N: usize>(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 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]
 fn test_exclude() {
     let mut config = configure("test", &["A"], &["A"]);
@@ -539,7 +578,7 @@ mod dist {
                 target: host,
                 mode: Mode::Std,
                 test_kind: test::TestKind::Test,
-                krate: INTERNER.intern_str("std"),
+                crates: vec![INTERNER.intern_str("std")],
             },]
         );
     }

src/bootstrap/cache.rs

@@ -270,15 +270,15 @@ impl Cache {
 
 #[cfg(test)]
 impl Cache {
-    pub fn all<S: Ord + Copy + Step>(&mut self) -> Vec<(S, S::Output)> {
+    pub fn all<S: Ord + Clone + Step>(&mut self) -> Vec<(S, S::Output)> {
         let cache = self.0.get_mut();
         let type_id = TypeId::of::<S>();
         let mut v = cache
             .remove(&type_id)
             .map(|b| b.downcast::<HashMap<S, S::Output>>().expect("correct type"))
             .map(|m| m.into_iter().collect::<Vec<_>>())
             .unwrap_or_default();
-        v.sort_by_key(|&(a, _)| a);
+        v.sort_by_key(|(s, _)| s.clone());
         v
     }
 

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;