From ff323940e549a0d8e36ce2c108972e7502ee2fa1 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Fri, 6 May 2022 19:46:22 -0500 Subject: [PATCH 1/2] [wip] Fix `x test clippy --stage 0` This makes a lot of changes. The basic idea is that clippy cannot depend on the sysroot it is *built* with to be the same as the sysroot it is *run* with. This assumption is hard-coded in lots of places throughout clippy, which is why the changes are so extensive. - Change clippy to use the sysroot of the run compiler (stage1, not stage0-sysroot) when running tests. This fixes an issue where clippy would try to load libraries from the beta compiler, then complain about a version mismatch. - Change clippy to be able to find host macros. Previously it assumed macros and dependencies were stored in the same target directory, which is not true for stage 0 (macros are in `release`, but other dependencies are in `release/x86_64-unknown-linux-gnu`). - Don't build rustc twice. The naive way to get dependencies into the stage1/ sysroot is to just compile everything we were doing before with a later compiler, but that takes a very long while to run. Instead: - Add a new crate in the clippy repo that has only the dependencies needed for UI tests. That allows compiling those crates without also compiling clippy and the whole compiler. - Don't require `clippy_lints` to be present when running tests in rust-lang/rust. That crate is only used for dogfood lints, which aren't even run in-tree. - Change various tests to remove usage of `rustc_*` crates. In all cases they were not testing the rustc crates themselves, they just happened to be conveniently in the sysroot. This currently breaks `--bless` because clippy_dev requires `rustc_lexer`. I hope to fix this shortly when the "build a single crate" PR lands; this PR is a WIP until then. --- Cargo.lock | 18 ++++++++ Cargo.toml | 1 + src/bootstrap/test.rs | 44 +++++++++++++++---- src/bootstrap/tool.rs | 12 +++++ src/tools/clippy/Cargo.toml | 1 + src/tools/clippy/tests/compile-test.rs | 31 +++++++++++-- .../tests/ui/auxiliary/proc_macro_derive.rs | 2 +- .../clippy/tests/ui/useless_attribute.fixed | 6 +-- .../clippy/tests/ui/useless_attribute.rs | 6 +-- .../clippy/ui-test-dependencies/Cargo.toml | 20 +++++++++ .../clippy/ui-test-dependencies/src/lib.rs | 0 11 files changed, 123 insertions(+), 18 deletions(-) create mode 100644 src/tools/clippy/ui-test-dependencies/Cargo.toml create mode 100644 src/tools/clippy/ui-test-dependencies/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index acda64172b10a..b6089f50fc6bf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -677,6 +677,7 @@ dependencies = [ "termize", "tester", "tokio", + "ui-test-dependencies", ] [[package]] @@ -5524,6 +5525,23 @@ version = "0.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "56dee185309b50d1f11bfedef0fe6d036842e3fb77413abef29f8f8d1c5d4c1c" +[[package]] +name = "ui-test-dependencies" +version = "0.1.0" +dependencies = [ + "derive-new", + "futures 0.3.19", + "if_chain", + "itertools", + "parking_lot 0.11.2", + "quote", + "regex", + "rustc-semver", + "serde", + "syn", + "tokio", +] + [[package]] name = "ui_test" version = "0.1.0" diff --git a/Cargo.toml b/Cargo.toml index 4e78399606445..7809b01a0cf45 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,6 +8,7 @@ members = [ "src/rustdoc-json-types", "src/tools/cargotest", "src/tools/clippy", + "src/tools/clippy/ui-test-dependencies", "src/tools/clippy/clippy_dev", "src/tools/compiletest", "src/tools/error_index_generator", diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index fdce078bbedf5..d881e7e687e77 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -642,14 +642,38 @@ impl Step for Clippy { fn run(self, builder: &Builder<'_>) { let stage = self.stage; let host = self.host; - let compiler = builder.compiler(stage, host); + let build_compiler = builder.compiler(stage, host); builder - .ensure(tool::Clippy { compiler, target: self.host, extra_features: Vec::new() }) + .ensure(tool::Clippy { + compiler: build_compiler, + target: self.host, + extra_features: Vec::new(), + }) .expect("in-tree tool"); + + // We linked Clippy with the previous stage, but now we need to run it with the *next* stage, + // so that it looks at the proper sysroot when loading libstd. + let run_compiler = builder.compiler(stage + 1, host); + builder.ensure(compile::Std { compiler: run_compiler, target: run_compiler.host }); + // Clippy uses some of its own dependencies in UI tests. Build those (but only those) with the same toolchain we'll run clippy with. + let mut dummy_bin_cargo = tool::prepare_tool_cargo( + builder, + run_compiler, + // Use ToolRustc to make sure the artifacts are placed in the same directory that clippy looks in during tests. + Mode::ToolRustc, + host, + "test", + "src/tools/clippy/ui-test-dependencies", + SourceType::InTree, + &[], + ); + dummy_bin_cargo.arg("-p=ui-test-dependencies"); + builder.run(&mut dummy_bin_cargo.into()); + let mut cargo = tool::prepare_tool_cargo( builder, - compiler, + build_compiler, Mode::ToolRustc, host, "test", @@ -658,14 +682,16 @@ impl Step for Clippy { &[], ); - cargo.env("RUSTC_TEST_SUITE", builder.rustc(compiler)); - cargo.env("RUSTC_LIB_PATH", builder.rustc_libdir(compiler)); - let host_libs = builder.stage_out(compiler, Mode::ToolRustc).join(builder.cargo_dir()); + cargo.env("RUSTC_TEST_SUITE", builder.rustc(run_compiler)); + cargo.env("RUSTC_LIB_PATH", builder.rustc_libdir(run_compiler)); + let host_libs = builder.stage_out(run_compiler, Mode::ToolRustc).join(builder.cargo_dir()); cargo.env("HOST_LIBS", host_libs); + let target_libs = builder.cargo_out(run_compiler, Mode::ToolRustc, run_compiler.host); + cargo.env("TARGET_LIBS", target_libs); cargo.arg("--").args(builder.config.cmd.test_args()); - cargo.add_rustc_lib_path(builder, compiler); + cargo.add_rustc_lib_path(builder, build_compiler); if builder.try_run(&mut cargo.into()) { // The tests succeeded; nothing to do. @@ -676,7 +702,9 @@ impl Step for Clippy { std::process::exit(1); } - let mut cargo = builder.cargo(compiler, Mode::ToolRustc, SourceType::InTree, host, "run"); + // FIXME: build rustc_lexer so we can support --bless. + let mut cargo = + builder.cargo(run_compiler, Mode::ToolRustc, SourceType::InTree, host, "run"); cargo.arg("-p").arg("clippy_dev"); // clippy_dev gets confused if it can't find `clippy/Cargo.toml` cargo.current_dir(&builder.src.join("src").join("tools").join("clippy")); diff --git a/src/bootstrap/tool.rs b/src/bootstrap/tool.rs index b7abf6cc78d71..d1cc4e95adee9 100644 --- a/src/bootstrap/tool.rs +++ b/src/bootstrap/tool.rs @@ -258,6 +258,18 @@ pub fn prepare_tool_cargo( // own copy cargo.env("LZMA_API_STATIC", "1"); + // clippy tests need to know about the stage sysroot. Set them consistently while building to + // avoid rebuilding when running tests. + if path.ends_with("clippy") { + // NOTE: this uses the compiler it will *run* with, not the compiler it is *built* with. + // See impl Step for test::Clippy for more details. + // NOTE: intentionally does *not* use `builder.compiler` - we don't actually want to build + // the compiler or create the sysroot, just make sure that clippy knows which path to use + // for the sysroot. + let run_compiler = Compiler { stage: compiler.stage + 1, host: compiler.host }; + cargo.env("SYSROOT", builder.sysroot(run_compiler)); + } + // CFG_RELEASE is needed by rustfmt (and possibly other tools) which // import rustc-ap-rustc_attr which requires this to be set for the // `#[cfg(version(...))]` attribute. diff --git a/src/tools/clippy/Cargo.toml b/src/tools/clippy/Cargo.toml index e4060ce29a7b5..717ee9f19e95d 100644 --- a/src/tools/clippy/Cargo.toml +++ b/src/tools/clippy/Cargo.toml @@ -40,6 +40,7 @@ filetime = "0.2" rustc-workspace-hack = "1.0" # UI test dependencies +ui-test-dependencies = { path = "ui-test-dependencies" } clippy_utils = { path = "clippy_utils" } derive-new = "0.5" if_chain = "1.0" diff --git a/src/tools/clippy/tests/compile-test.rs b/src/tools/clippy/tests/compile-test.rs index 04c2eeff08b6f..5ce13582d6377 100644 --- a/src/tools/clippy/tests/compile-test.rs +++ b/src/tools/clippy/tests/compile-test.rs @@ -1,6 +1,7 @@ #![feature(test)] // compiletest_rs requires this attribute #![feature(once_cell)] #![feature(is_sorted)] +#![feature(drain_filter)] #![cfg_attr(feature = "deny-warnings", deny(warnings))] #![warn(rust_2018_idioms, unused_lifetimes)] @@ -70,7 +71,23 @@ extern crate tokio; /// dependencies that are not *directly* used by this test module require an /// `extern crate` declaration. static EXTERN_FLAGS: SyncLazy = SyncLazy::new(|| { - let current_exe_depinfo = { + let current_exe_depinfo = if let Some(lib_deps) = option_env!("TARGET_LIBS") { + let mut deps = std::fs::read_dir(Path::new(lib_deps).join("deps")).unwrap().into_iter().map(|x| { + let mut line = x.unwrap().path().into_os_string().into_string().unwrap(); + line.push_str(":\n"); // for parse_name_path + line + }).collect(); + + if let Some(host_deps) = option_env!("HOST_LIBS") { + deps += &*std::fs::read_dir(Path::new(host_deps).join("deps")).unwrap().into_iter().map(|x| { + let mut line = x.unwrap().path().into_os_string().into_string().unwrap(); + line.push_str(":\n"); // for parse_name_path + line + }).collect::(); + } + + deps + } else { let mut path = env::current_exe().unwrap(); path.set_extension("d"); fs::read_to_string(path).unwrap() @@ -101,11 +118,15 @@ static EXTERN_FLAGS: SyncLazy = SyncLazy::new(|| { } } } - let not_found: Vec<&str> = TEST_DEPENDENCIES + let mut not_found: Vec<&str> = TEST_DEPENDENCIES .iter() .copied() .filter(|n| !crates.contains_key(n)) .collect(); + if option_env!("RUSTC_TEST_SUITE").is_some() { + // Bootstrap doesn't build clippy_utils, but that's ok since clippy only uses it for dogfood tests. + not_found.drain_filter(|x| *x == "clippy_utils"); + } assert!( not_found.is_empty(), "dependencies not found in depinfo: {:?}\n\ @@ -146,10 +167,14 @@ fn base_config(test_dir: &str) -> compiletest::Config { let host_libs = option_env!("HOST_LIBS") .map(|p| format!(" -L dependency={}", Path::new(p).join("deps").display())) .unwrap_or_default(); + let target_libs = option_env!("TARGET_LIBS") + .map(|p| format!(" -L dependency={}", Path::new(p).join("deps").display())) + .unwrap_or_default(); config.target_rustcflags = Some(format!( - "--emit=metadata -Dwarnings -Zui-testing -L dependency={}{}{}", + "--emit=metadata -Dwarnings -Zui-testing -L dependency={}{}{}{}", deps_path.display(), host_libs, + target_libs, &*EXTERN_FLAGS, )); diff --git a/src/tools/clippy/tests/ui/auxiliary/proc_macro_derive.rs b/src/tools/clippy/tests/ui/auxiliary/proc_macro_derive.rs index ed7b17651e675..716309d64b07a 100644 --- a/src/tools/clippy/tests/ui/auxiliary/proc_macro_derive.rs +++ b/src/tools/clippy/tests/ui/auxiliary/proc_macro_derive.rs @@ -20,7 +20,7 @@ pub fn derive(_: TokenStream) -> TokenStream { let output = quote! { // Should not trigger `useless_attribute` #[allow(dead_code)] - extern crate rustc_middle; + extern crate alloc; }; output } diff --git a/src/tools/clippy/tests/ui/useless_attribute.fixed b/src/tools/clippy/tests/ui/useless_attribute.fixed index c23231a99e9f0..e29c2eb6dcf65 100644 --- a/src/tools/clippy/tests/ui/useless_attribute.fixed +++ b/src/tools/clippy/tests/ui/useless_attribute.fixed @@ -8,11 +8,11 @@ #![allow(dead_code)] #![cfg_attr(feature = "cargo-clippy", allow(dead_code))] #[rustfmt::skip] -#[allow(unused_imports)] #[allow(unused_extern_crates)] -#[macro_use] -extern crate rustc_middle; +extern crate alloc; +// don't lint on unused_import when `macro_use` is present +#[allow(unused_imports)] #[macro_use] extern crate proc_macro_derive; diff --git a/src/tools/clippy/tests/ui/useless_attribute.rs b/src/tools/clippy/tests/ui/useless_attribute.rs index 7a7b198ea6078..e14241ae02074 100644 --- a/src/tools/clippy/tests/ui/useless_attribute.rs +++ b/src/tools/clippy/tests/ui/useless_attribute.rs @@ -8,11 +8,11 @@ #[allow(dead_code)] #[cfg_attr(feature = "cargo-clippy", allow(dead_code))] #[rustfmt::skip] -#[allow(unused_imports)] #[allow(unused_extern_crates)] -#[macro_use] -extern crate rustc_middle; +extern crate alloc; +// don't lint on unused_import when `macro_use` is present +#[allow(unused_imports)] #[macro_use] extern crate proc_macro_derive; diff --git a/src/tools/clippy/ui-test-dependencies/Cargo.toml b/src/tools/clippy/ui-test-dependencies/Cargo.toml new file mode 100644 index 0000000000000..a24a45f39c034 --- /dev/null +++ b/src/tools/clippy/ui-test-dependencies/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "ui-test-dependencies" +version = "0.1.0" +edition = "2021" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] +# clippy_utils = { path = "../clippy_utils" } +regex = "1.5" +derive-new = "0.5" +if_chain = "1.0" +itertools = "0.10.1" +quote = "1.0" +serde = { version = "1.0.125", features = ["derive"] } +syn = { version = "1.0", features = ["full"] } +futures = "0.3" +parking_lot = "0.11.2" +tokio = { version = "1", features = ["io-util"] } +rustc-semver = "1.1" diff --git a/src/tools/clippy/ui-test-dependencies/src/lib.rs b/src/tools/clippy/ui-test-dependencies/src/lib.rs new file mode 100644 index 0000000000000..e69de29bb2d1d From 8dcc51c78d58a5bb9ab91953720eada949d6e8a4 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 7 May 2022 10:41:51 -0500 Subject: [PATCH 2/2] Try removing UI test dependencies from clippy itself and see what breaks --- Cargo.lock | 12 +-------- src/tools/clippy/Cargo.toml | 10 -------- src/tools/clippy/tests/compile-test.rs | 25 +------------------ .../clippy/ui-test-dependencies/Cargo.toml | 2 +- 4 files changed, 3 insertions(+), 46 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b6089f50fc6bf..3335a0be31f3e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -659,24 +659,14 @@ dependencies = [ "clippy_lints", "clippy_utils", "compiletest_rs", - "derive-new", "filetime", - "futures 0.3.19", - "if_chain", - "itertools", - "parking_lot 0.12.1", - "quote", "regex", - "rustc-semver", "rustc-workspace-hack", "rustc_tools_util 0.2.0", "semver", - "serde", - "syn", "tempfile", "termize", "tester", - "tokio", "ui-test-dependencies", ] @@ -5533,7 +5523,7 @@ dependencies = [ "futures 0.3.19", "if_chain", "itertools", - "parking_lot 0.11.2", + "parking_lot 0.12.1", "quote", "regex", "rustc-semver", diff --git a/src/tools/clippy/Cargo.toml b/src/tools/clippy/Cargo.toml index 717ee9f19e95d..6b89ed828d8fa 100644 --- a/src/tools/clippy/Cargo.toml +++ b/src/tools/clippy/Cargo.toml @@ -42,16 +42,6 @@ rustc-workspace-hack = "1.0" # UI test dependencies ui-test-dependencies = { path = "ui-test-dependencies" } clippy_utils = { path = "clippy_utils" } -derive-new = "0.5" -if_chain = "1.0" -itertools = "0.10.1" -quote = "1.0" -serde = { version = "1.0.125", features = ["derive"] } -syn = { version = "1.0", features = ["full"] } -futures = "0.3" -parking_lot = "0.12" -tokio = { version = "1", features = ["io-util"] } -rustc-semver = "1.1" [build-dependencies] rustc_tools_util = { version = "0.2", path = "rustc_tools_util" } diff --git a/src/tools/clippy/tests/compile-test.rs b/src/tools/clippy/tests/compile-test.rs index 5ce13582d6377..c9b8e5c124f80 100644 --- a/src/tools/clippy/tests/compile-test.rs +++ b/src/tools/clippy/tests/compile-test.rs @@ -39,29 +39,6 @@ static TEST_DEPENDENCIES: &[&str] = &[ "rustc_semver", ]; -// Test dependencies may need an `extern crate` here to ensure that they show up -// in the depinfo file (otherwise cargo thinks they are unused) -#[allow(unused_extern_crates)] -extern crate clippy_utils; -#[allow(unused_extern_crates)] -extern crate derive_new; -#[allow(unused_extern_crates)] -extern crate futures; -#[allow(unused_extern_crates)] -extern crate if_chain; -#[allow(unused_extern_crates)] -extern crate itertools; -#[allow(unused_extern_crates)] -extern crate parking_lot; -#[allow(unused_extern_crates)] -extern crate quote; -#[allow(unused_extern_crates)] -extern crate rustc_semver; -#[allow(unused_extern_crates)] -extern crate syn; -#[allow(unused_extern_crates)] -extern crate tokio; - /// Produces a string with an `--extern` flag for all UI test crate /// dependencies. /// @@ -124,7 +101,7 @@ static EXTERN_FLAGS: SyncLazy = SyncLazy::new(|| { .filter(|n| !crates.contains_key(n)) .collect(); if option_env!("RUSTC_TEST_SUITE").is_some() { - // Bootstrap doesn't build clippy_utils, but that's ok since clippy only uses it for dogfood tests. + // Bootstrap doesn't build clippy_utils, but that's ok since clippy only uses it for internal UI tests. not_found.drain_filter(|x| *x == "clippy_utils"); } assert!( diff --git a/src/tools/clippy/ui-test-dependencies/Cargo.toml b/src/tools/clippy/ui-test-dependencies/Cargo.toml index a24a45f39c034..bbe297efdadee 100644 --- a/src/tools/clippy/ui-test-dependencies/Cargo.toml +++ b/src/tools/clippy/ui-test-dependencies/Cargo.toml @@ -15,6 +15,6 @@ quote = "1.0" serde = { version = "1.0.125", features = ["derive"] } syn = { version = "1.0", features = ["full"] } futures = "0.3" -parking_lot = "0.11.2" +parking_lot = "0.12" tokio = { version = "1", features = ["io-util"] } rustc-semver = "1.1"