Rollup merge of #91965 - ferrocene:pa-more-granular-exclude, r=Mark-S…

…imulacrum

Add more granular `--exclude` in `x.py`

x.py has support for excluding some steps from the current invocation, but unfortunately that's not granular enough: some steps have the same name in different modules, and that prevents excluding only *some* of them.

As a practical example, let's say you need to run everything in `./x.py test` except for the standard library tests, as those tests require IPv6 and need to be executed on a separate machine. Before this commit, if you were to just run this:

    ./x.py test --exclude library/std

...the invocation would eventually fail, as that would not only exclude running the tests for the standard library (`library/std` in the `test` module), it would also exclude generating its documentation (`library/std` in the `doc` module), breaking linkchecker.

This commit adds support to the `--exclude` flag for prefixing paths with the name of the module their step is defined in, allowing the user to choose which module to exclude from:

    ./x.py test --exclude test::library/std

This maintains backward compatibility with existing invocations, while allowing more ganular exclusion. Examples of the behavior:

| `--exclude`         | Docs    | Tests   |
| ------------------- | ------- | ------- |
| `library/std`       | Skipped | Skipped |
| `doc::library/std`  | Skipped | Run     |
| `test::library/std` | Run     | Skipped |

Note that this PR only changes the `--exclude` flag, and not in other `x.py` arguments or flags yet.

In the implementation I tried to limit the impact this would have with rustbuild as a whole as much as possible. The module name is extracted from the step by parsing the result of `std::any::type_name()`: unfortunately that output can change at any point in time, but IMO it's better than having to annotate all the existing and future `Step` implementations with the module name. I added a test to ensure the parsing works as expected, so hopefully if anyone makes changes to the output of `std::any::type_name()` they'll also notice they have to update rustbuild.

r? `@Mark-Simulacrum`

src/bootstrap/builder.rs

@@ -7,7 +7,7 @@ use std::fmt::Debug;
 use std::fs;
 use std::hash::Hash;
 use std::ops::Deref;
-use std::path::{Path, PathBuf};
+use std::path::{Component, Path, PathBuf};
 use std::process::Command;
 use std::time::{Duration, Instant};
 
