From 446ae429a6e30b416853d6ae0dea228b751671b0 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Thu, 2 Mar 2023 13:21:35 +0100 Subject: [PATCH 1/2] lintcheck: fix parallel processing handling Using `rayon::current_num_threads()` causes a bug: ``` thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: ThreadPoolBuildError { kind: GlobalPoolAlreadyInitialized }', src/main.rs:632:10 ``` Moreover, using the number of threads and dividing it by 2 wouldn't return the number of physical threads on modern processors which have a varying number of threads per core. It makes little sense to restrict ourselves to physical threads, especially when, in modern architectures, cores with multiple threads are often faster (performance) while cores with a unique threads are often slower (efficient). The Rust runtime will make a better choice. --- lintcheck/src/config.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lintcheck/src/config.rs b/lintcheck/src/config.rs index e0244ddcecb8..f87b902b92d9 100644 --- a/lintcheck/src/config.rs +++ b/lintcheck/src/config.rs @@ -89,14 +89,11 @@ impl LintcheckConfig { if markdown { "md" } else { "txt" } )); - // look at the --threads arg, if 0 is passed, ask rayon rayon how many threads it would spawn and - // use half of that for the physical core count - // by default use a single thread + // look at the --threads arg, if 0 is passed, use the threads count let max_jobs = match clap_config.get_one::("threads") { Some(&0) => { // automatic choice - // Rayon seems to return thread count so half that for core count - rayon::current_num_threads() / 2 + std::thread::available_parallelism().map(|n| n.get()).unwrap_or(1) }, Some(&threads) => threads, // no -j passed, use a single thread From 79829d8718626b681d26bfbd64182d5d32d2dc57 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Sat, 4 Mar 2023 11:28:52 +0100 Subject: [PATCH 2/2] lintcheck: use clap's derive interface This makes the code shorter and clearer. The only incompatible change is that an explicit command-line argument `--crates-toml=` will take precedence over the `LINTCHECK_TOML` environment variable. --- lintcheck/Cargo.toml | 2 +- lintcheck/src/config.rs | 144 ++++++++++++---------------------------- 2 files changed, 44 insertions(+), 102 deletions(-) diff --git a/lintcheck/Cargo.toml b/lintcheck/Cargo.toml index 653121af54dc..dbfeb8dd1d19 100644 --- a/lintcheck/Cargo.toml +++ b/lintcheck/Cargo.toml @@ -11,7 +11,7 @@ publish = false [dependencies] cargo_metadata = "0.15.3" -clap = "4.1.4" +clap = { version = "4.1.8", features = ["derive", "env"] } crossbeam-channel = "0.5.6" flate2 = "1.0" rayon = "1.5.1" diff --git a/lintcheck/src/config.rs b/lintcheck/src/config.rs index f87b902b92d9..e1836c19aa25 100644 --- a/lintcheck/src/config.rs +++ b/lintcheck/src/config.rs @@ -1,128 +1,70 @@ -use clap::{Arg, ArgAction, ArgMatches, Command}; -use std::env; +use clap::Parser; use std::path::PathBuf; -fn get_clap_config() -> ArgMatches { - Command::new("lintcheck") - .about("run clippy on a set of crates and check output") - .args([ - Arg::new("only") - .action(ArgAction::Set) - .value_name("CRATE") - .long("only") - .help("Only process a single crate of the list"), - Arg::new("crates-toml") - .action(ArgAction::Set) - .value_name("CRATES-SOURCES-TOML-PATH") - .long("crates-toml") - .help("Set the path for a crates.toml where lintcheck should read the sources from"), - Arg::new("threads") - .action(ArgAction::Set) - .value_name("N") - .value_parser(clap::value_parser!(usize)) - .short('j') - .long("jobs") - .help("Number of threads to use, 0 automatic choice"), - Arg::new("fix") - .long("fix") - .help("Runs cargo clippy --fix and checks if all suggestions apply"), - Arg::new("filter") - .long("filter") - .action(ArgAction::Append) - .value_name("clippy_lint_name") - .help("Apply a filter to only collect specified lints, this also overrides `allow` attributes"), - Arg::new("markdown") - .long("markdown") - .help("Change the reports table to use markdown links"), - Arg::new("recursive") - .long("recursive") - .help("Run clippy on the dependencies of crates specified in crates-toml") - .conflicts_with("threads") - .conflicts_with("fix"), - ]) - .get_matches() -} - -#[derive(Debug, Clone)] +#[derive(Clone, Debug, Parser)] pub(crate) struct LintcheckConfig { - /// max number of jobs to spawn (default 1) + /// Number of threads to use, 0 automatic choice + #[clap(long = "jobs", short = 'j', value_name = "N", default_value_t = 1)] pub max_jobs: usize, - /// we read the sources to check from here + /// Set the path for a crates.toml where lintcheck should read the sources from + #[clap( + long = "crates-toml", + value_name = "CRATES-SOURCES-TOML-PATH", + default_value = "lintcheck/lintcheck_crates.toml", + hide_default_value = true, + env = "LINTCHECK_TOML", + hide_env = true + )] pub sources_toml_path: PathBuf, - /// we save the clippy lint results here - pub lintcheck_results_path: PathBuf, - /// Check only a specified package + /// File to save the clippy lint results here + #[clap(skip = "")] + pub lintcheck_results_path: PathBuf, // Overridden in new() + /// Only process a single crate on the list + #[clap(long, value_name = "CRATE")] pub only: Option, - /// whether to just run --fix and not collect all the warnings + /// Runs cargo clippy --fix and checks if all suggestions apply + #[clap(long, conflicts_with("max_jobs"))] pub fix: bool, - /// A list of lints that this lintcheck run should focus on + /// Apply a filter to only collect specified lints, this also overrides `allow` attributes + #[clap(long = "filter", value_name = "clippy_lint_name", use_value_delimiter = true)] pub lint_filter: Vec, - /// Indicate if the output should support markdown syntax + /// Change the reports table to use markdown links + #[clap(long)] pub markdown: bool, - /// Run clippy on the dependencies of crates + /// Run clippy on the dependencies of crates specified in crates-toml + #[clap(long, conflicts_with("max_jobs"))] pub recursive: bool, } impl LintcheckConfig { pub fn new() -> Self { - let clap_config = get_clap_config(); - - // first, check if we got anything passed via the LINTCHECK_TOML env var, - // if not, ask clap if we got any value for --crates-toml - // if not, use the default "lintcheck/lintcheck_crates.toml" - let sources_toml = env::var("LINTCHECK_TOML").unwrap_or_else(|_| { - clap_config - .get_one::("crates-toml") - .map_or("lintcheck/lintcheck_crates.toml", |s| &**s) - .into() - }); - - let markdown = clap_config.contains_id("markdown"); - let sources_toml_path = PathBuf::from(sources_toml); + let mut config = LintcheckConfig::parse(); // for the path where we save the lint results, get the filename without extension (so for // wasd.toml, use "wasd"...) - let filename: PathBuf = sources_toml_path.file_stem().unwrap().into(); - let lintcheck_results_path = PathBuf::from(format!( + let filename: PathBuf = config.sources_toml_path.file_stem().unwrap().into(); + config.lintcheck_results_path = PathBuf::from(format!( "lintcheck-logs/{}_logs.{}", filename.display(), - if markdown { "md" } else { "txt" } + if config.markdown { "md" } else { "txt" } )); // look at the --threads arg, if 0 is passed, use the threads count - let max_jobs = match clap_config.get_one::("threads") { - Some(&0) => { - // automatic choice - std::thread::available_parallelism().map(|n| n.get()).unwrap_or(1) - }, - Some(&threads) => threads, - // no -j passed, use a single thread - None => 1, + if config.max_jobs == 0 { + // automatic choice + config.max_jobs = std::thread::available_parallelism().map_or(1, |n| n.get()); }; - let lint_filter: Vec = clap_config - .get_many::("filter") - .map(|iter| { - iter.map(|lint_name| { - let mut filter = lint_name.replace('_', "-"); - if !filter.starts_with("clippy::") { - filter.insert_str(0, "clippy::"); - } - filter - }) - .collect() - }) - .unwrap_or_default(); - - LintcheckConfig { - max_jobs, - sources_toml_path, - lintcheck_results_path, - only: clap_config.get_one::("only").map(String::from), - fix: clap_config.contains_id("fix"), - lint_filter, - markdown, - recursive: clap_config.contains_id("recursive"), + for lint_name in &mut config.lint_filter { + *lint_name = format!( + "clippy::{}", + lint_name + .strip_prefix("clippy::") + .unwrap_or(lint_name) + .replace('_', "-") + ); } + + config } }