From 91d064b44286d87723fc8ef2ae9d8e3d047d12f2 Mon Sep 17 00:00:00 2001 From: Jieyou Xu Date: Tue, 8 Jul 2025 17:52:00 +0800 Subject: [PATCH] Remove current implementation of `./x suggest` This is quite a bit of implementation complexity, yet it is quite broken, and we don't have the maintenance bandwidth to address. Remove the current implementation if only to reduce bootstrap's implementation complexity; the `suggest` flow comes with its own set of hacks. --- Cargo.lock | 8 -- Cargo.toml | 1 - src/bootstrap/src/core/build_steps/mod.rs | 1 - src/bootstrap/src/core/build_steps/suggest.rs | 68 ------------- src/bootstrap/src/core/build_steps/test.rs | 9 +- src/bootstrap/src/core/build_steps/tool.rs | 1 - src/bootstrap/src/core/builder/cargo.rs | 2 +- src/bootstrap/src/core/builder/mod.rs | 6 +- src/bootstrap/src/core/config/config.rs | 2 - src/bootstrap/src/core/config/flags.rs | 8 -- src/bootstrap/src/core/sanity.rs | 1 - src/bootstrap/src/lib.rs | 11 +-- src/bootstrap/src/utils/cc_detect.rs | 1 - src/bootstrap/src/utils/metrics.rs | 2 +- src/tools/suggest-tests/Cargo.toml | 8 -- .../suggest-tests/src/dynamic_suggestions.rs | 32 ------- src/tools/suggest-tests/src/lib.rs | 96 ------------------- src/tools/suggest-tests/src/main.rs | 40 -------- .../suggest-tests/src/static_suggestions.rs | 40 -------- src/tools/suggest-tests/src/tests.rs | 21 ---- 20 files changed, 10 insertions(+), 348 deletions(-) delete mode 100644 src/bootstrap/src/core/build_steps/suggest.rs delete mode 100644 src/tools/suggest-tests/Cargo.toml delete mode 100644 src/tools/suggest-tests/src/dynamic_suggestions.rs delete mode 100644 src/tools/suggest-tests/src/lib.rs delete mode 100644 src/tools/suggest-tests/src/main.rs delete mode 100644 src/tools/suggest-tests/src/static_suggestions.rs delete mode 100644 src/tools/suggest-tests/src/tests.rs diff --git a/Cargo.lock b/Cargo.lock index a6c36b4bdfa7..ff2b21c64b0e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5188,14 +5188,6 @@ dependencies = [ "syn 1.0.109", ] -[[package]] -name = "suggest-tests" -version = "0.1.0" -dependencies = [ - "build_helper", - "glob", -] - [[package]] name = "syn" version = "1.0.109" diff --git a/Cargo.toml b/Cargo.toml index 6d3425f4115a..67c7a9d67edc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,7 +39,6 @@ members = [ "src/tools/rustdoc-gui-test", "src/tools/rustdoc-themes", "src/tools/rustfmt", - "src/tools/suggest-tests", "src/tools/test-float-parse", "src/tools/tidy", "src/tools/tier-check", diff --git a/src/bootstrap/src/core/build_steps/mod.rs b/src/bootstrap/src/core/build_steps/mod.rs index fcb6abea4347..c2ad9a4df597 100644 --- a/src/bootstrap/src/core/build_steps/mod.rs +++ b/src/bootstrap/src/core/build_steps/mod.rs @@ -11,7 +11,6 @@ pub(crate) mod llvm; pub(crate) mod perf; pub(crate) mod run; pub(crate) mod setup; -pub(crate) mod suggest; pub(crate) mod synthetic_targets; pub(crate) mod test; pub(crate) mod tool; diff --git a/src/bootstrap/src/core/build_steps/suggest.rs b/src/bootstrap/src/core/build_steps/suggest.rs deleted file mode 100644 index fd4918961adb..000000000000 --- a/src/bootstrap/src/core/build_steps/suggest.rs +++ /dev/null @@ -1,68 +0,0 @@ -//! Attempt to magically identify good tests to run - -use std::path::PathBuf; -use std::str::FromStr; - -use clap::Parser; - -use crate::core::build_steps::tool::Tool; -use crate::core::builder::Builder; - -/// Suggests a list of possible `x.py` commands to run based on modified files in branch. -pub fn suggest(builder: &Builder<'_>, run: bool) { - let git_config = builder.config.git_config(); - let suggestions = builder - .tool_cmd(Tool::SuggestTests) - .env("SUGGEST_TESTS_NIGHTLY_BRANCH", git_config.nightly_branch) - .env("SUGGEST_TESTS_MERGE_COMMIT_EMAIL", git_config.git_merge_commit_email) - .run_capture_stdout(builder) - .stdout(); - - let suggestions = suggestions - .lines() - .map(|line| { - let mut sections = line.split_ascii_whitespace(); - - // this code expects one suggestion per line in the following format: - // {some number of flags} [optional stage number] - let cmd = sections.next().unwrap(); - let stage = sections.next_back().and_then(|s| str::parse(s).ok()); - let paths: Vec = sections.map(|p| PathBuf::from_str(p).unwrap()).collect(); - - (cmd, stage, paths) - }) - .collect::>(); - - if !suggestions.is_empty() { - println!("==== SUGGESTIONS ===="); - for sug in &suggestions { - print!("x {} ", sug.0); - if let Some(stage) = sug.1 { - print!("--stage {stage} "); - } - - for path in &sug.2 { - print!("{} ", path.display()); - } - println!(); - } - println!("====================="); - } else { - println!("No suggestions found!"); - return; - } - - if run { - for sug in suggestions { - let mut build: crate::Build = builder.build.clone(); - build.config.paths = sug.2; - build.config.cmd = crate::core::config::flags::Flags::parse_from(["x.py", sug.0]).cmd; - if let Some(stage) = sug.1 { - build.config.stage = stage; - } - build.build(); - } - } else { - println!("HELP: consider using the `--run` flag to automatically run suggested tests"); - } -} diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 757eac1475c1..9e7ea5c115f1 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -47,12 +47,11 @@ impl Step for CrateBootstrap { const DEFAULT: bool = true; fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { - // This step is responsible for several different tool paths. By default - // it will test all of them, but requesting specific tools on the - // command-line (e.g. `./x test suggest-tests`) will test only the - // specified tools. + // This step is responsible for several different tool paths. + // + // By default, it will test all of them, but requesting specific tools on the command-line + // (e.g. `./x test src/tools/coverage-dump`) will test only the specified tools. run.path("src/tools/jsondoclint") - .path("src/tools/suggest-tests") .path("src/tools/replace-version-placeholder") .path("src/tools/coverage-dump") // We want `./x test tidy` to _run_ the tidy tool, not its tests. diff --git a/src/bootstrap/src/core/build_steps/tool.rs b/src/bootstrap/src/core/build_steps/tool.rs index 814a44b9a13b..1c994b0ccfc8 100644 --- a/src/bootstrap/src/core/build_steps/tool.rs +++ b/src/bootstrap/src/core/build_steps/tool.rs @@ -517,7 +517,6 @@ bootstrap_tool!( ReplaceVersionPlaceholder, "src/tools/replace-version-placeholder", "replace-version-placeholder"; CollectLicenseMetadata, "src/tools/collect-license-metadata", "collect-license-metadata"; GenerateCopyright, "src/tools/generate-copyright", "generate-copyright"; - SuggestTests, "src/tools/suggest-tests", "suggest-tests"; GenerateWindowsSys, "src/tools/generate-windows-sys", "generate-windows-sys"; // rustdoc-gui-test has a crate dependency on compiletest, so it needs the same unstable features. RustdocGUITest, "src/tools/rustdoc-gui-test", "rustdoc-gui-test", is_unstable_tool = true, allow_features = COMPILETEST_ALLOW_FEATURES; diff --git a/src/bootstrap/src/core/builder/cargo.rs b/src/bootstrap/src/core/builder/cargo.rs index d5a290d804c5..a3b471ca56eb 100644 --- a/src/bootstrap/src/core/builder/cargo.rs +++ b/src/bootstrap/src/core/builder/cargo.rs @@ -113,7 +113,7 @@ impl Cargo { match cmd_kind { // No need to configure the target linker for these command types. - Kind::Clean | Kind::Check | Kind::Suggest | Kind::Format | Kind::Setup => {} + Kind::Clean | Kind::Check | Kind::Format | Kind::Setup => {} _ => { cargo.configure_linker(builder, mode); } diff --git a/src/bootstrap/src/core/builder/mod.rs b/src/bootstrap/src/core/builder/mod.rs index 7115c5a1cbe7..1b75d00b30e4 100644 --- a/src/bootstrap/src/core/builder/mod.rs +++ b/src/bootstrap/src/core/builder/mod.rs @@ -845,7 +845,6 @@ pub enum Kind { #[value(alias = "r")] Run, Setup, - Suggest, Vendor, Perf, } @@ -869,7 +868,6 @@ impl Kind { Kind::Install => "install", Kind::Run => "run", Kind::Setup => "setup", - Kind::Suggest => "suggest", Kind::Vendor => "vendor", Kind::Perf => "perf", } @@ -881,7 +879,6 @@ impl Kind { Kind::Bench => "Benchmarking", Kind::Doc => "Documenting", Kind::Run => "Running", - Kind::Suggest => "Suggesting", Kind::Clippy => "Linting", Kind::Perf => "Profiling & benchmarking", _ => { @@ -1201,7 +1198,7 @@ impl<'a> Builder<'a> { Kind::Clean => describe!(clean::CleanAll, clean::Rustc, clean::Std), Kind::Vendor => describe!(vendor::Vendor), // special-cased in Build::build() - Kind::Format | Kind::Suggest | Kind::Perf => vec![], + Kind::Format | Kind::Perf => vec![], Kind::MiriTest | Kind::MiriSetup => unreachable!(), } } @@ -1269,7 +1266,6 @@ impl<'a> Builder<'a> { Subcommand::Run { .. } => (Kind::Run, &paths[..]), Subcommand::Clean { .. } => (Kind::Clean, &paths[..]), Subcommand::Format { .. } => (Kind::Format, &[][..]), - Subcommand::Suggest { .. } => (Kind::Suggest, &[][..]), Subcommand::Setup { profile: ref path } => ( Kind::Setup, path.as_ref().map_or([].as_slice(), |path| std::slice::from_ref(path)), diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index 0039d44785c3..28958b60fc32 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -1050,7 +1050,6 @@ impl Config { | Subcommand::Run { .. } | Subcommand::Setup { .. } | Subcommand::Format { .. } - | Subcommand::Suggest { .. } | Subcommand::Vendor { .. } => flags_stage.unwrap_or(0), }; @@ -1098,7 +1097,6 @@ impl Config { | Subcommand::Run { .. } | Subcommand::Setup { .. } | Subcommand::Format { .. } - | Subcommand::Suggest { .. } | Subcommand::Vendor { .. } | Subcommand::Perf { .. } => {} } diff --git a/src/bootstrap/src/core/config/flags.rs b/src/bootstrap/src/core/config/flags.rs index 155b6f587589..1547ca444943 100644 --- a/src/bootstrap/src/core/config/flags.rs +++ b/src/bootstrap/src/core/config/flags.rs @@ -481,13 +481,6 @@ Arguments: #[arg(value_name = "|hook|editor|link")] profile: Option, }, - /// Suggest a subset of tests to run, based on modified files - #[command(long_about = "\n")] - Suggest { - /// run suggested tests - #[arg(long)] - run: bool, - }, /// Vendor dependencies Vendor { /// Additional `Cargo.toml` to sync and vendor @@ -518,7 +511,6 @@ impl Subcommand { Subcommand::Install => Kind::Install, Subcommand::Run { .. } => Kind::Run, Subcommand::Setup { .. } => Kind::Setup, - Subcommand::Suggest { .. } => Kind::Suggest, Subcommand::Vendor { .. } => Kind::Vendor, Subcommand::Perf { .. } => Kind::Perf, } diff --git a/src/bootstrap/src/core/sanity.rs b/src/bootstrap/src/core/sanity.rs index f2119e84cce7..b39d464493e5 100644 --- a/src/bootstrap/src/core/sanity.rs +++ b/src/bootstrap/src/core/sanity.rs @@ -216,7 +216,6 @@ than building it. build.config.cmd, Subcommand::Clean { .. } | Subcommand::Check { .. } - | Subcommand::Suggest { .. } | Subcommand::Format { .. } | Subcommand::Setup { .. } ); diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index 66a164703b7f..44be51815c7c 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -651,11 +651,9 @@ impl Build { // Handle hard-coded subcommands. { #[cfg(feature = "tracing")] - let _hardcoded_span = span!( - tracing::Level::DEBUG, - "handling hardcoded subcommands (Format, Suggest, Perf)" - ) - .entered(); + let _hardcoded_span = + span!(tracing::Level::DEBUG, "handling hardcoded subcommands (Format, Perf)") + .entered(); match &self.config.cmd { Subcommand::Format { check, all } => { @@ -666,9 +664,6 @@ impl Build { &self.config.paths, ); } - Subcommand::Suggest { run } => { - return core::build_steps::suggest::suggest(&builder::Builder::new(self), *run); - } Subcommand::Perf(args) => { return core::build_steps::perf::perf(&builder::Builder::new(self), args); } diff --git a/src/bootstrap/src/utils/cc_detect.rs b/src/bootstrap/src/utils/cc_detect.rs index dcafeb80f90c..778591189a3c 100644 --- a/src/bootstrap/src/utils/cc_detect.rs +++ b/src/bootstrap/src/utils/cc_detect.rs @@ -67,7 +67,6 @@ pub fn fill_compilers(build: &mut Build) { // We don't need to check cross targets for these commands. crate::Subcommand::Clean { .. } | crate::Subcommand::Check { .. } - | crate::Subcommand::Suggest { .. } | crate::Subcommand::Format { .. } | crate::Subcommand::Setup { .. } => { build.hosts.iter().cloned().chain(iter::once(build.host_target)).collect() diff --git a/src/bootstrap/src/utils/metrics.rs b/src/bootstrap/src/utils/metrics.rs index 862c44496241..9b1ccc32cb61 100644 --- a/src/bootstrap/src/utils/metrics.rs +++ b/src/bootstrap/src/utils/metrics.rs @@ -43,7 +43,7 @@ pub(crate) struct BuildMetrics { state: RefCell, } -/// NOTE: this isn't really cloning anything, but `x suggest` doesn't need metrics so this is probably ok. +// NOTE: this isn't really cloning anything, but necessary for `Build: Clone`. impl Clone for BuildMetrics { fn clone(&self) -> Self { Self::init() diff --git a/src/tools/suggest-tests/Cargo.toml b/src/tools/suggest-tests/Cargo.toml deleted file mode 100644 index d6f86078d7ef..000000000000 --- a/src/tools/suggest-tests/Cargo.toml +++ /dev/null @@ -1,8 +0,0 @@ -[package] -name = "suggest-tests" -version = "0.1.0" -edition = "2021" - -[dependencies] -glob = "0.3.0" -build_helper = { version = "0.1.0", path = "../../build_helper" } diff --git a/src/tools/suggest-tests/src/dynamic_suggestions.rs b/src/tools/suggest-tests/src/dynamic_suggestions.rs deleted file mode 100644 index f09720f1c91f..000000000000 --- a/src/tools/suggest-tests/src/dynamic_suggestions.rs +++ /dev/null @@ -1,32 +0,0 @@ -use std::path::Path; - -use crate::Suggestion; - -type DynamicSuggestion = fn(&Path) -> Vec; - -pub(crate) const DYNAMIC_SUGGESTIONS: &[DynamicSuggestion] = &[ - |path: &Path| -> Vec { - if path.starts_with("compiler/") || path.starts_with("library/") { - let path = path.components().take(2).collect::>(); - - vec![Suggestion::with_single_path( - "test", - None, - &format!( - "{}/{}", - path[0].as_os_str().to_str().unwrap(), - path[1].as_os_str().to_str().unwrap() - ), - )] - } else { - Vec::new() - } - }, - |path: &Path| -> Vec { - if path.starts_with("compiler/rustc_pattern_analysis") { - vec![Suggestion::new("test", None, &["tests/ui", "--test-args", "pattern"])] - } else { - Vec::new() - } - }, -]; diff --git a/src/tools/suggest-tests/src/lib.rs b/src/tools/suggest-tests/src/lib.rs deleted file mode 100644 index cc1288c6b72e..000000000000 --- a/src/tools/suggest-tests/src/lib.rs +++ /dev/null @@ -1,96 +0,0 @@ -use std::fmt::{self, Display}; -use std::path::Path; - -use dynamic_suggestions::DYNAMIC_SUGGESTIONS; -use glob::Pattern; -use static_suggestions::static_suggestions; - -mod dynamic_suggestions; -mod static_suggestions; - -#[cfg(test)] -mod tests; - -macro_rules! sug { - ($cmd:expr) => { - Suggestion::new($cmd, None, &[]) - }; - - ($cmd:expr, $paths:expr) => { - Suggestion::new($cmd, None, $paths.as_slice()) - }; - - ($cmd:expr, $stage:expr, $paths:expr) => { - Suggestion::new($cmd, Some($stage), $paths.as_slice()) - }; -} - -pub(crate) use sug; - -pub fn get_suggestions>(modified_files: &[T]) -> Vec { - let mut suggestions = Vec::new(); - - // static suggestions - for (globs, sugs) in static_suggestions().iter() { - let globs = globs - .iter() - .map(|glob| Pattern::new(glob).expect("Found invalid glob pattern!")) - .collect::>(); - let matches_some_glob = |file: &str| globs.iter().any(|glob| glob.matches(file)); - - if modified_files.iter().map(AsRef::as_ref).any(matches_some_glob) { - suggestions.extend_from_slice(sugs); - } - } - - // dynamic suggestions - for sug in DYNAMIC_SUGGESTIONS { - for file in modified_files { - let sugs = sug(Path::new(file.as_ref())); - - suggestions.extend_from_slice(&sugs); - } - } - - suggestions.sort(); - suggestions.dedup(); - - suggestions -} - -#[derive(Clone, PartialOrd, Ord, PartialEq, Eq, Debug)] -pub struct Suggestion { - pub cmd: String, - pub stage: Option, - pub paths: Vec, -} - -impl Suggestion { - pub fn new(cmd: &str, stage: Option, paths: &[&str]) -> Self { - Self { cmd: cmd.to_owned(), stage, paths: paths.iter().map(|p| p.to_string()).collect() } - } - - pub fn with_single_path(cmd: &str, stage: Option, path: &str) -> Self { - Self::new(cmd, stage, &[path]) - } -} - -impl Display for Suggestion { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { - write!(f, "{} ", self.cmd)?; - - for path in &self.paths { - write!(f, "{} ", path)?; - } - - if let Some(stage) = self.stage { - write!(f, "{}", stage)?; - } else { - // write a sentinel value here (in place of a stage) to be consumed - // by the shim in bootstrap, it will be read and ignored. - write!(f, "N/A")?; - } - - Ok(()) - } -} diff --git a/src/tools/suggest-tests/src/main.rs b/src/tools/suggest-tests/src/main.rs deleted file mode 100644 index d84f8e9fa1ba..000000000000 --- a/src/tools/suggest-tests/src/main.rs +++ /dev/null @@ -1,40 +0,0 @@ -use std::process::ExitCode; - -use build_helper::git::{GitConfig, get_git_modified_files}; -use suggest_tests::get_suggestions; - -fn main() -> ExitCode { - let modified_files = get_git_modified_files( - &GitConfig { - nightly_branch: &env("SUGGEST_TESTS_NIGHTLY_BRANCH"), - git_merge_commit_email: &env("SUGGEST_TESTS_MERGE_COMMIT_EMAIL"), - }, - None, - &Vec::new(), - ); - let modified_files = match modified_files { - Ok(files) => files, - Err(err) => { - eprintln!("Could not get modified files from git: \"{err}\""); - return ExitCode::FAILURE; - } - }; - - let suggestions = get_suggestions(&modified_files); - - for sug in &suggestions { - println!("{sug}"); - } - - ExitCode::SUCCESS -} - -fn env(key: &str) -> String { - match std::env::var(key) { - Ok(var) => var, - Err(err) => { - eprintln!("suggest-tests: failed to read environment variable {key}: {err}"); - std::process::exit(1); - } - } -} diff --git a/src/tools/suggest-tests/src/static_suggestions.rs b/src/tools/suggest-tests/src/static_suggestions.rs deleted file mode 100644 index d363d583b546..000000000000 --- a/src/tools/suggest-tests/src/static_suggestions.rs +++ /dev/null @@ -1,40 +0,0 @@ -use std::sync::OnceLock; - -use crate::{Suggestion, sug}; - -// FIXME: perhaps this could use `std::lazy` when it is stabilized -macro_rules! static_suggestions { - ($( [ $( $glob:expr ),* $(,)? ] => [ $( $suggestion:expr ),* $(,)? ] ),* $(,)? ) => { - pub(crate) fn static_suggestions() -> &'static [(Vec<&'static str>, Vec)] - { - static S: OnceLock, Vec)>> = OnceLock::new(); - S.get_or_init(|| vec![ $( (vec![ $($glob),* ], vec![ $($suggestion),* ]) ),*]) - } - } -} - -static_suggestions! { - ["*.md"] => [ - sug!("test", 0, ["linkchecker"]), - ], - - ["compiler/*"] => [ - sug!("check"), - sug!("test", 1, ["tests/ui", "tests/run-make"]), - ], - - ["compiler/rustc_mir_transform/*"] => [ - sug!("test", 1, ["mir-opt"]), - ], - - [ - "compiler/rustc_mir_transform/src/coverage/*", - "compiler/rustc_codegen_llvm/src/coverageinfo/*", - ] => [ - sug!("test", 1, ["coverage"]), - ], - - ["src/librustdoc/*"] => [ - sug!("test", 1, ["rustdoc"]), - ], -} diff --git a/src/tools/suggest-tests/src/tests.rs b/src/tools/suggest-tests/src/tests.rs deleted file mode 100644 index b4149136fa30..000000000000 --- a/src/tools/suggest-tests/src/tests.rs +++ /dev/null @@ -1,21 +0,0 @@ -macro_rules! sugg_test { - ( $( $name:ident: $paths:expr => $suggestions:expr ),* ) => { - $( - #[test] - fn $name() { - let suggestions = crate::get_suggestions(&$paths).into_iter().map(|s| s.to_string()).collect::>(); - assert_eq!(suggestions, $suggestions); - } - )* - }; -} - -sugg_test! { - test_error_code_docs: ["compiler/rustc_error_codes/src/error_codes/E0000.md"] => - ["check N/A", "test compiler/rustc_error_codes N/A", "test linkchecker 0", "test tests/ui tests/run-make 1"], - - test_rustdoc: ["src/librustdoc/src/lib.rs"] => ["test rustdoc 1"], - - test_rustdoc_and_libstd: ["src/librustdoc/src/lib.rs", "library/std/src/lib.rs"] => - ["test library/std N/A", "test rustdoc 1"] -}