From b5fd0108b3a6b8a9626645ac174cf653de576135 Mon Sep 17 00:00:00 2001 From: Jeremy Fitzhardinge Date: Tue, 15 Jan 2019 10:56:09 -0800 Subject: [PATCH 1/6] clippy-driver: more robust test to see if we're clippy-enabled Rather than looking for a fixed --emit arg set, just check to see if we're emitting metadata at all. This makes it more robust to being invoked by tools other than cargo (or if cargo changes its invocation). Issue #3663 --- Cargo.toml | 1 - src/driver.rs | 42 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 868f21c1a498..c37104850273 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,7 +34,6 @@ path = "src/main.rs" [[bin]] name = "clippy-driver" -test = false path = "src/driver.rs" [dependencies] diff --git a/src/driver.rs b/src/driver.rs index b41895f3c2bd..d290e403a315 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -20,6 +20,46 @@ fn show_version() { println!(env!("CARGO_PKG_VERSION")); } +/// If a command-line option matches `find_arg`, then apply the predicate `pred` on its value. If +/// true, then return it. The parameter is assumed to be either `--arg=value` or `--arg value`. +fn arg_value<'a>( + args: impl IntoIterator, + find_arg: &str, + pred: impl Fn(&str) -> bool, +) -> Option<&'a str> { + let mut args = args.into_iter().map(String::as_str); + + while let Some(arg) = args.next() { + let arg: Vec<_> = arg.splitn(2, '=').collect(); + if arg.get(0) != Some(&find_arg) { + continue; + } + + let value = arg.get(1).cloned().or_else(|| args.next()); + if value.as_ref().map_or(false, |p| pred(p)) { + return value; + } + } + None +} + +#[test] +fn test_arg_value() { + let args: Vec<_> = ["--bar=bar", "--foobar", "123", "--foo"] + .iter() + .map(|s| s.to_string()) + .collect(); + + assert_eq!(arg_value(None, "--foobar", |_| true), None); + assert_eq!(arg_value(&args, "--bar", |_| false), None); + assert_eq!(arg_value(&args, "--bar", |_| true), Some("bar")); + assert_eq!(arg_value(&args, "--bar", |p| p == "bar"), Some("bar")); + assert_eq!(arg_value(&args, "--bar", |p| p == "foo"), None); + assert_eq!(arg_value(&args, "--foobar", |p| p == "foo"), None); + assert_eq!(arg_value(&args, "--foobar", |p| p == "123"), Some("123")); + assert_eq!(arg_value(&args, "--foo", |_| true), None); +} + #[allow(clippy::too_many_lines)] pub fn main() { rustc_driver::init_rustc_env_logger(); @@ -79,7 +119,7 @@ pub fn main() { // crate is // linted but not built let clippy_enabled = env::var("CLIPPY_TESTS").ok().map_or(false, |val| val == "true") - || orig_args.iter().any(|s| s == "--emit=dep-info,metadata"); + || arg_value(&orig_args, "--emit", |val| val.split(',').any(|e| e == "metadata")).is_some(); if clippy_enabled { args.extend_from_slice(&["--cfg".to_owned(), r#"feature="cargo-clippy""#.to_owned()]); From 71d03ae29b175c5e99f4e978e90bab830cfbd530 Mon Sep 17 00:00:00 2001 From: Jeremy Fitzhardinge Date: Tue, 15 Jan 2019 11:39:23 -0800 Subject: [PATCH 2/6] clippy-driver: if --sysroot is specified on the command line, use that If the user explicitly sets sysroot on the command line, then use that value. Issue #3663 --- src/driver.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/driver.rs b/src/driver.rs index d290e403a315..fbff693f8870 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -72,8 +72,19 @@ pub fn main() { exit(0); } - let sys_root = option_env!("SYSROOT") - .map(String::from) + let mut orig_args: Vec = env::args().collect(); + + // Get the sysroot, looking from most specific to this invocation to the least: + // - command line + // - runtime environment + // - SYSROOT + // - RUSTUP_HOME, MULTIRUST_HOME, RUSTUP_TOOLCHAIN, MULTIRUST_TOOLCHAIN + // - sysroot from rustc in the path + // - compile-time environment + let sys_root_arg = arg_value(&orig_args, "--sysroot", |_| true); + let have_sys_root_arg = sys_root_arg.is_some(); + let sys_root = sys_root_arg + .map(|s| s.to_string()) .or_else(|| std::env::var("SYSROOT").ok()) .or_else(|| { let home = option_env!("RUSTUP_HOME").or(option_env!("MULTIRUST_HOME")); @@ -89,11 +100,11 @@ pub fn main() { .and_then(|out| String::from_utf8(out.stdout).ok()) .map(|s| s.trim().to_owned()) }) + .or_else(|| option_env!("SYSROOT").map(String::from)) .expect("need to specify SYSROOT env var during clippy compilation, or use rustup or multirust"); // Setting RUSTC_WRAPPER causes Cargo to pass 'rustc' as the first argument. // We're invoking the compiler programmatically, so we ignore this/ - let mut orig_args: Vec = env::args().collect(); if orig_args.len() <= 1 { std::process::exit(1); } @@ -104,7 +115,7 @@ pub fn main() { // this conditional check for the --sysroot flag is there so users can call // `clippy_driver` directly // without having to pass --sysroot or anything - let mut args: Vec = if orig_args.iter().any(|s| s == "--sysroot") { + let mut args: Vec = if have_sys_root_arg { orig_args.clone() } else { orig_args From 86c513e605e7982c00587e05a33a1062ca9a5334 Mon Sep 17 00:00:00 2001 From: Jeremy Fitzhardinge Date: Sat, 26 Jan 2019 16:11:30 -0800 Subject: [PATCH 3/6] Let CLIPPY_CONF_DIR be used to start search for config, and fall back to CARGO_MANIFEST_DIR if it isn't set. If CARGO_MANIFEST_DIR isn't set, fall back "." rather than panicing. Issue #3663 --- clippy_lints/src/utils/conf.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 09d204a562c9..b0b4394ebb88 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -163,8 +163,13 @@ pub fn lookup_conf_file() -> io::Result> { /// Possible filename to search for. const CONFIG_FILE_NAMES: [&str; 2] = [".clippy.toml", "clippy.toml"]; - let mut current = path::PathBuf::from(env::var("CARGO_MANIFEST_DIR").expect("CARGO_MANIFEST_DIR not set")); - + // Start looking for a config file in CLIPPY_CONF_DIR, or failing that, CARGO_MANIFEST_DIR. + // If neither of those exist, use ".". + let mut current = path::PathBuf::from( + env::var("CLIPPY_CONF_DIR") + .or_else(|_| env::var("CARGO_MANIFEST_DIR")) + .unwrap_or_else(|_| ".".to_string()), + ); loop { for config_file_name in &CONFIG_FILE_NAMES { let config_file = current.join(config_file_name); From 993e8ace8e82d28fc891d9cc84fa4b06754c3b58 Mon Sep 17 00:00:00 2001 From: Jeremy Fitzhardinge Date: Sat, 26 Jan 2019 16:44:52 -0800 Subject: [PATCH 4/6] Drive-by cleanups to cargo-clippy. No functional change. --- src/main.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/main.rs b/src/main.rs index 20466fc567d7..208262ca30f1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -61,10 +61,8 @@ where { let mut args = vec!["check".to_owned()]; - let mut found_dashes = false; for arg in old_args.by_ref() { - found_dashes |= arg == "--"; - if found_dashes { + if arg == "--" { break; } args.push(arg); @@ -82,11 +80,7 @@ where let target_dir = std::env::var_os("CLIPPY_DOGFOOD") .map(|_| { std::env::var_os("CARGO_MANIFEST_DIR").map_or_else( - || { - let mut fallback = std::ffi::OsString::new(); - fallback.push("clippy_dogfood"); - fallback - }, + || std::ffi::OsString::from("clippy_dogfood"), |d| { std::path::PathBuf::from(d) .join("target") From b07f1b097493809045c92bb3ca09b5792310e5a5 Mon Sep 17 00:00:00 2001 From: Jeremy Fitzhardinge Date: Sat, 26 Jan 2019 17:49:29 -0800 Subject: [PATCH 5/6] base-tests: use subshells to manage current directory It saves on having to pair `cd && think && cd ..`. --- ci/base-tests.sh | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/ci/base-tests.sh b/ci/base-tests.sh index 6675c795b3b2..17eec3a4149b 100755 --- a/ci/base-tests.sh +++ b/ci/base-tests.sh @@ -11,14 +11,15 @@ cargo build --features debugging cargo test --features debugging # for faster build, share target dir between subcrates export CARGO_TARGET_DIR=`pwd`/target/ -cd clippy_lints && cargo test && cd .. -cd rustc_tools_util && cargo test && cd .. -cd clippy_dev && cargo test && cd .. +(cd clippy_lints && cargo test) +(cd rustc_tools_util && cargo test) +(cd clippy_dev && cargo test) # make sure clippy can be called via ./path/to/cargo-clippy -cd clippy_workspace_tests -../target/debug/cargo-clippy -cd .. +( + cd clippy_workspace_tests + ../target/debug/cargo-clippy +) # Perform various checks for lint registration ./util/dev update_lints --check From f0131fbab6331eb44d88c94783be2ac9b87f8b06 Mon Sep 17 00:00:00 2001 From: Jeremy Fitzhardinge Date: Sat, 26 Jan 2019 18:24:45 -0800 Subject: [PATCH 6/6] Add a CI test for cargoless use of clippy-driver --- ci/base-tests.sh | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/ci/base-tests.sh b/ci/base-tests.sh index 17eec3a4149b..9b80960211dd 100755 --- a/ci/base-tests.sh +++ b/ci/base-tests.sh @@ -25,6 +25,31 @@ export CARGO_TARGET_DIR=`pwd`/target/ ./util/dev update_lints --check cargo +nightly fmt --all -- --check +# Check running clippy-driver without cargo +( + export LD_LIBRARY_PATH=$(rustc --print sysroot)/lib + + # Check sysroot handling + sysroot=$(./target/debug/clippy-driver --print sysroot) + test $sysroot = $(rustc --print sysroot) + + sysroot=$(./target/debug/clippy-driver --sysroot /tmp --print sysroot) + test $sysroot = /tmp + + sysroot=$(SYSROOT=/tmp ./target/debug/clippy-driver --print sysroot) + test $sysroot = /tmp + + # Make sure this isn't set - clippy-driver should cope without it + unset CARGO_MANIFEST_DIR + + # Run a lint and make sure it produces the expected output. It's also expected to exit with code 1 + # XXX How to match the clippy invocation in compile-test.rs? + ! ./target/debug/clippy-driver -Dwarnings -Aunused -Zui-testing --emit metadata --crate-type bin tests/ui/cstring.rs 2> cstring.stderr + diff <(sed -e 's,tests/ui,$DIR,' -e '/= help/d' cstring.stderr) tests/ui/cstring.stderr + + # TODO: CLIPPY_CONF_DIR / CARGO_MANIFEST_DIR +) + # make sure tests are formatted # some lints are sensitive to formatting, exclude some files