Skip to content
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

[wip] Fix x test clippy --stage 0 #96798

Closed
wants to merge 2 commits into from
Closed
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
28 changes: 18 additions & 10 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -659,24 +659,15 @@ 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",
]

[[package]]
Expand Down Expand Up @@ -5524,6 +5515,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.12.1",
"quote",
"regex",
"rustc-semver",
"serde",
"syn",
"tokio",
]

[[package]]
name = "ui_test"
version = "0.1.0"
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
44 changes: 36 additions & 8 deletions src/bootstrap/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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.
Expand All @@ -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"));
Expand Down
12 changes: 12 additions & 0 deletions src/bootstrap/tool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
11 changes: 1 addition & 10 deletions src/tools/clippy/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -40,17 +40,8 @@ filetime = "0.2"
rustc-workspace-hack = "1.0"

# UI test dependencies
ui-test-dependencies = { path = "ui-test-dependencies" }
clippy_utils = { path = "clippy_utils" }
Copy link
Member

Choose a reason for hiding this comment

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

Does Clippy still need all of those dependencies in the top level Cargo.toml or is it enough to have them all in ui-test-dependencies?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good callout :) I think it's enough to only have them in ui-test-dependencies? but there's a comment in compiletest that makes me wary:

// 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 derive_new;

Copy link
Member

Choose a reason for hiding this comment

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

I just tested it and with binary-dep-info they indeed need to stay in the top-level Cargo.toml

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" }
Expand Down
54 changes: 28 additions & 26 deletions src/tools/clippy/tests/compile-test.rs
Original file line number Diff line number Diff line change
@@ -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)]

Expand Down Expand Up @@ -38,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.
///
Expand All @@ -70,7 +48,23 @@ extern crate tokio;
/// dependencies that are not *directly* used by this test module require an
/// `extern crate` declaration.
static EXTERN_FLAGS: SyncLazy<String> = 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::<String>();
}

deps
} else {
let mut path = env::current_exe().unwrap();
path.set_extension("d");
fs::read_to_string(path).unwrap()
Expand Down Expand Up @@ -101,11 +95,15 @@ static EXTERN_FLAGS: SyncLazy<String> = 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 internal UI tests.
not_found.drain_filter(|x| *x == "clippy_utils");
}
assert!(
not_found.is_empty(),
"dependencies not found in depinfo: {:?}\n\
Expand Down Expand Up @@ -146,10 +144,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,
));

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 @@ -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
}
Expand Down
6 changes: 3 additions & 3 deletions src/tools/clippy/tests/ui/useless_attribute.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
6 changes: 3 additions & 3 deletions src/tools/clippy/tests/ui/useless_attribute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
20 changes: 20 additions & 0 deletions src/tools/clippy/ui-test-dependencies/Cargo.toml
Original file line number Diff line number Diff line change
@@ -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" }
Copy link
Member

Choose a reason for hiding this comment

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

This crate is required if Clippy internal lints should be tested in the Rust repo. Those are just UI tests as well and unrelated to dogfood.

Those internal UI tests are currently not run, because the internal feature has to be enabled during testing. I have a patch to do this, because this caused headaches during the sync multiple times in the past.

That being said, I think being able to test Clippy with --stage 0 is more important for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. I don't think we can run those lints without building the compiler twice, but we could add some way to opt-in to the internal tests so that they still run in CI (maybe something with --test-args?).

Copy link
Member

Choose a reason for hiding this comment

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

Worth looking into. But nothing to deal with in this PR.

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.12"
tokio = { version = "1", features = ["io-util"] }
rustc-semver = "1.1"
Empty file.