From 59c77c43d941dd621701d8e452928846af934497 Mon Sep 17 00:00:00 2001 From: Shunpoco Date: Sat, 17 Jan 2026 10:14:47 +0000 Subject: [PATCH 1/2] use clap on tidy Current tidy parses paths and options manually, and parsing is spreading multple files. This commit introduces a parser using clap to clean and centralize it. --- Cargo.lock | 9 +- src/tools/tidy/Cargo.toml | 1 + src/tools/tidy/src/alphabetical/tests.rs | 2 +- src/tools/tidy/src/arg_parser.rs | 92 +++++++++++++ src/tools/tidy/src/arg_parser/tests.rs | 162 +++++++++++++++++++++++ src/tools/tidy/src/diagnostics.rs | 12 +- src/tools/tidy/src/extra_checks/mod.rs | 12 +- src/tools/tidy/src/lib.rs | 1 + src/tools/tidy/src/main.rs | 39 +++--- 9 files changed, 284 insertions(+), 46 deletions(-) create mode 100644 src/tools/tidy/src/arg_parser.rs create mode 100644 src/tools/tidy/src/arg_parser/tests.rs diff --git a/Cargo.lock b/Cargo.lock index 01300d56cff9..f279ad6cae9d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -580,9 +580,9 @@ dependencies = [ [[package]] name = "clap" -version = "4.5.51" +version = "4.5.54" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4c26d721170e0295f191a69bd9a1f93efcdb0aff38684b61ab5750468972e5f5" +checksum = "c6e6ff9dcd79cff5cd969a17a545d79e84ab086e444102a591e288a8aa3ce394" dependencies = [ "clap_builder", "clap_derive", @@ -600,9 +600,9 @@ dependencies = [ [[package]] name = "clap_builder" -version = "4.5.51" +version = "4.5.54" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "75835f0c7bf681bfd05abe44e965760fea999a5286c6eb2d59883634fd02011a" +checksum = "fa42cf4d2b7a41bc8f663a7cab4031ebafa1bf3875705bfaf8466dc60ab52c00" dependencies = [ "anstream", "anstyle", @@ -5622,6 +5622,7 @@ version = "0.1.0" dependencies = [ "build_helper", "cargo_metadata 0.21.0", + "clap", "fluent-syntax", "globset", "ignore", diff --git a/src/tools/tidy/Cargo.toml b/src/tools/tidy/Cargo.toml index cbf27ea87a07..d5433080efc0 100644 --- a/src/tools/tidy/Cargo.toml +++ b/src/tools/tidy/Cargo.toml @@ -20,6 +20,7 @@ fluent-syntax = "0.12" similar = "2.5.0" toml = "0.7.8" tempfile = "3.15.0" +clap = "4.5.54" [features] build-metrics = ["dep:serde"] diff --git a/src/tools/tidy/src/alphabetical/tests.rs b/src/tools/tidy/src/alphabetical/tests.rs index 4a1d263f1a4f..5fa0dd751b64 100644 --- a/src/tools/tidy/src/alphabetical/tests.rs +++ b/src/tools/tidy/src/alphabetical/tests.rs @@ -37,7 +37,7 @@ fn bless_test(before: &str, after: &str) { let temp_path = tempfile::Builder::new().tempfile().unwrap().into_temp_path(); std::fs::write(&temp_path, before).unwrap(); - let tidy_ctx = TidyCtx::new(Path::new("/"), false, TidyFlags::new(&["--bless".to_owned()])); + let tidy_ctx = TidyCtx::new(Path::new("/"), false, TidyFlags::new(true)); let mut check = tidy_ctx.start_check("alphabetical-test"); check_lines(&temp_path, before, &tidy_ctx, &mut check); diff --git a/src/tools/tidy/src/arg_parser.rs b/src/tools/tidy/src/arg_parser.rs new file mode 100644 index 000000000000..ca7ee04da8fb --- /dev/null +++ b/src/tools/tidy/src/arg_parser.rs @@ -0,0 +1,92 @@ +use std::num::NonZeroUsize; +use std::path::PathBuf; + +use clap::{Arg, ArgAction, ArgMatches, Command, value_parser}; + +#[cfg(test)] +mod tests; + +#[derive(Debug, Clone)] +pub struct TidyArgParser { + pub root_path: PathBuf, + pub cargo: PathBuf, + pub output_directory: PathBuf, + pub concurrency: NonZeroUsize, + pub npm: PathBuf, + pub verbose: bool, + pub bless: bool, + pub extra_checks: Option>, + pub pos_args: Vec, +} + +impl TidyArgParser { + fn command() -> Command { + Command::new("rust-tidy") + .arg( + Arg::new("root_path") + .help("path of the root directory") + .required(true) + .value_parser(value_parser!(PathBuf)), + ) + .arg( + Arg::new("cargo") + .help("path of cargo") + .required(true) + .value_parser(value_parser!(PathBuf)), + ) + .arg( + Arg::new("output_directory") + .help("path of output directory") + .required(true) + .value_parser(value_parser!(PathBuf)), + ) + .arg(Arg::new("concurrency").required(true).value_parser(value_parser!(NonZeroUsize))) + .arg( + Arg::new("npm") + .help("path of npm") + .required(true) + .value_parser(value_parser!(PathBuf)), + ) + .arg(Arg::new("verbose").help("verbose").long("verbose").action(ArgAction::SetTrue)) + .arg(Arg::new("bless").help("bless").long("bless").action(ArgAction::SetTrue)) + .arg( + Arg::new("extra_checks") + .help("extra checks") + .long("extra-checks") + .value_delimiter(',') + .action(ArgAction::Append), + ) + .arg(Arg::new("pos_args").help("for extra checks. you can specify configs and target files for external check tools").action(ArgAction::Append).last(true)) + } + + fn build(matches: ArgMatches) -> Self { + let mut tidy_flags = Self { + root_path: matches.get_one::("root_path").unwrap().clone(), + cargo: matches.get_one::("cargo").unwrap().clone(), + output_directory: matches.get_one::("output_directory").unwrap().clone(), + concurrency: *matches.get_one::("concurrency").unwrap(), + npm: matches.get_one::("npm").unwrap().clone(), + verbose: *matches.get_one::("verbose").unwrap(), + bless: *matches.get_one::("bless").unwrap(), + extra_checks: None, + pos_args: vec![], + }; + + if let Some(extra_checks) = matches.get_many::("extra_checks") { + tidy_flags.extra_checks = Some(extra_checks.map(|s| s.to_string()).collect::>()); + } + + tidy_flags.pos_args = matches + .get_many::("pos_args") + .unwrap_or_default() + .map(|v| v.to_string()) + .collect::>(); + + tidy_flags + } + + pub fn parse() -> Self { + let matches = Self::command().get_matches(); + Self::build(matches) + } +} diff --git a/src/tools/tidy/src/arg_parser/tests.rs b/src/tools/tidy/src/arg_parser/tests.rs new file mode 100644 index 000000000000..856e80279e27 --- /dev/null +++ b/src/tools/tidy/src/arg_parser/tests.rs @@ -0,0 +1,162 @@ +use std::path::PathBuf; + +use crate::arg_parser::TidyArgParser; + +#[test] +fn test_tidy_parser() { + let args = vec![ + "rust-tidy", + "/home/user/rust", // Root dir + "/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo", // Cardo location + "/home/user/rust/build", // Build dir + "16", // Number of concurrency + "/home/user/rust/build/misc-tools/bin/yarn", // Yarn location + "--verbose", + "--bless", + "--extra-checks", + "if-installed:auto:js,auto:if-installed:py,if-installed:auto:cpp,if-installed:auto:spellcheck", + "--", // pos_args + "some-file", + "some-file2", + ]; + let cmd = TidyArgParser::command(); + let parsed_args = TidyArgParser::build(cmd.get_matches_from(args)); + + assert_eq!(parsed_args.root_path, PathBuf::from("/home/user/rust")); + assert_eq!( + parsed_args.cargo, + PathBuf::from("/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo") + ); + assert_eq!(parsed_args.output_directory, PathBuf::from("/home/user/rust/build")); + assert_eq!(parsed_args.concurrency.get(), 16); + assert_eq!(parsed_args.npm, PathBuf::from("/home/user/rust/build/misc-tools/bin/yarn")); + assert!(parsed_args.verbose); + assert!(parsed_args.bless); + assert_eq!( + parsed_args.extra_checks, + Some(vec![ + "if-installed:auto:js".to_string(), + "auto:if-installed:py".to_string(), + "if-installed:auto:cpp".to_string(), + "if-installed:auto:spellcheck".to_string(), + ]) + ); + assert_eq!(parsed_args.pos_args, vec!["some-file".to_string(), "some-file2".to_string()]); +} + +// The parser can take required args any order +#[test] +fn test_tidy_parser_any_order() { + let args = vec![ + "rust-tidy", + "--npm-path", + "yarn", + "--concurrency", + "16", + "--output-dir", + "/home/user/rust/build", + "--cargo-path", + "/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo", + "--root-path", + "/home/user/rust", + ]; + let cmd = TidyArgParser::command(); + let parsed_args = TidyArgParser::build(cmd.get_matches_from(args)); + + assert_eq!(parsed_args.root_path, PathBuf::from("/home/user/rust")); + assert_eq!( + parsed_args.cargo, + PathBuf::from("/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo") + ); + assert_eq!(parsed_args.output_directory, PathBuf::from("/home/user/rust/build")); + assert_eq!(parsed_args.concurrency.get(), 16); + assert_eq!(parsed_args.npm, PathBuf::from("yarn")); +} + +// --root-path is required +#[test] +fn test_tidy_parser_missing_root_path() { + let args = vec![ + "rust-tidy", + "--npm-path", + "yarn", + "--concurrency", + "16", + "--output-dir", + "/home/user/rust/build", + "--cargo-path", + "/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo", + ]; + let cmd = TidyArgParser::command(); + assert!(cmd.try_get_matches_from(args).is_err()); +} + +// --cargo-path is required +#[test] +fn test_tidy_parser_missing_cargo_path() { + let args = vec![ + "rust-tidy", + "--npm-path", + "yarn", + "--concurrency", + "16", + "--output-dir", + "/home/user/rust/build", + "--root-path", + "/home/user/rust", + ]; + let cmd = TidyArgParser::command(); + assert!(cmd.try_get_matches_from(args).is_err()); +} + +// --output-dir is required +#[test] +fn test_tidy_parser_missing_output_dir() { + let args = vec![ + "rust-tidy", + "--npm-path", + "yarn", + "--concurrency", + "16", + "--cargo-path", + "/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo", + "--root-path", + "/home/user/rust", + ]; + let cmd = TidyArgParser::command(); + assert!(cmd.try_get_matches_from(args).is_err()); +} + +// --concurrency is required +#[test] +fn test_tidy_parser_missing_concurrency() { + let args = vec![ + "rust-tidy", + "--npm-path", + "yarn", + "--output-dir", + "/home/user/rust/build", + "--cargo-path", + "/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo", + "--root-path", + "/home/user/rust", + ]; + let cmd = TidyArgParser::command(); + assert!(cmd.try_get_matches_from(args).is_err()); +} + +// --npm-path is required +#[test] +fn test_tidy_parser_missing_npm_path() { + let args = vec![ + "rust-tidy", + "--concurrency", + "16", + "--output-dir", + "/home/user/rust/build", + "--cargo-path", + "/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo", + ]; + let cmd = TidyArgParser::command(); + assert!(cmd.try_get_matches_from(args).is_err()); +} diff --git a/src/tools/tidy/src/diagnostics.rs b/src/tools/tidy/src/diagnostics.rs index 6f53f2fff1a4..4e6c316f5e18 100644 --- a/src/tools/tidy/src/diagnostics.rs +++ b/src/tools/tidy/src/diagnostics.rs @@ -14,16 +14,8 @@ pub struct TidyFlags { } impl TidyFlags { - pub fn new(cfg_args: &[String]) -> Self { - let mut flags = Self::default(); - - for arg in cfg_args { - match arg.as_str() { - "--bless" => flags.bless = true, - _ => continue, - } - } - flags + pub fn new(bless: bool) -> Self { + Self { bless } } } diff --git a/src/tools/tidy/src/extra_checks/mod.rs b/src/tools/tidy/src/extra_checks/mod.rs index 1c81a485608a..6272e00591d7 100644 --- a/src/tools/tidy/src/extra_checks/mod.rs +++ b/src/tools/tidy/src/extra_checks/mod.rs @@ -58,8 +58,8 @@ pub fn check( tools_path: &Path, npm: &Path, cargo: &Path, - extra_checks: Option<&str>, - pos_args: &[String], + extra_checks: Option>, + pos_args: Vec, tidy_ctx: TidyCtx, ) { let mut check = tidy_ctx.start_check("extra_checks"); @@ -88,8 +88,8 @@ fn check_impl( tools_path: &Path, npm: &Path, cargo: &Path, - extra_checks: Option<&str>, - pos_args: &[String], + extra_checks: Option>, + pos_args: Vec, tidy_ctx: &TidyCtx, ) -> Result<(), Error> { let show_diff = @@ -99,9 +99,7 @@ fn check_impl( // Split comma-separated args up let mut lint_args = match extra_checks { Some(s) => s - .strip_prefix("--extra-checks=") - .unwrap() - .split(',') + .iter() .map(|s| { if s == "spellcheck:fix" { eprintln!("warning: `spellcheck:fix` is no longer valid, use `--extra-checks=spellcheck --bless`"); diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index 425f43e42b7f..19a9fa80d9f0 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -157,6 +157,7 @@ pub fn files_modified(ci_info: &CiInfo, pred: impl Fn(&str) -> bool) -> bool { } pub mod alphabetical; +pub mod arg_parser; pub mod bins; pub mod debug_artifacts; pub mod deps; diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index 94c24f11ed12..457fbe93e6d2 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -5,12 +5,10 @@ //! builders. The tidy checks can be executed with `./x.py test tidy`. use std::collections::VecDeque; -use std::num::NonZeroUsize; -use std::path::PathBuf; -use std::str::FromStr; use std::thread::{self, ScopedJoinHandle, scope}; use std::{env, process}; +use tidy::arg_parser::TidyArgParser; use tidy::diagnostics::{COLOR_ERROR, COLOR_SUCCESS, TidyCtx, TidyFlags, output_message}; use tidy::*; @@ -22,14 +20,13 @@ fn main() { env::set_var("RUSTC_BOOTSTRAP", "1"); } - let root_path: PathBuf = env::args_os().nth(1).expect("need path to root of repo").into(); - let cargo: PathBuf = env::args_os().nth(2).expect("need path to cargo").into(); - let output_directory: PathBuf = - env::args_os().nth(3).expect("need path to output directory").into(); - let concurrency: NonZeroUsize = - FromStr::from_str(&env::args().nth(4).expect("need concurrency")) - .expect("concurrency must be a number"); - let npm: PathBuf = env::args_os().nth(5).expect("need name/path of npm command").into(); + let parsed_args = TidyArgParser::parse(); + + let root_path = parsed_args.root_path; + let cargo = parsed_args.cargo; + let output_directory = parsed_args.output_directory; + let concurrency = parsed_args.concurrency.get(); + let npm = parsed_args.npm; let root_manifest = root_path.join("Cargo.toml"); let src_path = root_path.join("src"); @@ -40,17 +37,12 @@ fn main() { let tools_path = src_path.join("tools"); let crashes_path = tests_path.join("crashes"); - let args: Vec = env::args().skip(1).collect(); - let (cfg_args, pos_args) = match args.iter().position(|arg| arg == "--") { - Some(pos) => (&args[..pos], &args[pos + 1..]), - None => (&args[..], [].as_slice()), - }; - let verbose = cfg_args.iter().any(|s| *s == "--verbose"); - let extra_checks = - cfg_args.iter().find(|s| s.starts_with("--extra-checks=")).map(String::as_str); + let verbose = parsed_args.verbose; + let bless = parsed_args.bless; + let extra_checks = parsed_args.extra_checks; + let pos_args = parsed_args.pos_args; - let tidy_flags = TidyFlags::new(cfg_args); - let tidy_ctx = TidyCtx::new(&root_path, verbose, tidy_flags); + let tidy_ctx = TidyCtx::new(&root_path, verbose, TidyFlags::new(bless)); let ci_info = CiInfo::new(tidy_ctx.clone()); let drain_handles = |handles: &mut VecDeque>| { @@ -61,14 +53,13 @@ fn main() { } } - while handles.len() >= concurrency.get() { + while handles.len() >= concurrency { handles.pop_front().unwrap().join().unwrap(); } }; scope(|s| { - let mut handles: VecDeque> = - VecDeque::with_capacity(concurrency.get()); + let mut handles: VecDeque> = VecDeque::with_capacity(concurrency); macro_rules! check { ($p:ident) => { From 14858732330380e488ffb406d4a537bf133d90f9 Mon Sep 17 00:00:00 2001 From: Shunpoco Date: Sun, 25 Jan 2026 10:13:48 +0000 Subject: [PATCH 2/2] set long variants for required args Currently some required arguments (like path of the root dir) are ordered, but it causes an user (mainly bootstrap) needs to remember the order. This commit introduces long arguments (e.g., --root-path) for required args. --- src/bootstrap/src/core/build_steps/test.rs | 12 ++++++------ src/tools/tidy/src/arg_parser.rs | 14 ++++++++++++-- src/tools/tidy/src/arg_parser/tests.rs | 20 +++++++++++++------- 3 files changed, 31 insertions(+), 15 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 52b38421eec2..ca70a7758d58 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -1293,19 +1293,19 @@ impl Step for Tidy { /// for the `dev` or `nightly` channels. fn run(self, builder: &Builder<'_>) { let mut cmd = builder.tool_cmd(Tool::Tidy); - cmd.arg(&builder.src); - cmd.arg(&builder.initial_cargo); - cmd.arg(&builder.out); + cmd.arg(format!("--root-path={}", &builder.src.display())); + cmd.arg(format!("--cargo-path={}", &builder.initial_cargo.display())); + cmd.arg(format!("--output-dir={}", &builder.out.display())); // Tidy is heavily IO constrained. Still respect `-j`, but use a higher limit if `jobs` hasn't been configured. let jobs = builder.config.jobs.unwrap_or_else(|| { 8 * std::thread::available_parallelism().map_or(1, std::num::NonZeroUsize::get) as u32 }); - cmd.arg(jobs.to_string()); + cmd.arg(format!("--concurrency={jobs}")); // pass the path to the yarn command used for installing js deps. if let Some(yarn) = &builder.config.yarn { - cmd.arg(yarn); + cmd.arg(format!("--npm-path={}", yarn.display())); } else { - cmd.arg("yarn"); + cmd.arg("--npm-path=yarn"); } if builder.is_verbose() { cmd.arg("--verbose"); diff --git a/src/tools/tidy/src/arg_parser.rs b/src/tools/tidy/src/arg_parser.rs index ca7ee04da8fb..8041f739308d 100644 --- a/src/tools/tidy/src/arg_parser.rs +++ b/src/tools/tidy/src/arg_parser.rs @@ -25,30 +25,40 @@ impl TidyArgParser { .arg( Arg::new("root_path") .help("path of the root directory") + .long("root-path") .required(true) .value_parser(value_parser!(PathBuf)), ) .arg( Arg::new("cargo") .help("path of cargo") + .long("cargo-path") .required(true) .value_parser(value_parser!(PathBuf)), ) .arg( Arg::new("output_directory") .help("path of output directory") + .long("output-dir") .required(true) .value_parser(value_parser!(PathBuf)), ) - .arg(Arg::new("concurrency").required(true).value_parser(value_parser!(NonZeroUsize))) + .arg( + Arg::new("concurrency") + .help("number of threads working concurrently") + .long("concurrency") + .required(true) + .value_parser(value_parser!(NonZeroUsize)), + ) .arg( Arg::new("npm") .help("path of npm") + .long("npm-path") .required(true) .value_parser(value_parser!(PathBuf)), ) .arg(Arg::new("verbose").help("verbose").long("verbose").action(ArgAction::SetTrue)) - .arg(Arg::new("bless").help("bless").long("bless").action(ArgAction::SetTrue)) + .arg(Arg::new("bless").help("target files are modified").long("bless").action(ArgAction::SetTrue)) .arg( Arg::new("extra_checks") .help("extra checks") diff --git a/src/tools/tidy/src/arg_parser/tests.rs b/src/tools/tidy/src/arg_parser/tests.rs index 856e80279e27..c5e7aed21c1a 100644 --- a/src/tools/tidy/src/arg_parser/tests.rs +++ b/src/tools/tidy/src/arg_parser/tests.rs @@ -2,15 +2,21 @@ use std::path::PathBuf; use crate::arg_parser::TidyArgParser; +// Test all arguments #[test] -fn test_tidy_parser() { +fn test_tidy_parser_full() { let args = vec![ "rust-tidy", - "/home/user/rust", // Root dir - "/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo", // Cardo location - "/home/user/rust/build", // Build dir - "16", // Number of concurrency - "/home/user/rust/build/misc-tools/bin/yarn", // Yarn location + "--root-path", + "/home/user/rust", + "--cargo-path", + "/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo", + "--output-dir", + "/home/user/rust/build", + "--concurrency", + "16", + "--npm-path", + "yarn", "--verbose", "--bless", "--extra-checks", @@ -29,7 +35,7 @@ fn test_tidy_parser() { ); assert_eq!(parsed_args.output_directory, PathBuf::from("/home/user/rust/build")); assert_eq!(parsed_args.concurrency.get(), 16); - assert_eq!(parsed_args.npm, PathBuf::from("/home/user/rust/build/misc-tools/bin/yarn")); + assert_eq!(parsed_args.npm, PathBuf::from("yarn")); assert!(parsed_args.verbose); assert!(parsed_args.bless); assert_eq!(