From a8516c052e35951f86ccdc0a2ee9ce670bd159c8 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Thu, 2 Jan 2025 18:24:28 +0300 Subject: [PATCH 1/6] refactor bootstrap path resolution Previously we removed paths as soon as we found the first intersection, which made it impossible to find other intersecting paths. This patch changes that by marking the intersecting paths instead, so we can collect them all and remove them together when needed. Signed-off-by: onur-ozkan --- src/bootstrap/src/core/builder/mod.rs | 37 +++++++++++++++++++++------ 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/src/bootstrap/src/core/builder/mod.rs b/src/bootstrap/src/core/builder/mod.rs index 30e42a5bfb78d..2a95301c4142f 100644 --- a/src/bootstrap/src/core/builder/mod.rs +++ b/src/bootstrap/src/core/builder/mod.rs @@ -3,7 +3,7 @@ mod cargo; use std::any::{Any, type_name}; use std::cell::{Cell, RefCell}; use std::collections::BTreeSet; -use std::fmt::{Debug, Write}; +use std::fmt::{self, Debug, Write}; use std::hash::Hash; use std::ops::Deref; use std::path::{Path, PathBuf}; @@ -271,12 +271,12 @@ impl PathSet { /// 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, module: Kind) -> PathSet { + fn intersection_removing_matches(&self, needles: &mut [CLIStepPath], module: Kind) -> PathSet { let mut check = |p| { - for (i, n) in needles.iter().enumerate() { - let matched = Self::check(p, n, module); + for n in needles.iter_mut() { + let matched = Self::check(p, &n.path, module); if matched { - needles.remove(i); + n.will_be_executed = true; return true; } } @@ -361,6 +361,24 @@ fn remap_paths(paths: &mut Vec) { paths.append(&mut add); } +#[derive(Clone, PartialEq)] +struct CLIStepPath { + path: PathBuf, + will_be_executed: bool, +} + +impl Debug for CLIStepPath { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.path.display()) + } +} + +impl From for CLIStepPath { + fn from(path: PathBuf) -> Self { + Self { path, will_be_executed: false } + } +} + impl StepDescription { fn from(kind: Kind) -> StepDescription { StepDescription { @@ -478,7 +496,8 @@ impl StepDescription { return; } - let mut path_lookup: Vec<(PathBuf, bool)> = + let mut paths: Vec = paths.into_iter().map(|p| p.into()).collect(); + let mut path_lookup: Vec<(CLIStepPath, bool)> = paths.clone().into_iter().map(|p| (p, false)).collect(); // List of `(usize, &StepDescription, Vec)` where `usize` is the closest index of a path @@ -518,8 +537,10 @@ impl StepDescription { } } + paths.retain(|p| !p.will_be_executed); + if !paths.is_empty() { - eprintln!("ERROR: no `{}` rules matched {:?}", builder.kind.as_str(), paths,); + 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() @@ -682,7 +703,7 @@ impl<'a> ShouldRun<'a> { /// (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, + paths: &mut [CLIStepPath], kind: Kind, ) -> Vec { let mut sets = vec![]; From 00cd94370959ece80f675249ba6f4235a4ff8c3c Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Thu, 2 Jan 2025 18:39:25 +0300 Subject: [PATCH 2/6] adapt bootstrap tests to the new path resolution logic Signed-off-by: onur-ozkan --- src/bootstrap/src/core/build_steps/compile.rs | 2 +- src/bootstrap/src/core/build_steps/doc.rs | 5 ++++- src/bootstrap/src/core/builder/mod.rs | 8 ++++++++ src/bootstrap/src/core/builder/tests.rs | 12 ++++++++---- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs index ca337aa9f4c32..148b96181d1d2 100644 --- a/src/bootstrap/src/core/build_steps/compile.rs +++ b/src/bootstrap/src/core/build_steps/compile.rs @@ -93,7 +93,7 @@ impl Step for Std { const DEFAULT: bool = true; fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { - run.crate_or_deps("sysroot").path("library") + run.crate_or_deps("sysroot").path("library").alias("core") } fn make_run(run: RunConfig<'_>) { diff --git a/src/bootstrap/src/core/build_steps/doc.rs b/src/bootstrap/src/core/build_steps/doc.rs index 1e9f7cbd9b4ca..75edc8ff78128 100644 --- a/src/bootstrap/src/core/build_steps/doc.rs +++ b/src/bootstrap/src/core/build_steps/doc.rs @@ -574,7 +574,10 @@ impl Step for Std { fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { let builder = run.builder; - run.crate_or_deps("sysroot").path("library").default_condition(builder.config.docs) + run.crate_or_deps("sysroot") + .path("library") + .alias("core") + .default_condition(builder.config.docs) } fn make_run(run: RunConfig<'_>) { diff --git a/src/bootstrap/src/core/builder/mod.rs b/src/bootstrap/src/core/builder/mod.rs index 2a95301c4142f..e9a31500ad128 100644 --- a/src/bootstrap/src/core/builder/mod.rs +++ b/src/bootstrap/src/core/builder/mod.rs @@ -367,6 +367,14 @@ struct CLIStepPath { will_be_executed: bool, } +#[cfg(test)] +impl CLIStepPath { + fn will_be_executed(mut self, will_be_executed: bool) -> Self { + self.will_be_executed = will_be_executed; + self + } +} + impl Debug for CLIStepPath { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}", self.path.display()) diff --git a/src/bootstrap/src/core/builder/tests.rs b/src/bootstrap/src/core/builder/tests.rs index 21694cf46fe21..fd84c2667eeed 100644 --- a/src/bootstrap/src/core/builder/tests.rs +++ b/src/bootstrap/src/core/builder/tests.rs @@ -108,13 +108,17 @@ fn test_intersection() { }; let library_set = set(&["library/core", "library/alloc", "library/std"]); let mut command_paths = vec![ - PathBuf::from("library/core"), - PathBuf::from("library/alloc"), - PathBuf::from("library/stdarch"), + CLIStepPath::from(PathBuf::from("library/core")), + CLIStepPath::from(PathBuf::from("library/alloc")), + CLIStepPath::from(PathBuf::from("library/stdarch")), ]; let subset = library_set.intersection_removing_matches(&mut command_paths, Kind::Build); assert_eq!(subset, set(&["library/core", "library/alloc"]),); - assert_eq!(command_paths, vec![PathBuf::from("library/stdarch")]); + assert_eq!(command_paths, vec![ + CLIStepPath::from(PathBuf::from("library/core")).will_be_executed(true), + CLIStepPath::from(PathBuf::from("library/alloc")).will_be_executed(true), + CLIStepPath::from(PathBuf::from("library/stdarch")).will_be_executed(false), + ]); } #[test] From c367c62b27b8f75b04d59d34727066c080472db8 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Thu, 2 Jan 2025 18:41:16 +0300 Subject: [PATCH 3/6] revert step order from #134919 Signed-off-by: onur-ozkan --- src/bootstrap/src/core/builder/mod.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/bootstrap/src/core/builder/mod.rs b/src/bootstrap/src/core/builder/mod.rs index e9a31500ad128..e04be1ae69743 100644 --- a/src/bootstrap/src/core/builder/mod.rs +++ b/src/bootstrap/src/core/builder/mod.rs @@ -958,14 +958,10 @@ impl<'a> Builder<'a> { test::Rustdoc, test::CoverageRunRustdoc, test::Pretty, - test::Crate, - test::CrateLibrustc, - // The cranelift and gcc tests need to be listed after the - // compiler unit tests (CrateLibrustc) so that they don't - // hijack the whole `compiler` directory during path matching. - // test::CodegenCranelift, test::CodegenGCC, + test::Crate, + test::CrateLibrustc, test::CrateRustdoc, test::CrateRustdocJsonTypes, test::CrateBootstrap, From be2f75f3b7c226c6dd025fe43c8df8d8d2477aac Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 3 Jan 2025 08:57:58 +0300 Subject: [PATCH 4/6] Revert "bootstrap: temporarily flip `compile::Rustc` vs `compile::Assemble`" This reverts commit 552c1f5f45ec8b8cb5c9427754a7c3d16ca9f741. --- src/bootstrap/src/core/builder/mod.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/bootstrap/src/core/builder/mod.rs b/src/bootstrap/src/core/builder/mod.rs index e04be1ae69743..3bfa9071e70e0 100644 --- a/src/bootstrap/src/core/builder/mod.rs +++ b/src/bootstrap/src/core/builder/mod.rs @@ -854,12 +854,8 @@ impl<'a> Builder<'a> { match kind { Kind::Build => describe!( compile::Std, - // FIXME(#135022): `compile::Assemble` **must** come before `compile::Rustc` after - // `PathSet` also permits prefix-matching, because `compile::Rustc` can consume the - // `"compiler"` path filter first, causing `compile::Assemble` to no longer run when - // the user writes `./x build compiler --stage 0`. - compile::Assemble, compile::Rustc, + compile::Assemble, compile::CodegenBackend, compile::StartupObjects, tool::BuildManifest, From 3807440a000d07da1c589887f847efdecee6b429 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 3 Jan 2025 09:54:36 +0300 Subject: [PATCH 5/6] avoid early return to handle all paths Signed-off-by: onur-ozkan --- src/bootstrap/src/core/builder/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/bootstrap/src/core/builder/mod.rs b/src/bootstrap/src/core/builder/mod.rs index 3bfa9071e70e0..04d51fab5d521 100644 --- a/src/bootstrap/src/core/builder/mod.rs +++ b/src/bootstrap/src/core/builder/mod.rs @@ -273,14 +273,15 @@ impl PathSet { /// See `tests.rs` for examples. fn intersection_removing_matches(&self, needles: &mut [CLIStepPath], module: Kind) -> PathSet { let mut check = |p| { + let mut result = false; for n in needles.iter_mut() { let matched = Self::check(p, &n.path, module); if matched { n.will_be_executed = true; - return true; + result = true; } } - false + result }; match self { PathSet::Set(set) => PathSet::Set(set.iter().filter(|&p| check(p)).cloned().collect()), From baa7fcec85e1939b38e84d885e746df08d2f5bb0 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 3 Jan 2025 10:01:22 +0300 Subject: [PATCH 6/6] add coverage for multiple paths Signed-off-by: onur-ozkan --- src/bootstrap/src/core/builder/tests.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/bootstrap/src/core/builder/tests.rs b/src/bootstrap/src/core/builder/tests.rs index fd84c2667eeed..5769198afac64 100644 --- a/src/bootstrap/src/core/builder/tests.rs +++ b/src/bootstrap/src/core/builder/tests.rs @@ -121,6 +121,26 @@ fn test_intersection() { ]); } +#[test] +fn test_resolve_parent_and_subpaths() { + let set = |paths: &[&str]| { + PathSet::Set(paths.into_iter().map(|p| TaskPath { path: p.into(), kind: None }).collect()) + }; + + let mut command_paths = vec![ + CLIStepPath::from(PathBuf::from("src/tools/miri")), + CLIStepPath::from(PathBuf::from("src/tools/miri/cargo-miri")), + ]; + + let library_set = set(&["src/tools/miri", "src/tools/miri/cargo-miri"]); + library_set.intersection_removing_matches(&mut command_paths, Kind::Build); + + assert_eq!(command_paths, vec![ + CLIStepPath::from(PathBuf::from("src/tools/miri")).will_be_executed(true), + CLIStepPath::from(PathBuf::from("src/tools/miri/cargo-miri")).will_be_executed(true), + ]); +} + #[test] fn validate_path_remap() { let build = Build::new(configure("test", &[TEST_TRIPLE_1], &[TEST_TRIPLE_1]));