Skip to content

clippy: make tests work in stage 1 #144027

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 20 additions & 24 deletions src/bootstrap/src/core/build_steps/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -739,7 +739,6 @@ impl Step for CompiletestTest {

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct Clippy {
stage: u32,
host: TargetSelection,
}

Expand All @@ -753,33 +752,23 @@ impl Step for Clippy {
}

fn make_run(run: RunConfig<'_>) {
// If stage is explicitly set or not lower than 2, keep it. Otherwise, make sure it's at least 2
// as tests for this step don't work with a lower stage.
let stage = if run.builder.config.is_explicit_stage() || run.builder.top_stage >= 2 {
run.builder.top_stage
} else {
2
};

run.builder.ensure(Clippy { stage, host: run.target });
run.builder.ensure(Clippy { host: run.target });
}

/// Runs `cargo test` for clippy.
fn run(self, builder: &Builder<'_>) {
let stage = self.stage;
let stage = builder.top_stage;
let host = self.host;
let compiler = builder.compiler(stage, host);

if stage < 2 {
eprintln!("WARNING: clippy tests on stage {stage} may not behave well.");
eprintln!("HELP: consider using stage 2");
}
// We need to carefully distinguish the compiler that builds clippy, and the compiler
// that is linked into the clippy being tested. `target_compiler` is the latter,
// and it must also be used by clippy's test runner to build tests and their dependencies.
let target_compiler = builder.compiler(stage, host);

let tool_result = builder.ensure(tool::Clippy { compiler, target: self.host });
let compiler = tool_result.build_compiler;
let tool_result = builder.ensure(tool::Clippy { compiler: target_compiler, target: host });
let tool_compiler = tool_result.build_compiler;
let mut cargo = tool::prepare_tool_cargo(
builder,
compiler,
tool_compiler,
Mode::ToolRustc,
host,
Kind::Test,
Expand All @@ -788,11 +777,17 @@ 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(tool_compiler));
cargo.env("RUSTC_LIB_PATH", builder.rustc_libdir(tool_compiler));
let host_libs = builder.stage_out(tool_compiler, Mode::ToolRustc).join(builder.cargo_dir());
cargo.env("HOST_LIBS", host_libs);

// Build the standard library that the tests can use.
builder.std(target_compiler, host);
cargo.env("TEST_SYSROOT", builder.sysroot(target_compiler));
cargo.env("TEST_RUSTC", builder.rustc(target_compiler));
cargo.env("TEST_RUSTC_LIB", builder.rustc_libdir(target_compiler));

// Collect paths of tests to run
'partially_test: {
let paths = &builder.config.paths[..];
Expand All @@ -813,7 +808,8 @@ impl Step for Clippy {
cargo.add_rustc_lib_path(builder);
let cargo = prepare_cargo_test(cargo, &[], &[], host, builder);

let _guard = builder.msg_sysroot_tool(Kind::Test, compiler.stage, "clippy", host, host);
let _guard =
builder.msg_sysroot_tool(Kind::Test, tool_compiler.stage, "clippy", host, host);

// Clippy reports errors if it blessed the outputs
if cargo.allow_failure().run(builder) {
Expand Down
29 changes: 28 additions & 1 deletion src/tools/clippy/tests/compile-test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,31 @@ impl TestContext {
defaults.set_custom(
"dependencies",
DependencyBuilder {
program: CommandBuilder::cargo(),
program: {
let mut p = CommandBuilder::cargo();
// If we run in bootstrap, we need to use the right compiler for building the
// tests -- not the compiler that built clippy, but the compiler that got linked
// into clippy. Just invoking TEST_RUSTC does not work because LD_LIBRARY_PATH
// is set in a way that makes it pick the wrong sysroot. Sadly due to
// <https://github.com/rust-lang/cargo/issues/4423> we cannot use RUSTFLAGS to
// set `--sysroot`, so we need to use bootstrap's rustc wrapper. That wrapper
// however has some staging logic that is hurting us here, so to work around
// that we set both the "real" and "staging" rustc to TEST_RUSTC, including the
// associated library paths.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be possible to use a different approach here... somehow, aux builds are working for clippy tests. It seems to be using clippy itself as the compiler for that, and that's somehow okay? So, we could try to do the same here, instead of all this TEST_RUSTC stuff. (That would also be closer to what Miri does.)

But TBH I found something that works so I am not overly inclined to debug a different approach. ;)

if let Some(rustc) = option_env!("TEST_RUSTC") {
let libdir = option_env!("TEST_RUSTC_LIB").unwrap();
let sysroot = option_env!("TEST_SYSROOT").unwrap();
p.envs.push(("RUSTC_REAL".into(), Some(rustc.into())));
p.envs.push(("RUSTC_REAL_LIBDIR".into(), Some(libdir.into())));
p.envs.push(("RUSTC_SNAPSHOT".into(), Some(rustc.into())));
p.envs.push(("RUSTC_SNAPSHOT_LIBDIR".into(), Some(libdir.into())));
p.envs.push((
"RUSTC_SYSROOT".into(),
Some(sysroot.into()),
));
}
p
},
crate_manifest_path: Path::new("clippy_test_deps").join("Cargo.toml"),
build_std: None,
bless_lockfile: self.args.bless,
Expand Down Expand Up @@ -192,6 +216,9 @@ impl TestContext {
let dep = format!("-Ldependency={}", Path::new(host_libs).join("deps").display());
config.program.args.push(dep.into());
}
if let Some(sysroot) = option_env!("TEST_SYSROOT") {
config.program.args.push(format!("--sysroot={sysroot}").into());
}

config.program.program = profile_path.join(if cfg!(windows) {
"clippy-driver.exe"
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/tests/ui/auxiliary/proc_macro_derive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub fn derive(_: TokenStream) -> TokenStream {
let output = quote! {
// Should not trigger `useless_attribute`
#[allow(dead_code)]
extern crate rustc_middle;
extern crate core;
};
output
}
Expand Down
11 changes: 7 additions & 4 deletions src/tools/clippy/tests/ui/iter_over_hash_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,18 @@
#![warn(clippy::iter_over_hash_type)]
use std::collections::{HashMap, HashSet};

extern crate rustc_data_structures;

extern crate proc_macros;

// Ensure it also works via type aliases (this isn't really the Fx hasher but that does not matter).
type FxBuildHasher = std::collections::hash_map::RandomState;
type FxHashMap<K, V> = HashMap<K, V, FxBuildHasher>;
type FxHashSet<K> = HashSet<K, FxBuildHasher>;

fn main() {
let mut hash_set = HashSet::<i32>::new();
let mut hash_map = HashMap::<i32, i32>::new();
let mut fx_hash_map = rustc_data_structures::fx::FxHashMap::<i32, i32>::default();
let mut fx_hash_set = rustc_data_structures::fx::FxHashMap::<i32, i32>::default();
let mut fx_hash_map = FxHashMap::<i32, i32>::default();
let mut fx_hash_set = FxHashSet::<i32>::default();
let vec = Vec::<i32>::new();

// test hashset
Expand Down
26 changes: 13 additions & 13 deletions src/tools/clippy/tests/ui/iter_over_hash_type.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: iteration over unordered hash-based type
--> tests/ui/iter_over_hash_type.rs:18:5
--> tests/ui/iter_over_hash_type.rs:21:5
|
LL | / for x in &hash_set {
LL | |
Expand All @@ -11,7 +11,7 @@ LL | | }
= help: to override `-D warnings` add `#[allow(clippy::iter_over_hash_type)]`

error: iteration over unordered hash-based type
--> tests/ui/iter_over_hash_type.rs:22:5
--> tests/ui/iter_over_hash_type.rs:25:5
|
LL | / for x in hash_set.iter() {
LL | |
Expand All @@ -20,7 +20,7 @@ LL | | }
| |_____^

error: iteration over unordered hash-based type
--> tests/ui/iter_over_hash_type.rs:26:5
--> tests/ui/iter_over_hash_type.rs:29:5
|
LL | / for x in hash_set.clone() {
LL | |
Expand All @@ -29,7 +29,7 @@ LL | | }
| |_____^

error: iteration over unordered hash-based type
--> tests/ui/iter_over_hash_type.rs:30:5
--> tests/ui/iter_over_hash_type.rs:33:5
|
LL | / for x in hash_set.drain() {
LL | |
Expand All @@ -38,7 +38,7 @@ LL | | }
| |_____^

error: iteration over unordered hash-based type
--> tests/ui/iter_over_hash_type.rs:36:5
--> tests/ui/iter_over_hash_type.rs:39:5
|
LL | / for (x, y) in &hash_map {
LL | |
Expand All @@ -47,7 +47,7 @@ LL | | }
| |_____^

error: iteration over unordered hash-based type
--> tests/ui/iter_over_hash_type.rs:40:5
--> tests/ui/iter_over_hash_type.rs:43:5
|
LL | / for x in hash_map.keys() {
LL | |
Expand All @@ -56,7 +56,7 @@ LL | | }
| |_____^

error: iteration over unordered hash-based type
--> tests/ui/iter_over_hash_type.rs:44:5
--> tests/ui/iter_over_hash_type.rs:47:5
|
LL | / for x in hash_map.values() {
LL | |
Expand All @@ -65,7 +65,7 @@ LL | | }
| |_____^

error: iteration over unordered hash-based type
--> tests/ui/iter_over_hash_type.rs:48:5
--> tests/ui/iter_over_hash_type.rs:51:5
|
LL | / for x in hash_map.values_mut() {
LL | |
Expand All @@ -74,7 +74,7 @@ LL | | }
| |_____^

error: iteration over unordered hash-based type
--> tests/ui/iter_over_hash_type.rs:52:5
--> tests/ui/iter_over_hash_type.rs:55:5
|
LL | / for x in hash_map.iter() {
LL | |
Expand All @@ -83,7 +83,7 @@ LL | | }
| |_____^

error: iteration over unordered hash-based type
--> tests/ui/iter_over_hash_type.rs:56:5
--> tests/ui/iter_over_hash_type.rs:59:5
|
LL | / for x in hash_map.clone() {
LL | |
Expand All @@ -92,7 +92,7 @@ LL | | }
| |_____^

error: iteration over unordered hash-based type
--> tests/ui/iter_over_hash_type.rs:60:5
--> tests/ui/iter_over_hash_type.rs:63:5
|
LL | / for x in hash_map.drain() {
LL | |
Expand All @@ -101,7 +101,7 @@ LL | | }
| |_____^

error: iteration over unordered hash-based type
--> tests/ui/iter_over_hash_type.rs:66:5
--> tests/ui/iter_over_hash_type.rs:69:5
|
LL | / for x in fx_hash_set {
LL | |
Expand All @@ -110,7 +110,7 @@ LL | | }
| |_____^

error: iteration over unordered hash-based type
--> tests/ui/iter_over_hash_type.rs:70:5
--> tests/ui/iter_over_hash_type.rs:73:5
|
LL | / for x in fx_hash_map {
LL | |
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/tests/ui/useless_attribute.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
#[allow(unused_imports)]
#[allow(unused_extern_crates)]
#[macro_use]
extern crate rustc_middle;
extern crate regex as regex_crate;

#[macro_use]
extern crate proc_macro_derive;
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/tests/ui/useless_attribute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
#[allow(unused_imports)]
#[allow(unused_extern_crates)]
#[macro_use]
extern crate rustc_middle;
extern crate regex as regex_crate;

#[macro_use]
extern crate proc_macro_derive;
Expand Down
Loading