@@ -105,6 +105,44 @@ struct StepDescription {
     should_run: fn(ShouldRun<'_>) -> ShouldRun<'_>,
     make_run: fn(RunConfig<'_>),
     name: &'static str,
+    kind: Kind,
+}
+
+#[derive(Clone, PartialOrd, Ord, PartialEq, Eq)]
+pub struct TaskPath {
+    pub path: PathBuf,
+    pub kind: Option<Kind>,
+}
+
+impl TaskPath {
+    pub fn parse(path: impl Into<PathBuf>) -> TaskPath {
+        let mut kind = None;
+        let mut path = path.into();
+
+        let mut components = path.components();
+        if let Some(Component::Normal(os_str)) = components.next() {
+            if let Some(str) = os_str.to_str() {
+                if let Some((found_kind, found_prefix)) = str.split_once("::") {
+                    if found_kind.is_empty() {
+                        panic!("empty kind in task path {}", path.display());
+                    }
+                    kind = Some(Kind::parse(found_kind));
+                    path = Path::new(found_prefix).join(components.as_path());
+                }
+            }
+        }
+
+        TaskPath { path, kind }
+    }
+}
+
+impl Debug for TaskPath {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        if let Some(kind) = &self.kind {
+            write!(f, "{}::", kind.as_str())?;
+        }
+        write!(f, "{}", self.path.display())
+    }
 }
 
 /// Collection of paths used to match a task rule.
@@ -115,50 +153,61 @@ pub enum PathSet {
     /// These are generally matched as a path suffix. For example, a
     /// command-line value of `libstd` will match if `src/libstd` is in the
     /// set.
-    Set(BTreeSet<PathBuf>),
+    Set(BTreeSet<TaskPath>),
     /// A "suite" of paths.
     ///
     /// These can match as a path suffix (like `Set`), or as a prefix. For
     /// example, a command-line value of `src/test/ui/abi/variadic-ffi.rs`
     /// will match `src/test/ui`. A command-line value of `ui` would also
     /// match `src/test/ui`.
-    Suite(PathBuf),
+    Suite(TaskPath),
 }
 
 impl PathSet {
     fn empty() -> PathSet {
         PathSet::Set(BTreeSet::new())
     }
 
-    fn one<P: Into<PathBuf>>(path: P) -> PathSet {
+    fn one<P: Into<PathBuf>>(path: P, kind: Kind) -> PathSet {
         let mut set = BTreeSet::new();
-        set.insert(path.into());
+        set.insert(TaskPath { path: path.into(), kind: Some(kind.into()) });
         PathSet::Set(set)
     }
 
-    fn has(&self, needle: &Path) -> bool {
+    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| p.ends_with(needle)),
-            PathSet::Suite(suite) => suite.ends_with(needle),
+            PathSet::Set(set) => set.iter().any(check),
+            PathSet::Suite(suite) => check(suite),
         }
     }
 
     fn path(&self, builder: &Builder<'_>) -> PathBuf {
         match self {
-            PathSet::Set(set) => set.iter().next().unwrap_or(&builder.build.src).to_path_buf(),
-            PathSet::Suite(path) => PathBuf::from(path),
+            PathSet::Set(set) => {
+                set.iter().next().map(|p| &p.path).unwrap_or(&builder.build.src).clone()
+            }
+            PathSet::Suite(path) => path.path.clone(),
         }
     }
 }
 
 impl StepDescription {
-    fn from<S: Step>() -> StepDescription {
+    fn from<S: Step>(kind: Kind) -> StepDescription {
         StepDescription {
             default: S::DEFAULT,
             only_hosts: S::ONLY_HOSTS,
             should_run: S::should_run,
             make_run: S::make_run,
             name: std::any::type_name::<S>(),
+            kind,
         }
     }
 
@@ -177,7 +226,7 @@ impl StepDescription {
     }
 
     fn is_excluded(&self, builder: &Builder<'_>, pathset: &PathSet) -> bool {
-        if builder.config.exclude.iter().any(|e| pathset.has(e)) {
+        if builder.config.exclude.iter().any(|e| pathset.has(&e.path, e.kind)) {
             eprintln!("Skipping {:?} because it is excluded", pathset);
             return true;
         }
@@ -192,8 +241,10 @@ impl StepDescription {
     }
 
     fn run(v: &[StepDescription], builder: &Builder<'_>, paths: &[PathBuf]) {
-        let should_runs =
-            v.iter().map(|desc| (desc.should_run)(ShouldRun::new(builder))).collect::<Vec<_>>();
+        let should_runs = v
+            .iter()
+            .map(|desc| (desc.should_run)(ShouldRun::new(builder, desc.kind)))
+            .collect::<Vec<_>>();
 
         // sanity checks on rules
         for (desc, should_run) in v.iter().zip(&should_runs) {
@@ -226,7 +277,7 @@ impl StepDescription {
                 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) {
+                } else if let Some(pathset) = should_run.pathset_for_path(path, desc.kind) {
                     attempted_run = true;
                     desc.maybe_run(builder, pathset);
                 }
@@ -246,6 +297,8 @@ enum ReallyDefault<'a> {
 
 pub struct ShouldRun<'a> {
     pub builder: &'a Builder<'a>,
+    kind: Kind,
+
     // use a BTreeSet to maintain sort order
     paths: BTreeSet<PathSet>,
 
@@ -255,9 +308,10 @@ pub struct ShouldRun<'a> {
 }
 
 impl<'a> ShouldRun<'a> {
-    fn new(builder: &'a Builder<'_>) -> ShouldRun<'a> {
+    fn new(builder: &'a Builder<'_>, kind: Kind) -> ShouldRun<'a> {
         ShouldRun {
             builder,
+            kind,
             paths: BTreeSet::new(),
             is_really_default: ReallyDefault::Bool(true), // by default no additional conditions
         }
@@ -293,7 +347,7 @@ impl<'a> ShouldRun<'a> {
         let mut set = BTreeSet::new();
         for krate in self.builder.in_tree_crates(name, None) {
             let path = krate.local_path(self.builder);
-            set.insert(path);
+            set.insert(TaskPath { path, kind: Some(self.kind) });
         }
         self.paths.insert(PathSet::Set(set));
         self
@@ -306,7 +360,7 @@ impl<'a> ShouldRun<'a> {
     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);
-            self.paths.insert(PathSet::one(path));
+            self.paths.insert(PathSet::one(path, self.kind));
         }
         self
     }
@@ -318,19 +372,25 @@ impl<'a> ShouldRun<'a> {
 
     // multiple aliases for the same job
     pub fn paths(mut self, paths: &[&str]) -> Self {
-        self.paths.insert(PathSet::Set(paths.iter().map(PathBuf::from).collect()));
+        self.paths.insert(PathSet::Set(
+            paths
+                .iter()
+                .map(|p| TaskPath { path: p.into(), kind: Some(self.kind.into()) })
+                .collect(),
+        ));
         self
     }
 
     pub fn is_suite_path(&self, path: &Path) -> Option<&PathSet> {
         self.paths.iter().find(|pathset| match pathset {
-            PathSet::Suite(p) => path.starts_with(p),
+            PathSet::Suite(p) => path.starts_with(&p.path),
             PathSet::Set(_) => false,
         })
     }
 
     pub fn suite_path(mut self, suite: &str) -> Self {
-        self.paths.insert(PathSet::Suite(PathBuf::from(suite)));
+        self.paths
+            .insert(PathSet::Suite(TaskPath { path: suite.into(), kind: Some(self.kind.into()) }));
         self
     }
 
@@ -340,12 +400,12 @@ impl<'a> ShouldRun<'a> {
         self
     }
 
-    fn pathset_for_path(&self, path: &Path) -> Option<&PathSet> {
-        self.paths.iter().find(|pathset| pathset.has(path))
+    fn pathset_for_path(&self, path: &Path, kind: Kind) -> Option<&PathSet> {
+        self.paths.iter().find(|pathset| pathset.has(path, Some(kind)))
     }
 }
 
-#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug)]
 pub enum Kind {
     Build,
     Check,
@@ -359,11 +419,44 @@ pub enum Kind {
     Run,
 }
 
+impl Kind {
+    fn parse(string: &str) -> Kind {
+        match string {
+            "build" => Kind::Build,
+            "check" => Kind::Check,
+            "clippy" => Kind::Clippy,
+            "fix" => Kind::Fix,
+            "test" => Kind::Test,
+            "bench" => Kind::Bench,
+            "dist" => Kind::Dist,
+            "doc" => Kind::Doc,
+            "install" => Kind::Install,
+            "run" => Kind::Run,
+            other => panic!("unknown kind: {}", other),
+        }
+    }
+
+    fn as_str(&self) -> &'static str {
+        match self {
+            Kind::Build => "build",
+            Kind::Check => "check",
+            Kind::Clippy => "clippy",
+            Kind::Fix => "fix",
+            Kind::Test => "test",
+            Kind::Bench => "bench",
+            Kind::Dist => "dist",
+            Kind::Doc => "doc",
+            Kind::Install => "install",
+            Kind::Run => "run",
+        }
+    }
+}
+
 impl<'a> Builder<'a> {
     fn get_step_descriptions(kind: Kind) -> Vec<StepDescription> {
         macro_rules! describe {
             ($($rule:ty),+ $(,)?) => {{
-                vec![$(StepDescription::from::<$rule>()),+]
+                vec![$(StepDescription::from::<$rule>(kind)),+]
             }};
         }
         match kind {
@@ -540,8 +633,11 @@ impl<'a> Builder<'a> {
 
         let builder = Self::new_internal(build, kind, vec![]);
         let builder = &builder;
-        let mut should_run = ShouldRun::new(builder);
+        // The "build" kind here is just a placeholder, it will be replaced with something else in
+        // the following statement.
+        let mut should_run = ShouldRun::new(builder, Kind::Build);
         for desc in Builder::get_step_descriptions(builder.kind) {
+            should_run.kind = desc.kind;
             should_run = (desc.should_run)(should_run);
         }
         let mut help = String::from("Available paths:\n");
@@ -552,11 +648,11 @@ impl<'a> Builder<'a> {
             match pathset {
                 PathSet::Set(set) => {
                     for path in set {
-                        add_path(&path);
+                        add_path(&path.path);
                     }
                 }
                 PathSet::Suite(path) => {
-                    add_path(&path.join("..."));
+                    add_path(&path.path.join("..."));
                 }
             }
         }
@@ -1626,9 +1722,10 @@ impl<'a> Builder<'a> {
     pub(crate) fn ensure_if_default<T, S: Step<Output = Option<T>>>(
         &'a self,
         step: S,
+        kind: Kind,
     ) -> S::Output {
-        let desc = StepDescription::from::<S>();
-        let should_run = (desc.should_run)(ShouldRun::new(self));
+        let desc = StepDescription::from::<S>(kind);
+        let should_run = (desc.should_run)(ShouldRun::new(self, desc.kind));
 
         // Avoid running steps contained in --exclude
         for pathset in &should_run.paths {
@@ -1642,13 +1739,16 @@ impl<'a> Builder<'a> {
     }
 
     /// Checks if any of the "should_run" paths is in the `Builder` paths.
-    pub(crate) fn was_invoked_explicitly<S: Step>(&'a self) -> bool {
-        let desc = StepDescription::from::<S>();
-        let should_run = (desc.should_run)(ShouldRun::new(self));
+    pub(crate) fn was_invoked_explicitly<S: Step>(&'a self, kind: Kind) -> bool {
+        let desc = StepDescription::from::<S>(kind);
+        let should_run = (desc.should_run)(ShouldRun::new(self, desc.kind));
 
         for path in &self.paths {
-            if should_run.paths.iter().any(|s| s.has(path))
-                && !desc.is_excluded(self, &PathSet::Suite(path.clone()))
+            if should_run.paths.iter().any(|s| s.has(path, Some(desc.kind)))
+                && !desc.is_excluded(
+                    self,
+                    &PathSet::Suite(TaskPath { path: path.clone(), kind: Some(desc.kind.into()) }),
+                )
             {
                 return true;
             }

src/bootstrap/builder/tests.rs

@@ -499,7 +499,7 @@ mod dist {
         let host = TargetSelection::from_user("A");
 
         builder.run_step_descriptions(
-            &[StepDescription::from::<test::Crate>()],
+            &[StepDescription::from::<test::Crate>(Kind::Test)],
             &["library/std".into()],
         );
 
@@ -520,7 +520,7 @@ mod dist {
     #[test]
     fn test_exclude() {
         let mut config = configure(&["A"], &["A"]);
-        config.exclude = vec!["src/tools/tidy".into()];
+        config.exclude = vec![TaskPath::parse("src/tools/tidy")];
         config.cmd = Subcommand::Test {
             paths: Vec::new(),
             test_args: Vec::new(),

src/bootstrap/config.rs

@@ -12,6 +12,7 @@ use std::fs;
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
 
+use crate::builder::TaskPath;
 use crate::cache::{Interned, INTERNER};
 use crate::channel::GitInfo;
 pub use crate::flags::Subcommand;
@@ -62,7 +63,7 @@ pub struct Config {
     pub sanitizers: bool,
     pub profiler: bool,
     pub ignore_git: bool,
-    pub exclude: Vec<PathBuf>,
+    pub exclude: Vec<TaskPath>,
     pub include_default_paths: bool,
     pub rustc_error_format: Option<String>,
     pub json_output: bool,
@@ -635,7 +636,7 @@ impl Config {
         let flags = Flags::parse(&args);
 
         let mut config = Config::default_opts();
-        config.exclude = flags.exclude;
+        config.exclude = flags.exclude.into_iter().map(|path| TaskPath::parse(path)).collect();
         config.include_default_paths = flags.include_default_paths;
         config.rustc_error_format = flags.rustc_error_format;
         config.json_output = flags.json_output;