Skip to content

Commit

Permalink
Auto merge of rust-lang#12999 - ehuss:fix-clippy-wrapper-race, r=epage
Browse files Browse the repository at this point in the history
Fix clippy-wrapper test race condition.

This fixes an issue where if certain tests ran concurrently, they would stomp on each other writing to a global directory. The problem is that `wrapped_clippy_driver` was writing to a global directory without any locking. The solution is to use the existing locking setup we have for supporting similar global tools.

This doesn't often show up because the offending tests are usually separated enough alphabetically, but if they happen to run near each other, they would stomp on each other and corrupt the directory.
  • Loading branch information
bors committed Nov 19, 2023
2 parents 4ac051e + 2eac6f5 commit 879c564
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 27 deletions.
23 changes: 0 additions & 23 deletions crates/cargo-test-support/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -523,29 +523,6 @@ pub fn cargo_exe() -> PathBuf {
snapbox::cmd::cargo_bin("cargo")
}

/// A wrapper around `rustc` instead of calling `clippy`.
pub fn wrapped_clippy_driver() -> PathBuf {
let clippy_driver = project()
.at(paths::global_root().join("clippy-driver"))
.file("Cargo.toml", &basic_manifest("clippy-driver", "0.0.1"))
.file(
"src/main.rs",
r#"
fn main() {
let mut args = std::env::args_os();
let _me = args.next().unwrap();
let rustc = args.next().unwrap();
let status = std::process::Command::new(rustc).args(args).status().unwrap();
std::process::exit(status.code().unwrap_or(1));
}
"#,
)
.build();
clippy_driver.cargo("build").run();

clippy_driver.bin("clippy-driver")
}

/// This is the raw output from the process.
///
/// This is similar to `std::process::Output`, however the `status` is
Expand Down
32 changes: 32 additions & 0 deletions crates/cargo-test-support/src/tools.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use std::sync::OnceLock;

static ECHO_WRAPPER: OnceLock<Mutex<Option<PathBuf>>> = OnceLock::new();
static ECHO: OnceLock<Mutex<Option<PathBuf>>> = OnceLock::new();
static CLIPPY_DRIVER: OnceLock<Mutex<Option<PathBuf>>> = OnceLock::new();

/// Returns the path to an executable that works as a wrapper around rustc.
///
Expand Down Expand Up @@ -107,3 +108,34 @@ pub fn echo_subcommand() -> Project {
p.cargo("build").run();
p
}

/// A wrapper around `rustc` instead of calling `clippy`.
pub fn wrapped_clippy_driver() -> PathBuf {
let mut lock = CLIPPY_DRIVER
.get_or_init(|| Default::default())
.lock()
.unwrap();
if let Some(path) = &*lock {
return path.clone();
}
let clippy_driver = project()
.at(paths::global_root().join("clippy-driver"))
.file("Cargo.toml", &basic_manifest("clippy-driver", "0.0.1"))
.file(
"src/main.rs",
r#"
fn main() {
let mut args = std::env::args_os();
let _me = args.next().unwrap();
let rustc = args.next().unwrap();
let status = std::process::Command::new(rustc).args(args).status().unwrap();
std::process::exit(status.code().unwrap_or(1));
}
"#,
)
.build();
clippy_driver.cargo("build").run();
let path = clippy_driver.bin("clippy-driver");
*lock = Some(path.clone());
path
}
4 changes: 2 additions & 2 deletions tests/testsuite/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ use crate::messages::raw_rustc_output;
use cargo_test_support::install::exe;
use cargo_test_support::paths::CargoPathExt;
use cargo_test_support::registry::Package;
use cargo_test_support::tools;
use cargo_test_support::{basic_bin_manifest, basic_manifest, git, project};
use cargo_test_support::{tools, wrapped_clippy_driver};

#[cargo_test]
fn check_success() {
Expand Down Expand Up @@ -1432,7 +1432,7 @@ fn check_fixable_warning_for_clippy() {

foo.cargo("check")
// We can't use `clippy` so we use a `rustc` workspace wrapper instead
.env("RUSTC_WORKSPACE_WRAPPER", wrapped_clippy_driver())
.env("RUSTC_WORKSPACE_WRAPPER", tools::wrapped_clippy_driver())
.with_stderr_contains("[..] (run `cargo clippy --fix --lib -p foo` to apply 1 suggestion)")
.run();
}
Expand Down
4 changes: 2 additions & 2 deletions tests/testsuite/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ use cargo_test_support::compare::assert_match_exact;
use cargo_test_support::git::{self, init};
use cargo_test_support::paths::{self, CargoPathExt};
use cargo_test_support::registry::{Dependency, Package};
use cargo_test_support::tools;
use cargo_test_support::{basic_manifest, is_nightly, project, Project};
use cargo_test_support::{tools, wrapped_clippy_driver};

#[cargo_test]
fn do_not_fix_broken_builds() {
Expand Down Expand Up @@ -193,7 +193,7 @@ fn broken_clippy_fixes_backed_out() {
.env("__CARGO_FIX_YOLO", "1")
.env("RUSTC", p.root().join("foo/target/debug/foo"))
// We can't use `clippy` so we use a `rustc` workspace wrapper instead
.env("RUSTC_WORKSPACE_WRAPPER", wrapped_clippy_driver())
.env("RUSTC_WORKSPACE_WRAPPER", tools::wrapped_clippy_driver())
.with_stderr_contains(
"warning: failed to automatically apply fixes suggested by rustc \
to crate `bar`\n\
Expand Down

0 comments on commit 879c564

Please sign in to comment.