diff --git a/Cargo.lock b/Cargo.lock index 01300d56cff9c..f279ad6cae9dd 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/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 52b38421eec22..ca70a7758d581 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/Cargo.toml b/src/tools/tidy/Cargo.toml index cbf27ea87a076..d5433080efc08 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 4a1d263f1a4f3..5fa0dd751b647 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 0000000000000..8041f739308d4 --- /dev/null +++ b/src/tools/tidy/src/arg_parser.rs @@ -0,0 +1,102 @@ +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") + .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") + .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("target files are modified").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 0000000000000..c5e7aed21c1a0 --- /dev/null +++ b/src/tools/tidy/src/arg_parser/tests.rs @@ -0,0 +1,168 @@ +use std::path::PathBuf; + +use crate::arg_parser::TidyArgParser; + +// Test all arguments +#[test] +fn test_tidy_parser_full() { + let args = vec![ + "rust-tidy", + "--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", + "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("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 6f53f2fff1a48..4e6c316f5e18e 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 1c81a485608ae..6272e00591d78 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 425f43e42b7f8..19a9fa80d9f04 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 94c24f11ed12f..457fbe93e6d22 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) => {