Skip to content

improve lintcheck #9731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

improve lintcheck #9731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5461e99
Better logging for `lintcheck`
kraktus Sep 10, 2022
3a2c1e5
Fix `lintcheck --fix`
kraktus Oct 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lintcheck/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
31 changes: 30 additions & 1 deletion lintcheck/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -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 <foo>
Expand All @@ -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" }
));
Expand Down
88 changes: 56 additions & 32 deletions lintcheck/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -163,10 +164,10 @@ fn get(path: &str) -> Result<ureq::Response, ureq::Error> {
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;
}
Expand All @@ -181,33 +182,35 @@ 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");
println!("Downloading and extracting {name} {version} from {url}");
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(),
}
},
Expand All @@ -234,7 +237,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
Expand All @@ -247,7 +250,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 {
Expand Down Expand Up @@ -298,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 {
Expand Down Expand Up @@ -337,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", "--"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're in the neighbourhood, could change this bit to

Suggested change
vec!["--", "--message-format=json", "--"]
vec!["clippy", "--message-format=json", "--"]

I did find this quite confusing before the conversation in #9461

};
Expand All @@ -347,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");
Expand Down Expand Up @@ -390,9 +406,12 @@ 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:?}")))
.env("RUSTFLAGS", rustc_flags)
.args(&cargo_clippy_args)
.current_dir(&self.path)
.output()
Expand All @@ -409,21 +428,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
Expand All @@ -449,7 +466,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);
}
}
Expand Down Expand Up @@ -553,7 +570,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);
}

Expand Down Expand Up @@ -615,8 +632,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);
Expand Down Expand Up @@ -693,8 +710,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);
Expand Down Expand Up @@ -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 = [
Expand Down