From fe007179ec3560f86bd15686d60372f8307ca225 Mon Sep 17 00:00:00 2001 From: Philipp Krones Date: Thu, 12 Jan 2023 18:30:51 +0100 Subject: [PATCH] Add cargo-clippy sysroot test Whne SYSROOT is defined, clippy-driver will insert a --sysroot argument when calling rustc. However, when a sysroot argument is already defined, e.g. through RUSTFLAGS=--sysroot=... the `cargo clippy` call would error. This tests that the sysroot argument is only passed once and that SYSROOT is ignored in this case. --- .github/driver.sh | 7 ++++ tests/integration.rs | 98 +++++++++++++------------------------------- 2 files changed, 36 insertions(+), 69 deletions(-) diff --git a/.github/driver.sh b/.github/driver.sh index 6ff189fc859..798782340ee 100644 --- a/.github/driver.sh +++ b/.github/driver.sh @@ -17,6 +17,13 @@ test "$sysroot" = $desired_sysroot sysroot=$(SYSROOT=$desired_sysroot ./target/debug/clippy-driver --print sysroot) test "$sysroot" = $desired_sysroot +# Check that the --sysroot argument is only passed once (SYSROOT is ignored) +( + cd rustc_tools_util + touch src/lib.rs + SYSROOT=/tmp RUSTFLAGS="--sysroot=$(rustc --print sysroot)" ../target/debug/cargo-clippy clippy --verbose +) + # Make sure this isn't set - clippy-driver should cope without it unset CARGO_MANIFEST_DIR diff --git a/tests/integration.rs b/tests/integration.rs index 319e8eb2da6..a771d8b87c8 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -1,42 +1,28 @@ -//! To run this test, use +//! This test is meant to only be run in CI. To run it locally use: +//! //! `env INTEGRATION=rust-lang/log cargo test --test integration --features=integration` //! //! You can use a different `INTEGRATION` value to test different repositories. +//! +//! This test will clone the specified repository and run Clippy on it. The test succeeds, if +//! Clippy doesn't produce an ICE. Lint warnings are ignored by this test. #![cfg(feature = "integration")] #![cfg_attr(feature = "deny-warnings", deny(warnings))] #![warn(rust_2018_idioms, unused_lifetimes)] +use std::env; use std::ffi::OsStr; -use std::path::{Path, PathBuf}; use std::process::Command; -use std::{env, eprintln}; #[cfg(not(windows))] const CARGO_CLIPPY: &str = "cargo-clippy"; #[cfg(windows)] const CARGO_CLIPPY: &str = "cargo-clippy.exe"; -// NOTE: arguments passed to the returned command will be `clippy-driver` args, not `cargo-clippy` -// args. Use `cargo_args` to pass arguments to cargo-clippy. -fn clippy_command(repo_dir: &Path, cargo_args: &[&str]) -> Command { - let root_dir = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")); - let target_dir = option_env!("CARGO_TARGET_DIR").map_or_else(|| root_dir.join("target"), PathBuf::from); - let clippy_binary = target_dir.join(env!("PROFILE")).join(CARGO_CLIPPY); - - let mut cargo_clippy = Command::new(clippy_binary); - cargo_clippy - .current_dir(repo_dir) - .env("RUST_BACKTRACE", "full") - .env("CARGO_TARGET_DIR", root_dir.join("target")) - .args(["clippy", "--all-targets", "--all-features"]) - .args(cargo_args) - .args(["--", "--cap-lints", "warn", "-Wclippy::pedantic", "-Wclippy::nursery"]); - cargo_clippy -} - -/// Return a directory with a checkout of the repository in `INTEGRATION`. -fn repo_dir(repo_name: &str) -> PathBuf { +#[cfg_attr(feature = "integration", test)] +fn integration_test() { + let repo_name = env::var("INTEGRATION").expect("`INTEGRATION` var not set"); let repo_url = format!("https://github.com/{repo_name}"); let crate_name = repo_name .split('/') @@ -57,19 +43,28 @@ fn repo_dir(repo_name: &str) -> PathBuf { .expect("unable to run git"); assert!(st.success()); - repo_dir -} + let root_dir = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")); + let target_dir = std::path::Path::new(&root_dir).join("target"); + let clippy_binary = target_dir.join(env!("PROFILE")).join(CARGO_CLIPPY); + + let output = Command::new(clippy_binary) + .current_dir(repo_dir) + .env("RUST_BACKTRACE", "full") + .env("CARGO_TARGET_DIR", target_dir) + .args([ + "clippy", + "--all-targets", + "--all-features", + "--", + "--cap-lints", + "warn", + "-Wclippy::pedantic", + "-Wclippy::nursery", + ]) + .output() + .expect("unable to run clippy"); -#[cfg_attr(feature = "integration", test)] -fn integration_test() { - let repo_name = env::var("INTEGRATION").expect("`INTEGRATION` var not set"); - let repo_dir = repo_dir(&repo_name); - let output = clippy_command(&repo_dir, &[]).output().expect("failed to run clippy"); let stderr = String::from_utf8_lossy(&output.stderr); - if !stderr.is_empty() { - eprintln!("{stderr}"); - } - if let Some(backtrace_start) = stderr.find("error: internal compiler error") { static BACKTRACE_END_MSG: &str = "end of query stack"; let backtrace_end = stderr[backtrace_start..] @@ -104,38 +99,3 @@ fn integration_test() { None => panic!("Process terminated by signal"), } } - -#[cfg_attr(feature = "integration", test)] -fn test_sysroot() { - #[track_caller] - fn verify_cmd(cmd: &mut Command) { - // Test that SYSROOT is ignored if `--sysroot` is passed explicitly. - cmd.env("SYSROOT", "/dummy/value/does/not/exist"); - // We don't actually care about emitting lints, we only want to verify clippy doesn't give a hard - // error. - cmd.arg("-Awarnings"); - let output = cmd.output().expect("failed to run clippy"); - let stderr = String::from_utf8_lossy(&output.stderr); - assert!(stderr.is_empty(), "clippy printed an error: {stderr}"); - assert!(output.status.success(), "clippy exited with an error"); - } - - let rustc = std::env::var("RUSTC").unwrap_or("rustc".to_string()); - let rustc_output = Command::new(rustc) - .args(["--print", "sysroot"]) - .output() - .expect("unable to run rustc"); - assert!(rustc_output.status.success()); - let sysroot = String::from_utf8(rustc_output.stdout).unwrap(); - let sysroot = sysroot.trim_end(); - - // This is a fairly small repo; we want to avoid checking out anything heavy twice, so just - // hard-code it. - let repo_name = "rust-lang/log"; - let repo_dir = repo_dir(repo_name); - // Pass the sysroot through RUSTFLAGS. - verify_cmd(clippy_command(&repo_dir, &["--quiet"]).env("RUSTFLAGS", format!("--sysroot={sysroot}"))); - // NOTE: we don't test passing the arguments directly to clippy-driver (with `-- --sysroot`) - // because it breaks for some reason. I (@jyn514) haven't taken time to track down the bug - // because rust-lang/rust uses RUSTFLAGS and nearly no one else uses --sysroot. -}