From 8c05a15f0a5b541a5eb29f88f2d6dffa5888a2ce Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Fri, 3 Sep 2021 15:24:29 -0500 Subject: [PATCH] Use binary-dep-depinfo to resolve UI dependencies --- .cargo/config | 3 +- Cargo.toml | 13 ++-- tests/compile-test.rs | 152 +++++++++++++++++++++--------------------- 3 files changed, 88 insertions(+), 80 deletions(-) diff --git a/.cargo/config b/.cargo/config index e95ea224cb6..84ae36a46d7 100644 --- a/.cargo/config +++ b/.cargo/config @@ -5,4 +5,5 @@ lintcheck = "run --target-dir lintcheck/target --package lintcheck --bin lintche collect-metadata = "test --test dogfood --features metadata-collector-lint -- run_metadata_collection_lint --ignored" [build] -rustflags = ["-Zunstable-options"] +# -Zbinary-dep-depinfo allows us to track which rlib files to use for compiling UI tests +rustflags = ["-Zunstable-options", "-Zbinary-dep-depinfo"] diff --git a/Cargo.toml b/Cargo.toml index 20327573db7..2310370fb9f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,11 +32,7 @@ tempfile = { version = "3.1.0", optional = true } cargo_metadata = "0.12" compiletest_rs = { version = "0.6.0", features = ["tmp"] } tester = "0.9" -serde = { version = "1.0", features = ["derive"] } -derive-new = "0.5" regex = "1.4" -quote = "1" -syn = { version = "1", features = ["full"] } # This is used by the `collect-metadata` alias. filetime = "0.2" @@ -45,6 +41,15 @@ filetime = "0.2" # for more information. rustc-workspace-hack = "1.0.0" +# UI test dependencies +clippy_utils = { path = "clippy_utils" } +derive-new = "0.5" +if_chain = "1.0" +itertools = "0.10.1" +quote = "1" +serde = { version = "1.0", features = ["derive"] } +syn = { version = "1", features = ["full"] } + [build-dependencies] rustc_tools_util = { version = "0.2.0", path = "rustc_tools_util" } diff --git a/tests/compile-test.rs b/tests/compile-test.rs index 6116acffe07..74fe6f548a1 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -1,10 +1,10 @@ #![feature(test)] // compiletest_rs requires this attribute #![feature(once_cell)] -#![feature(try_blocks)] use compiletest_rs as compiletest; use compiletest_rs::common::Mode as TestMode; +use std::collections::HashMap; use std::env::{self, remove_var, set_var, var_os}; use std::ffi::{OsStr, OsString}; use std::fs; @@ -16,6 +16,28 @@ // whether to run internal tests or not const RUN_INTERNAL_TESTS: bool = cfg!(feature = "internal-lints"); +/// All crates used in UI tests are listed here +static TEST_DEPENDENCIES: &[&str] = &[ + "clippy_utils", + "derive_new", + "if_chain", + "itertools", + "quote", + "regex", + "serde", + "serde_derive", + "syn", +]; + +// Test dependencies may need an `extern crate` here to ensure that they show up +// in the depinfo file (otherwise cargo thinks they are unused) +extern crate clippy_utils; +extern crate derive_new; +extern crate if_chain; +extern crate itertools; +extern crate quote; +extern crate syn; + fn host_lib() -> PathBuf { option_env!("HOST_LIBS").map_or(cargo::CARGO_TARGET_DIR.join(env!("PROFILE")), PathBuf::from) } @@ -24,72 +46,58 @@ fn clippy_driver_path() -> PathBuf { option_env!("CLIPPY_DRIVER_PATH").map_or(cargo::TARGET_LIB.join("clippy-driver"), PathBuf::from) } -// When we'll want to use `extern crate ..` for a dependency that is used -// both by the crate and the compiler itself, we can't simply pass -L flags -// as we'll get a duplicate matching versions. Instead, disambiguate with -// `--extern dep=path`. -// See https://github.com/rust-lang/rust-clippy/issues/4015. -// -// FIXME: We cannot use `cargo build --message-format=json` to resolve to dependency files. -// Because it would force-rebuild if the options passed to `build` command is not the same -// as what we manually pass to `cargo` invocation -fn third_party_crates() -> String { - use std::collections::HashMap; - static CRATES: &[&str] = &[ - "clippy_lints", - "clippy_utils", - "if_chain", - "itertools", - "quote", - "regex", - "serde", - "serde_derive", - "syn", - ]; - let dep_dir = cargo::TARGET_LIB.join("deps"); - let mut crates: HashMap<&str, Vec> = HashMap::with_capacity(CRATES.len()); - let mut flags = String::new(); - for entry in fs::read_dir(dep_dir).unwrap().flatten() { - let path = entry.path(); - if let Some(name) = try { - let name = path.file_name()?.to_str()?; - let (name, _) = name.strip_suffix(".rlib")?.strip_prefix("lib")?.split_once('-')?; - CRATES.iter().copied().find(|&c| c == name)? - } { - flags += &format!(" --extern {}={}", name, path.display()); - crates.entry(name).or_default().push(path.clone()); +/// Produces a string with an `--extern` flag for all UI test crate +/// dependencies. +/// +/// The dependency files are located by parsing the depinfo file for this test +/// module. This assumes the `-Z binary-dep-depinfo` flag is enabled. All test +/// dependencies must be added to Cargo.toml at the project root. Test +/// dependencies that are not *directly* used by this test module require an +/// `extern crate` declaration. +fn extern_flags() -> String { + let current_exe_depinfo = { + let mut path = env::current_exe().unwrap(); + path.set_extension("d"); + std::fs::read_to_string(path).unwrap() + }; + let mut crates: HashMap<&str, &str> = HashMap::with_capacity(TEST_DEPENDENCIES.len()); + for line in current_exe_depinfo.lines() { + // each dependency is expected to have a Makefile rule like `/path/to/crate-hash.rlib:` + let parse_name_path = || { + if line.starts_with(char::is_whitespace) { + return None; + } + let path_str = line.strip_suffix(':')?; + let path = Path::new(path_str); + if !matches!(path.extension()?.to_str()?, "rlib" | "so" | "dylib" | "dll") { + return None; + } + let (name, _hash) = path.file_stem()?.to_str()?.rsplit_once('-')?; + // the "lib" prefix is not present for dll files + let name = name.strip_prefix("lib").unwrap_or(name); + Some((name, path_str)) + }; + if let Some((name, path)) = parse_name_path() { + if TEST_DEPENDENCIES.contains(&name) { + // A dependency may be listed twice if it is available in sysroot, + // and the sysroot dependencies are listed first. As of the writing, + // this only seems to apply to if_chain. + crates.insert(name, path); + } } } - crates.retain(|_, paths| paths.len() > 1); - if !crates.is_empty() { - let crate_names = crates.keys().map(|s| format!("`{}`", s)).collect::>().join(", "); - // add backslashes for an easy copy-paste `rm` command - let paths = crates - .into_values() - .flatten() - .map(|p| strip_current_dir(&p).display().to_string()) - .collect::>() - .join(" \\\n"); - // Check which action should be done in order to remove compiled deps. - // If pre-installed version of compiler is used, `cargo clean` will do. - // Otherwise (for bootstrapped compiler), the dependencies directory - // must be removed manually. - let suggested_action = if std::env::var_os("RUSTC_BOOTSTRAP").is_some() { - "removing the stageN-tools directory" - } else { - "running `cargo clean`" - }; - - panic!( - "\n----------------------------------------------------------------------\n\ - ERROR: Found multiple rlibs for crates: {}\n\ - Try {} or remove the following files:\n\n{}\n\n\ - For details on this error see https://github.com/rust-lang/rust-clippy/issues/7343\n\ - ----------------------------------------------------------------------\n", - crate_names, suggested_action, paths - ); + let not_found: Vec<&str> = TEST_DEPENDENCIES + .iter() + .copied() + .filter(|n| !crates.contains_key(n)) + .collect(); + if !not_found.is_empty() { + panic!("dependencies not found in depinfo: {:?}", not_found); } - flags + crates + .into_iter() + .map(|(name, path)| format!("--extern {}={} ", name, path)) + .collect() } fn default_config() -> compiletest::Config { @@ -105,11 +113,14 @@ fn default_config() -> compiletest::Config { config.compile_lib_path = path; } + // Using `-L dependency={}` enforces that external dependencies are added with `--extern`. + // This is valuable because a) it allows us to monitor what external dependencies are used + // and b) it ensures that conflicting rlibs are resolved properly. config.target_rustcflags = Some(format!( - "--emit=metadata -L {0} -L {1} -Dwarnings -Zui-testing {2}", + "--emit=metadata -L dependency={} -L dependency={} -Dwarnings -Zui-testing {}", host_lib().join("deps").display(), cargo::TARGET_LIB.join("deps").display(), - third_party_crates(), + extern_flags(), )); config.build_base = host_lib().join("test_build_base"); @@ -316,12 +327,3 @@ fn drop(&mut self) { } } } - -fn strip_current_dir(path: &Path) -> &Path { - if let Ok(curr) = env::current_dir() { - if let Ok(stripped) = path.strip_prefix(curr) { - return stripped; - } - } - path -}