From 5461e99e65e813c6791e705c440d2ccabd2a9b80 Mon Sep 17 00:00:00 2001 From: kraktus Date: Sat, 10 Sep 2022 19:05:15 +0200 Subject: [PATCH 1/2] Better logging for `lintcheck` Use `simplelog` to handle logs, as `env_logger` does not handle writing to file for the moment, see https://github.com/env-logger-rs/env_logger/issues/208 Do not push most verbose logs on stdout, only push them in the log file --- lintcheck/Cargo.toml | 2 ++ lintcheck/src/config.rs | 31 ++++++++++++++++++++++++++++++- lintcheck/src/main.rs | 34 +++++++++++++++++----------------- 3 files changed, 49 insertions(+), 18 deletions(-) diff --git a/lintcheck/Cargo.toml b/lintcheck/Cargo.toml index de31c16b819e..d8154db0c5eb 100644 --- a/lintcheck/Cargo.toml +++ b/lintcheck/Cargo.toml @@ -13,7 +13,9 @@ publish = false cargo_metadata = "0.14" clap = "3.2" crossbeam-channel = "0.5.6" +simplelog = "0.12.0" flate2 = "1.0" +log = "0.4" rayon = "1.5.1" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0.85" diff --git a/lintcheck/src/config.rs b/lintcheck/src/config.rs index b8824024e6c7..981fe2ad8121 100644 --- a/lintcheck/src/config.rs +++ b/lintcheck/src/config.rs @@ -1,5 +1,8 @@ use clap::{Arg, ArgAction, ArgMatches, Command}; +use log::LevelFilter; +use simplelog::{ColorChoice, CombinedLogger, Config, TermLogger, TerminalMode, WriteLogger}; use std::env; +use std::fs::{self, File}; use std::path::PathBuf; fn get_clap_config() -> ArgMatches { @@ -39,6 +42,11 @@ fn get_clap_config() -> ArgMatches { .help("Run clippy on the dependencies of crates specified in crates-toml") .conflicts_with("threads") .conflicts_with("fix"), + Arg::new("verbose") + .short('v') + .long("--verbose") + .action(ArgAction::Count) + .help("Verbosity to use, default to WARN"), ]) .get_matches() } @@ -66,6 +74,27 @@ pub(crate) struct LintcheckConfig { impl LintcheckConfig { pub fn new() -> Self { let clap_config = get_clap_config(); + let level_filter = match clap_config.get_count("verbose") { + 0 => LevelFilter::Warn, + 1 => LevelFilter::Info, + 2 => LevelFilter::Debug, + _ => LevelFilter::Trace, + }; + // using `create_dir_all` as it does not error when the dir already exists + fs::create_dir_all("lintcheck-logs").expect("Creating the log dir failed"); + let _ = CombinedLogger::init(vec![ + TermLogger::new( + std::cmp::min(level_filter, LevelFilter::Info), // do not print more verbose log than `INFO` to stdout, + Config::default(), + TerminalMode::Mixed, + ColorChoice::Auto, + ), + WriteLogger::new( + level_filter, + Config::default(), + File::create("lintcheck-logs/lintcheck.log").unwrap(), + ), + ]); // 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 @@ -84,7 +113,7 @@ impl LintcheckConfig { // wasd.toml, use "wasd"...) let filename: PathBuf = sources_toml_path.file_stem().unwrap().into(); let lintcheck_results_path = PathBuf::from(format!( - "lintcheck-logs/{}_logs.{}", + "lintcheck-logs/{}_results.{}", filename.display(), if markdown { "md" } else { "txt" } )); diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs index ee8ab7c1d7cb..1a1730a32f26 100644 --- a/lintcheck/src/main.rs +++ b/lintcheck/src/main.rs @@ -28,6 +28,7 @@ use std::time::Duration; use cargo_metadata::diagnostic::{Diagnostic, DiagnosticLevel}; use cargo_metadata::Message; +use log::{debug, error, trace, warn}; use rayon::prelude::*; use serde::{Deserialize, Serialize}; use walkdir::{DirEntry, WalkDir}; @@ -163,10 +164,10 @@ fn get(path: &str) -> Result { match ureq::get(path).call() { Ok(res) => return Ok(res), Err(e) if retries >= MAX_RETRIES => return Err(e), - Err(ureq::Error::Transport(e)) => eprintln!("Error: {e}"), + Err(ureq::Error::Transport(e)) => error!("{}", e), Err(e) => return Err(e), } - eprintln!("retrying in {retries} seconds..."); + warn!("retrying in {retries} seconds..."); thread::sleep(Duration::from_secs(u64::from(retries))); retries += 1; } @@ -234,7 +235,7 @@ impl CrateSource { .expect("Failed to clone git repo!") .success() { - eprintln!("Failed to clone {url} into {}", repo_path.display()); + warn!("Failed to clone {url} into {}", repo_path.display()); } } // check out the commit/branch/whatever @@ -247,7 +248,7 @@ impl CrateSource { .expect("Failed to check out commit") .success() { - eprintln!("Failed to checkout {commit} of repo at {}", repo_path.display()); + warn!("Failed to checkout {commit} of repo at {}", repo_path.display()); } Crate { @@ -390,6 +391,8 @@ impl Crate { cargo_clippy_args.extend(clippy_args); + debug!("Arguments passed to cargo clippy driver: {:?}", cargo_clippy_args); + let all_output = Command::new(&cargo_clippy_path) // use the looping index to create individual target dirs .env("CARGO_TARGET_DIR", shared_target_dir.join(format!("_{thread_index:?}"))) @@ -409,21 +412,19 @@ impl Crate { let status = &all_output.status; if !status.success() { - eprintln!( - "\nWARNING: bad exit status after checking {} {} \n", - self.name, self.version - ); + warn!("bad exit status after checking {} {} \n", self.name, self.version); } if config.fix { + trace!("{}", stderr); if let Some(stderr) = stderr .lines() .find(|line| line.contains("failed to automatically apply fixes suggested by rustc to crate")) { let subcrate = &stderr[63..]; - println!( - "ERROR: failed to apply some suggetion to {} / to (sub)crate {subcrate}", - self.name + error!( + "failed to apply some suggetion to {} / to (sub)crate {}", + self.name, subcrate ); } // fast path, we don't need the warnings anyway @@ -449,7 +450,7 @@ fn build_clippy() { .status() .expect("Failed to build clippy!"); if !status.success() { - eprintln!("Error: Failed to compile Clippy!"); + error!("Failed to compile Clippy!"); std::process::exit(1); } } @@ -553,7 +554,7 @@ fn main() { // assert that we launch lintcheck from the repo root (via cargo lintcheck) if std::fs::metadata("lintcheck/Cargo.toml").is_err() { - eprintln!("lintcheck needs to be run from clippy's repo root!\nUse `cargo lintcheck` alternatively."); + error!("lintcheck needs to be run from clippy's repo root!\nUse `cargo lintcheck` alternatively."); std::process::exit(3); } @@ -615,8 +616,8 @@ fn main() { .collect(); if crates.is_empty() { - eprintln!( - "ERROR: could not find crate '{}' in lintcheck/lintcheck_crates.toml", + error!( + "could not find crate '{}' in lintcheck/lintcheck_crates.toml", config.only.unwrap(), ); std::process::exit(1); @@ -693,8 +694,7 @@ fn main() { let _ = write!(text, "{cratename}: '{msg}'"); } - println!("Writing logs to {}", config.lintcheck_results_path.display()); - fs::create_dir_all(config.lintcheck_results_path.parent().unwrap()).unwrap(); + println!("Writing results to {}", config.lintcheck_results_path.display()); fs::write(&config.lintcheck_results_path, text).unwrap(); print_stats(old_stats, new_stats, &config.lint_filter); From 3a2c1e5d6367919170e6c5fb789923d5d129bbb2 Mon Sep 17 00:00:00 2001 From: kraktus Date: Thu, 27 Oct 2022 16:04:19 +0200 Subject: [PATCH 2/2] Fix `lintcheck --fix` There were several issues: - `--fix` was ignored as part of https://github.com/rust-lang/rust-clippy/pull/9461 - `--filter` in conjunction to `--fix` was broken due to https://github.com/rust-lang/rust-clippy/issues/9703 After `lintcheck --fix` is used, crates will be re-extracted since their content has been modified --- lintcheck/src/main.rs | 54 +++++++++++++++++++++++++++++++------------ 1 file changed, 39 insertions(+), 15 deletions(-) diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs index 1a1730a32f26..dfa7f150aed5 100644 --- a/lintcheck/src/main.rs +++ b/lintcheck/src/main.rs @@ -182,6 +182,7 @@ impl CrateSource { CrateSource::CratesIo { name, version, options } => { let extract_dir = PathBuf::from(LINTCHECK_SOURCES); let krate_download_dir = PathBuf::from(LINTCHECK_DOWNLOADS); + let extracted_krate_dir = extract_dir.join(format!("{name}-{version}/")); // url to download the crate from crates.io let url = format!("https://crates.io/api/v1/crates/{name}/{version}/download"); @@ -189,26 +190,27 @@ impl CrateSource { create_dirs(&krate_download_dir, &extract_dir); let krate_file_path = krate_download_dir.join(format!("{name}-{version}.crate.tar.gz")); - // don't download/extract if we already have done so + // don't download if we already have done so if !krate_file_path.is_file() { // create a file path to download and write the crate data into let mut krate_dest = std::fs::File::create(&krate_file_path).unwrap(); let mut krate_req = get(&url).unwrap().into_reader(); // copy the crate into the file std::io::copy(&mut krate_req, &mut krate_dest).unwrap(); + } - // unzip the tarball - let ungz_tar = flate2::read::GzDecoder::new(std::fs::File::open(&krate_file_path).unwrap()); - // extract the tar archive - let mut archive = tar::Archive::new(ungz_tar); - archive.unpack(&extract_dir).expect("Failed to extract!"); + // if the source code was altered (previous use of `--fix`), we need to restore the crate + // to its original state by re-extracting it + if !extracted_krate_dir.exists() || has_been_modified(&extracted_krate_dir) { + debug!("extracting {} {}", name, version); + Self::extract(&extract_dir, &krate_file_path); } // crate is extracted, return a new Krate object which contains the path to the extracted // sources that clippy can check Crate { version: version.clone(), name: name.clone(), - path: extract_dir.join(format!("{name}-{version}/")), + path: extracted_krate_dir, options: options.clone(), } }, @@ -299,6 +301,14 @@ impl CrateSource { }, } } + + fn extract(extract_dir: &Path, krate_file_path: &Path) { + // unzip the tarball + let ungz_tar = flate2::read::GzDecoder::new(std::fs::File::open(krate_file_path).unwrap()); + // extract the tar archive + let mut archive = tar::Archive::new(ungz_tar); + archive.unpack(extract_dir).expect("Failed to extract!"); + } } impl Crate { @@ -338,7 +348,9 @@ impl Crate { let shared_target_dir = clippy_project_root().join("target/lintcheck/shared_target_dir"); let mut cargo_clippy_args = if config.fix { - vec!["--fix", "--"] + // need to pass `clippy` arg even if already feeding `cargo-clippy` + // see https://github.com/rust-lang/rust-clippy/pull/9461 + vec!["clippy", "--fix", "--allow-no-vcs", "--"] } else { vec!["--", "--message-format=json", "--"] }; @@ -348,16 +360,19 @@ impl Crate { for opt in options { clippy_args.push(opt); } - } else { - clippy_args.extend(["-Wclippy::pedantic", "-Wclippy::cargo"]); } - if lint_filter.is_empty() { - clippy_args.push("--cap-lints=warn"); + // cap-lints flag is ignored when using `clippy --fix` for now + // So it needs to be passed directly to rustc + // see https://github.com/rust-lang/rust-clippy/issues/9703 + let rustc_flags = if lint_filter.is_empty() { + clippy_args.extend(["-Wclippy::pedantic", "-Wclippy::cargo"]); + "--cap-lints=warn".to_string() } else { - clippy_args.push("--cap-lints=allow"); - clippy_args.extend(lint_filter.iter().map(std::string::String::as_str)); - } + let mut lint_filter_buf = lint_filter.clone(); + lint_filter_buf.push("--cap-lints=allow".to_string()); + lint_filter_buf.join(" ") + }; if let Some(server) = server { let target = shared_target_dir.join("recursive"); @@ -396,6 +411,7 @@ impl Crate { let all_output = Command::new(&cargo_clippy_path) // use the looping index to create individual target dirs .env("CARGO_TARGET_DIR", shared_target_dir.join(format!("_{thread_index:?}"))) + .env("RUSTFLAGS", rustc_flags) .args(&cargo_clippy_args) .current_dir(&self.path) .output() @@ -807,6 +823,14 @@ fn clippy_project_root() -> &'static Path { Path::new(env!("CARGO_MANIFEST_DIR")).parent().unwrap() } +fn has_been_modified(extracted_krate_dir: &Path) -> bool { + extracted_krate_dir.metadata().map_or(false, |metadata| { + metadata.created().map_or(false, |created| { + metadata.modified().map_or(false, |modified| created != modified) + }) + }) +} + #[test] fn lintcheck_test() { let args = [