From b27d59d083a97e7253bcc8a040bc606ae0725fc4 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Wed, 15 Dec 2021 12:50:06 +0100 Subject: [PATCH 1/2] replace paths in PathSet with a dedicated TaskPath struct --- src/bootstrap/builder.rs | 42 ++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index 6ccf8b1d5221c..c20177c92309d 100644 --- a/src/bootstrap/builder.rs +++ b/src/bootstrap/builder.rs @@ -107,6 +107,18 @@ struct StepDescription { name: &'static str, } +#[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq)] +pub struct TaskPath { + pub path: PathBuf, + pub module: Option, +} + +impl TaskPath { + pub fn parse(path: impl Into) -> TaskPath { + TaskPath { path: path.into(), module: None } + } +} + /// Collection of paths used to match a task rule. #[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq)] pub enum PathSet { @@ -115,14 +127,14 @@ 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), + Set(BTreeSet), /// 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 { @@ -132,21 +144,23 @@ impl PathSet { fn one>(path: P) -> PathSet { let mut set = BTreeSet::new(); - set.insert(path.into()); + set.insert(TaskPath::parse(path)); PathSet::Set(set) } fn has(&self, needle: &Path) -> bool { 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(|p| p.path.ends_with(needle)), + PathSet::Suite(suite) => suite.path.ends_with(needle), } } 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(), } } } @@ -293,7 +307,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::parse(path)); } self.paths.insert(PathSet::Set(set)); self @@ -318,19 +332,19 @@ 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::parse(p)).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::parse(suite))); self } @@ -552,11 +566,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("...")); } } } @@ -1648,7 +1662,7 @@ impl<'a> Builder<'a> { for path in &self.paths { if should_run.paths.iter().any(|s| s.has(path)) - && !desc.is_excluded(self, &PathSet::Suite(path.clone())) + && !desc.is_excluded(self, &PathSet::Suite(TaskPath::parse(path))) { return true; } From b3ad40532db231cd3ca55bc37047aa50fb1a1a71 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Wed, 15 Dec 2021 12:51:26 +0100 Subject: [PATCH 2/2] allow excluding paths only from a single module x.py has support for excluding some steps from the 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 execution would fail, as that would not only exclude running the tests for the standard library, it would also exclude generating its documentation (breaking linkchecker). This commit adds support for an optional module annotation in --exclude paths, allowing the user to choose which module to exclude from: ./x.py test --exclude test::library/std This maintains backward compatibility, but also allows for more ganular exclusion. More examples on how this works: | `--exclude` | Docs | Tests | | ------------------- | ------- | ------- | | `library/std` | Skipped | Skipped | | `doc::library/std` | Skipped | Run | | `test::library/std` | Run | Skipped | Note that the new behavior only works in the `--exclude` flag, and not in other x.py arguments or flags yet. --- src/bootstrap/builder.rs | 148 ++++++++++++++++++++++++++------- src/bootstrap/builder/tests.rs | 4 +- src/bootstrap/config.rs | 5 +- src/bootstrap/dist.rs | 4 +- src/bootstrap/doc.rs | 8 +- 5 files changed, 128 insertions(+), 41 deletions(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index c20177c92309d..5cab3e8be1039 100644 --- a/src/bootstrap/builder.rs +++ b/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,17 +105,43 @@ struct StepDescription { should_run: fn(ShouldRun<'_>) -> ShouldRun<'_>, make_run: fn(RunConfig<'_>), name: &'static str, + kind: Kind, } -#[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq)] +#[derive(Clone, PartialOrd, Ord, PartialEq, Eq)] pub struct TaskPath { pub path: PathBuf, - pub module: Option, + pub kind: Option, } impl TaskPath { pub fn parse(path: impl Into) -> TaskPath { - TaskPath { path: path.into(), module: None } + 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()) } } @@ -142,16 +168,24 @@ impl PathSet { PathSet::Set(BTreeSet::new()) } - fn one>(path: P) -> PathSet { + fn one>(path: P, kind: Kind) -> PathSet { let mut set = BTreeSet::new(); - set.insert(TaskPath::parse(path)); + 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) -> 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.path.ends_with(needle)), - PathSet::Suite(suite) => suite.path.ends_with(needle), + PathSet::Set(set) => set.iter().any(check), + PathSet::Suite(suite) => check(suite), } } @@ -166,13 +200,14 @@ impl PathSet { } impl StepDescription { - fn from() -> StepDescription { + fn from(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::(), + kind, } } @@ -191,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; } @@ -206,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::>(); + let should_runs = v + .iter() + .map(|desc| (desc.should_run)(ShouldRun::new(builder, desc.kind))) + .collect::>(); // sanity checks on rules for (desc, should_run) in v.iter().zip(&should_runs) { @@ -240,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); } @@ -260,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, @@ -269,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 } @@ -307,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(TaskPath::parse(path)); + set.insert(TaskPath { path, kind: Some(self.kind) }); } self.paths.insert(PathSet::Set(set)); self @@ -320,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 } @@ -332,7 +372,12 @@ 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(|p| TaskPath::parse(p)).collect())); + self.paths.insert(PathSet::Set( + paths + .iter() + .map(|p| TaskPath { path: p.into(), kind: Some(self.kind.into()) }) + .collect(), + )); self } @@ -344,7 +389,8 @@ impl<'a> ShouldRun<'a> { } pub fn suite_path(mut self, suite: &str) -> Self { - self.paths.insert(PathSet::Suite(TaskPath::parse(suite))); + self.paths + .insert(PathSet::Suite(TaskPath { path: suite.into(), kind: Some(self.kind.into()) })); self } @@ -354,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, @@ -373,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 { macro_rules! describe { ($($rule:ty),+ $(,)?) => {{ - vec![$(StepDescription::from::<$rule>()),+] + vec![$(StepDescription::from::<$rule>(kind)),+] }}; } match kind { @@ -554,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"); @@ -1640,9 +1722,10 @@ impl<'a> Builder<'a> { pub(crate) fn ensure_if_default>>( &'a self, step: S, + kind: Kind, ) -> S::Output { - let desc = StepDescription::from::(); - let should_run = (desc.should_run)(ShouldRun::new(self)); + let desc = StepDescription::from::(kind); + let should_run = (desc.should_run)(ShouldRun::new(self, desc.kind)); // Avoid running steps contained in --exclude for pathset in &should_run.paths { @@ -1656,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(&'a self) -> bool { - let desc = StepDescription::from::(); - let should_run = (desc.should_run)(ShouldRun::new(self)); + pub(crate) fn was_invoked_explicitly(&'a self, kind: Kind) -> bool { + let desc = StepDescription::from::(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(TaskPath::parse(path))) + 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; } diff --git a/src/bootstrap/builder/tests.rs b/src/bootstrap/builder/tests.rs index bb3ea04d4ace9..bc71034496968 100644 --- a/src/bootstrap/builder/tests.rs +++ b/src/bootstrap/builder/tests.rs @@ -499,7 +499,7 @@ mod dist { let host = TargetSelection::from_user("A"); builder.run_step_descriptions( - &[StepDescription::from::()], + &[StepDescription::from::(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(), diff --git a/src/bootstrap/config.rs b/src/bootstrap/config.rs index 5af9248583cae..683cfc630e771 100644 --- a/src/bootstrap/config.rs +++ b/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, + pub exclude: Vec, pub include_default_paths: bool, pub rustc_error_format: Option, 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; diff --git a/src/bootstrap/dist.rs b/src/bootstrap/dist.rs index a46a4e63714c2..66b63cd1278c5 100644 --- a/src/bootstrap/dist.rs +++ b/src/bootstrap/dist.rs @@ -16,7 +16,7 @@ use std::process::Command; use build_helper::{output, t}; -use crate::builder::{Builder, RunConfig, ShouldRun, Step}; +use crate::builder::{Builder, Kind, RunConfig, ShouldRun, Step}; use crate::cache::{Interned, INTERNER}; use crate::compile; use crate::config::TargetSelection; @@ -1368,7 +1368,7 @@ impl Step for Extended { let mut built_tools = HashSet::new(); macro_rules! add_component { ($name:expr => $step:expr) => { - if let Some(tarball) = builder.ensure_if_default($step) { + if let Some(tarball) = builder.ensure_if_default($step, Kind::Dist) { tarballs.push(tarball); built_tools.insert($name); } diff --git a/src/bootstrap/doc.rs b/src/bootstrap/doc.rs index f0f31c447bda4..23b5ddcd47a0e 100644 --- a/src/bootstrap/doc.rs +++ b/src/bootstrap/doc.rs @@ -15,7 +15,7 @@ use std::path::{Path, PathBuf}; use crate::Mode; use build_helper::{t, up_to_date}; -use crate::builder::{Builder, Compiler, RunConfig, ShouldRun, Step}; +use crate::builder::{Builder, Compiler, Kind, RunConfig, ShouldRun, Step}; use crate::cache::{Interned, INTERNER}; use crate::compile; use crate::config::{Config, TargetSelection}; @@ -240,7 +240,7 @@ impl Step for TheBook { invoke_rustdoc(builder, compiler, target, path); } - if builder.was_invoked_explicitly::() { + if builder.was_invoked_explicitly::(Kind::Doc) { let out = builder.doc_out(target); let index = out.join("book").join("index.html"); open(builder, &index); @@ -400,7 +400,7 @@ impl Step for Standalone { // We open doc/index.html as the default if invoked as `x.py doc --open` // with no particular explicit doc requested (e.g. library/core). - if builder.paths.is_empty() || builder.was_invoked_explicitly::() { + if builder.paths.is_empty() || builder.was_invoked_explicitly::(Kind::Doc) { let index = out.join("index.html"); open(builder, &index); } @@ -902,7 +902,7 @@ impl Step for RustcBook { name: INTERNER.intern_str("rustc"), src: INTERNER.intern_path(out_base), }); - if builder.was_invoked_explicitly::() { + if builder.was_invoked_explicitly::(Kind::Doc) { let out = builder.doc_out(self.target); let index = out.join("rustc").join("index.html"); open(builder, &index);