From 81b7944163b48ce051248bc84f30b6ea65d7c1c0 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 22 Mar 2024 18:05:56 +0300 Subject: [PATCH 1/8] create new build step `clippy` Signed-off-by: onur-ozkan --- src/bootstrap/src/core/build_steps/check.rs | 111 +------ src/bootstrap/src/core/build_steps/clippy.rs | 307 +++++++++++++++++++ src/bootstrap/src/core/build_steps/mod.rs | 1 + src/bootstrap/src/core/builder.rs | 14 +- src/bootstrap/src/core/config/tests.rs | 2 +- 5 files changed, 335 insertions(+), 100 deletions(-) create mode 100644 src/bootstrap/src/core/build_steps/clippy.rs diff --git a/src/bootstrap/src/core/build_steps/check.rs b/src/bootstrap/src/core/build_steps/check.rs index 37d91b14ca1b6..654ef1ec6a7f8 100644 --- a/src/bootstrap/src/core/build_steps/check.rs +++ b/src/bootstrap/src/core/build_steps/check.rs @@ -11,6 +11,14 @@ use crate::core::config::TargetSelection; use crate::{Compiler, Mode, Subcommand}; use std::path::{Path, PathBuf}; +pub fn cargo_subcommand(kind: Kind) -> &'static str { + match kind { + Kind::Check | Kind::Clippy => "check", + Kind::Fix => "fix", + _ => unreachable!(), + } +} + #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct Std { pub target: TargetSelection, @@ -22,97 +30,6 @@ pub struct Std { crates: Vec, } -/// Returns args for the subcommand itself (not for cargo) -fn args(builder: &Builder<'_>) -> Vec { - fn strings<'a>(arr: &'a [&str]) -> impl Iterator + 'a { - arr.iter().copied().map(String::from) - } - - if let Subcommand::Clippy { fix, allow_dirty, allow_staged, allow, deny, warn, forbid } = - &builder.config.cmd - { - // disable the most spammy clippy lints - let ignored_lints = [ - "many_single_char_names", // there are a lot in stdarch - "collapsible_if", - "type_complexity", - "missing_safety_doc", // almost 3K warnings - "too_many_arguments", - "needless_lifetimes", // people want to keep the lifetimes - "wrong_self_convention", - ]; - let mut args = vec![]; - if *fix { - #[rustfmt::skip] - args.extend(strings(&[ - "--fix", "-Zunstable-options", - // FIXME: currently, `--fix` gives an error while checking tests for libtest, - // possibly because libtest is not yet built in the sysroot. - // As a workaround, avoid checking tests and benches when passed --fix. - "--lib", "--bins", "--examples", - ])); - - if *allow_dirty { - args.push("--allow-dirty".to_owned()); - } - - if *allow_staged { - args.push("--allow-staged".to_owned()); - } - } - - args.extend(strings(&["--"])); - - if deny.is_empty() && forbid.is_empty() { - args.extend(strings(&["--cap-lints", "warn"])); - } - - let all_args = std::env::args().collect::>(); - args.extend(get_clippy_rules_in_order(&all_args, allow, deny, warn, forbid)); - - args.extend(ignored_lints.iter().map(|lint| format!("-Aclippy::{}", lint))); - args.extend(builder.config.free_args.clone()); - args - } else { - builder.config.free_args.clone() - } -} - -/// We need to keep the order of the given clippy lint rules before passing them. -/// Since clap doesn't offer any useful interface for this purpose out of the box, -/// we have to handle it manually. -pub(crate) fn get_clippy_rules_in_order( - all_args: &[String], - allow_rules: &[String], - deny_rules: &[String], - warn_rules: &[String], - forbid_rules: &[String], -) -> Vec { - let mut result = vec![]; - - for (prefix, item) in - [("-A", allow_rules), ("-D", deny_rules), ("-W", warn_rules), ("-F", forbid_rules)] - { - item.iter().for_each(|v| { - let rule = format!("{prefix}{v}"); - let position = all_args.iter().position(|t| t == &rule).unwrap(); - result.push((position, rule)); - }); - } - - result.sort_by_key(|&(position, _)| position); - result.into_iter().map(|v| v.1).collect() -} - -fn cargo_subcommand(kind: Kind) -> &'static str { - match kind { - Kind::Check => "check", - Kind::Clippy => "clippy", - Kind::Fix => "fix", - _ => unreachable!(), - } -} - impl Std { pub fn new(target: TargetSelection) -> Self { Self { target, crates: vec![] } @@ -164,7 +81,7 @@ impl Step for Std { run_cargo( builder, cargo, - args(builder), + builder.config.free_args.clone(), &libstd_stamp(builder, compiler, target), vec![], true, @@ -221,7 +138,7 @@ impl Step for Std { run_cargo( builder, cargo, - args(builder), + builder.config.free_args.clone(), &libstd_test_stamp(builder, compiler, target), vec![], true, @@ -318,7 +235,7 @@ impl Step for Rustc { run_cargo( builder, cargo, - args(builder), + builder.config.free_args.clone(), &librustc_stamp(builder, compiler, target), vec![], true, @@ -384,7 +301,7 @@ impl Step for CodegenBackend { run_cargo( builder, cargo, - args(builder), + builder.config.free_args.clone(), &codegen_backend_stamp(builder, compiler, target, backend), vec![], true, @@ -450,7 +367,7 @@ impl Step for RustAnalyzer { run_cargo( builder, cargo, - args(builder), + builder.config.free_args.clone(), &stamp(builder, compiler, target), vec![], true, @@ -513,7 +430,7 @@ macro_rules! tool_check_step { run_cargo( builder, cargo, - args(builder), + builder.config.free_args.clone(), &stamp(builder, compiler, target), vec![], true, diff --git a/src/bootstrap/src/core/build_steps/clippy.rs b/src/bootstrap/src/core/build_steps/clippy.rs new file mode 100644 index 0000000000000..e2c2d3c9ef83d --- /dev/null +++ b/src/bootstrap/src/core/build_steps/clippy.rs @@ -0,0 +1,307 @@ +use std::path::Path; + +use crate::builder::Builder; +use crate::builder::ShouldRun; +use crate::core::builder; +use crate::core::builder::crate_description; +use crate::core::builder::Alias; +use crate::core::builder::RunConfig; +use crate::core::builder::Step; +use crate::Mode; +use crate::Subcommand; +use crate::TargetSelection; + +use super::check; +use super::compile; +use super::compile::librustc_stamp; +use super::compile::libstd_stamp; +use super::compile::run_cargo; +use super::compile::rustc_cargo; +use super::compile::std_cargo; +use super::tool::prepare_tool_cargo; +use super::tool::SourceType; + +// Disable the most spammy clippy lints +const IGNORED_RULES_FOR_STD_AND_RUSTC: &[&str] = &[ + "many_single_char_names", // there are a lot in stdarch + "collapsible_if", + "type_complexity", + "missing_safety_doc", // almost 3K warnings + "too_many_arguments", + "needless_lifetimes", // people want to keep the lifetimes + "wrong_self_convention", +]; + +fn lint_args(builder: &Builder<'_>, ignored_rules: &[&str]) -> Vec { + fn strings<'a>(arr: &'a [&str]) -> impl Iterator + 'a { + arr.iter().copied().map(String::from) + } + + let Subcommand::Clippy { fix, allow_dirty, allow_staged, allow, deny, warn, forbid } = + &builder.config.cmd + else { + unreachable!("clippy::lint_args can only be called from `clippy` subcommands."); + }; + + let mut args = vec![]; + if *fix { + #[rustfmt::skip] + args.extend(strings(&[ + "--fix", "-Zunstable-options", + // FIXME: currently, `--fix` gives an error while checking tests for libtest, + // possibly because libtest is not yet built in the sysroot. + // As a workaround, avoid checking tests and benches when passed --fix. + "--lib", "--bins", "--examples", + ])); + + if *allow_dirty { + args.push("--allow-dirty".to_owned()); + } + + if *allow_staged { + args.push("--allow-staged".to_owned()); + } + } + + args.extend(strings(&["--"])); + + if deny.is_empty() && forbid.is_empty() { + args.extend(strings(&["--cap-lints", "warn"])); + } + + let all_args = std::env::args().collect::>(); + args.extend(get_clippy_rules_in_order(&all_args, allow, deny, warn, forbid)); + + args.extend(ignored_rules.iter().map(|lint| format!("-Aclippy::{}", lint))); + args.extend(builder.config.free_args.clone()); + args +} + +/// We need to keep the order of the given clippy lint rules before passing them. +/// Since clap doesn't offer any useful interface for this purpose out of the box, +/// we have to handle it manually. +pub(crate) fn get_clippy_rules_in_order( + all_args: &[String], + allow_rules: &[String], + deny_rules: &[String], + warn_rules: &[String], + forbid_rules: &[String], +) -> Vec { + let mut result = vec![]; + + for (prefix, item) in + [("-A", allow_rules), ("-D", deny_rules), ("-W", warn_rules), ("-F", forbid_rules)] + { + item.iter().for_each(|v| { + let rule = format!("{prefix}{v}"); + let position = all_args.iter().position(|t| t == &rule).unwrap(); + result.push((position, rule)); + }); + } + + result.sort_by_key(|&(position, _)| position); + result.into_iter().map(|v| v.1).collect() +} + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct Std { + pub target: TargetSelection, + /// Whether to lint only a subset of crates. + crates: Vec, +} + +impl Step for Std { + type Output = (); + const DEFAULT: bool = true; + + fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { + run.crate_or_deps("sysroot").path("library") + } + + fn make_run(run: RunConfig<'_>) { + let crates = run.make_run_crates(Alias::Library); + run.builder.ensure(Std { target: run.target, crates }); + } + + fn run(self, builder: &Builder<'_>) { + builder.update_submodule(&Path::new("library").join("stdarch")); + + let target = self.target; + let compiler = builder.compiler(builder.top_stage, builder.config.build); + + let mut cargo = + builder::Cargo::new(builder, compiler, Mode::Std, SourceType::InTree, target, "clippy"); + + std_cargo(builder, target, compiler.stage, &mut cargo); + + if matches!(builder.config.cmd, Subcommand::Fix { .. }) { + // By default, cargo tries to fix all targets. Tell it not to fix tests until we've added `test` to the sysroot. + cargo.arg("--lib"); + } + + for krate in &*self.crates { + cargo.arg("-p").arg(krate); + } + + let _guard = builder.msg_check( + format_args!("library artifacts{}", crate_description(&self.crates)), + target, + ); + + run_cargo( + builder, + cargo, + lint_args(builder, IGNORED_RULES_FOR_STD_AND_RUSTC), + &libstd_stamp(builder, compiler, target), + vec![], + true, + false, + ); + } +} + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct Rustc { + pub target: TargetSelection, + /// Whether to lint only a subset of crates. + crates: Vec, +} + +impl Step for Rustc { + type Output = (); + const ONLY_HOSTS: bool = true; + const DEFAULT: bool = true; + + fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { + run.crate_or_deps("rustc-main").path("compiler") + } + + fn make_run(run: RunConfig<'_>) { + let crates = run.make_run_crates(Alias::Compiler); + run.builder.ensure(Rustc { target: run.target, crates }); + } + + /// Lints the compiler. + /// + /// This will lint the compiler for a particular stage of the build using + /// the `compiler` targeting the `target` architecture. + fn run(self, builder: &Builder<'_>) { + let compiler = builder.compiler(builder.top_stage, builder.config.build); + let target = self.target; + + if compiler.stage != 0 { + // If we're not in stage 0, then we won't have a std from the beta + // compiler around. That means we need to make sure there's one in + // the sysroot for the compiler to find. Otherwise, we're going to + // fail when building crates that need to generate code (e.g., build + // scripts and their dependencies). + builder.ensure(compile::Std::new(compiler, compiler.host)); + builder.ensure(compile::Std::new(compiler, target)); + } else { + builder.ensure(check::Std::new(target)); + } + + let mut cargo = builder::Cargo::new( + builder, + compiler, + Mode::Rustc, + SourceType::InTree, + target, + "clippy", + ); + + rustc_cargo(builder, &mut cargo, target, compiler.stage); + + // Explicitly pass -p for all compiler crates -- this will force cargo + // to also check the tests/benches/examples for these crates, rather + // than just the leaf crate. + for krate in &*self.crates { + cargo.arg("-p").arg(krate); + } + + let _guard = builder.msg_check( + format_args!("compiler artifacts{}", crate_description(&self.crates)), + target, + ); + + run_cargo( + builder, + cargo, + lint_args(builder, IGNORED_RULES_FOR_STD_AND_RUSTC), + &librustc_stamp(builder, compiler, target), + vec![], + true, + false, + ); + } +} + +macro_rules! lint_any { + ($( + $name:ident, $path:expr, $tool_name:expr + $(,is_external_tool = $external:expr)* + $(,is_unstable_tool = $unstable:expr)* + $(,allow_features = $allow_features:expr)? + ; + )+) => { + $( + + #[derive(Debug, Clone, Hash, PartialEq, Eq)] + pub struct $name { + pub target: TargetSelection, + } + + impl Step for $name { + type Output = (); + + fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { + run.path($path) + } + + fn make_run(run: RunConfig<'_>) { + run.builder.ensure($name { + target: run.target, + }); + } + + fn run(self, builder: &Builder<'_>) -> Self::Output { + let compiler = builder.compiler(builder.top_stage, builder.config.build); + let target = self.target; + + builder.ensure(check::Rustc::new(target, builder)); + + let cargo = prepare_tool_cargo( + builder, + compiler, + Mode::ToolRustc, + target, + "clippy", + $path, + SourceType::InTree, + &[], + ); + + run_cargo( + builder, + cargo, + lint_args(builder, &[]), + &libstd_stamp(builder, compiler, target), + vec![], + true, + false, + ); + } + } + )+ + } +} + +lint_any!( + Bootstrap, "src/bootstrap", "bootstrap"; + BuildHelper, "src/tools/build_helper", "build_helper"; + CoverageDump, "src/tools/coverage-dump", "coverage-dump"; + Tidy, "src/tools/tidy", "tidy"; + Compiletest, "src/tools/compiletest", "compiletest"; + RemoteTestServer, "src/tools/remote-test-server", "remote-test-server"; + RemoteTestClient, "src/tools/remote-test-client", "remote-test-client"; +); diff --git a/src/bootstrap/src/core/build_steps/mod.rs b/src/bootstrap/src/core/build_steps/mod.rs index 50d83789be82a..9b7378165de41 100644 --- a/src/bootstrap/src/core/build_steps/mod.rs +++ b/src/bootstrap/src/core/build_steps/mod.rs @@ -1,5 +1,6 @@ pub(crate) mod check; pub(crate) mod clean; +pub(crate) mod clippy; pub(crate) mod compile; pub(crate) mod dist; pub(crate) mod doc; diff --git a/src/bootstrap/src/core/builder.rs b/src/bootstrap/src/core/builder.rs index f31bc46d25fcb..6ee37acc90541 100644 --- a/src/bootstrap/src/core/builder.rs +++ b/src/bootstrap/src/core/builder.rs @@ -13,7 +13,7 @@ use std::process::Command; use std::sync::OnceLock; use std::time::{Duration, Instant}; -use crate::core::build_steps::llvm; +use crate::core::build_steps::{clippy, llvm}; use crate::core::build_steps::tool::{self, SourceType}; use crate::core::build_steps::{check, clean, compile, dist, doc, install, run, setup, test}; use crate::core::config::flags::{Color, Subcommand}; @@ -726,7 +726,17 @@ impl<'a> Builder<'a> { tool::CoverageDump, tool::LlvmBitcodeLinker ), - Kind::Check | Kind::Clippy | Kind::Fix => describe!( + Kind::Clippy => describe!( + clippy::Std, + clippy::Rustc, + clippy::Bootstrap, + clippy::BuildHelper, + clippy::CoverageDump, + clippy::Tidy, + clippy::RemoteTestServer, + clippy::RemoteTestClient, + ), + Kind::Check | Kind::Fix => describe!( check::Std, check::Rustc, check::Rustdoc, diff --git a/src/bootstrap/src/core/config/tests.rs b/src/bootstrap/src/core/config/tests.rs index 8cd538953c56d..59e16b6542728 100644 --- a/src/bootstrap/src/core/config/tests.rs +++ b/src/bootstrap/src/core/config/tests.rs @@ -1,5 +1,5 @@ use super::{flags::Flags, ChangeIdWrapper, Config}; -use crate::core::build_steps::check::get_clippy_rules_in_order; +use crate::core::build_steps::clippy::get_clippy_rules_in_order; use crate::core::config::{LldMode, TomlConfig}; use clap::CommandFactory; From 8a865a0fa9b1aacbcf84ad64ddb9f125f6007544 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 22 Mar 2024 18:15:18 +0300 Subject: [PATCH 2/8] create `Builder::msg_clippy` Signed-off-by: onur-ozkan --- src/bootstrap/src/core/build_steps/clippy.rs | 11 +++-------- src/bootstrap/src/core/builder.rs | 3 ++- src/bootstrap/src/lib.rs | 10 ++++++++++ 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/clippy.rs b/src/bootstrap/src/core/build_steps/clippy.rs index e2c2d3c9ef83d..190ea1752d84c 100644 --- a/src/bootstrap/src/core/build_steps/clippy.rs +++ b/src/bootstrap/src/core/build_steps/clippy.rs @@ -134,16 +134,11 @@ impl Step for Std { std_cargo(builder, target, compiler.stage, &mut cargo); - if matches!(builder.config.cmd, Subcommand::Fix { .. }) { - // By default, cargo tries to fix all targets. Tell it not to fix tests until we've added `test` to the sysroot. - cargo.arg("--lib"); - } - for krate in &*self.crates { cargo.arg("-p").arg(krate); } - let _guard = builder.msg_check( + let _guard = builder.msg_clippy( format_args!("library artifacts{}", crate_description(&self.crates)), target, ); @@ -213,13 +208,13 @@ impl Step for Rustc { rustc_cargo(builder, &mut cargo, target, compiler.stage); // Explicitly pass -p for all compiler crates -- this will force cargo - // to also check the tests/benches/examples for these crates, rather + // to also lint the tests/benches/examples for these crates, rather // than just the leaf crate. for krate in &*self.crates { cargo.arg("-p").arg(krate); } - let _guard = builder.msg_check( + let _guard = builder.msg_clippy( format_args!("compiler artifacts{}", crate_description(&self.crates)), target, ); diff --git a/src/bootstrap/src/core/builder.rs b/src/bootstrap/src/core/builder.rs index 6ee37acc90541..9d2352135c2f6 100644 --- a/src/bootstrap/src/core/builder.rs +++ b/src/bootstrap/src/core/builder.rs @@ -13,9 +13,9 @@ use std::process::Command; use std::sync::OnceLock; use std::time::{Duration, Instant}; -use crate::core::build_steps::{clippy, llvm}; use crate::core::build_steps::tool::{self, SourceType}; use crate::core::build_steps::{check, clean, compile, dist, doc, install, run, setup, test}; +use crate::core::build_steps::{clippy, llvm}; use crate::core::config::flags::{Color, Subcommand}; use crate::core::config::{DryRun, SplitDebuginfo, TargetSelection}; use crate::prepare_behaviour_dump_dir; @@ -672,6 +672,7 @@ impl Kind { Kind::Doc => "Documenting", Kind::Run => "Running", Kind::Suggest => "Suggesting", + Kind::Clippy => "Linting", _ => { let title_letter = self.as_str()[0..1].to_ascii_uppercase(); return format!("{title_letter}{}ing", &self.as_str()[1..]); diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index 1a8322c0dfd88..e1ae9fe2bf71a 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -1078,6 +1078,16 @@ impl Build { } } + #[must_use = "Groups should not be dropped until the Step finishes running"] + #[track_caller] + fn msg_clippy( + &self, + what: impl Display, + target: impl Into>, + ) -> Option { + self.msg(Kind::Clippy, self.config.stage, what, self.config.build, target) + } + #[must_use = "Groups should not be dropped until the Step finishes running"] #[track_caller] fn msg_check( From 77ba3f289109ae9823d86a9ad8dbf160b42ad76e Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 22 Mar 2024 18:23:01 +0300 Subject: [PATCH 3/8] support different `Kind`s in `Builder::msg_tool` Signed-off-by: onur-ozkan --- src/bootstrap/src/core/build_steps/clippy.rs | 12 +++++++++++- src/bootstrap/src/core/build_steps/tool.rs | 7 +++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/clippy.rs b/src/bootstrap/src/core/build_steps/clippy.rs index 190ea1752d84c..21ac6c505c309 100644 --- a/src/bootstrap/src/core/build_steps/clippy.rs +++ b/src/bootstrap/src/core/build_steps/clippy.rs @@ -5,6 +5,7 @@ use crate::builder::ShouldRun; use crate::core::builder; use crate::core::builder::crate_description; use crate::core::builder::Alias; +use crate::core::builder::Kind; use crate::core::builder::RunConfig; use crate::core::builder::Step; use crate::Mode; @@ -233,7 +234,7 @@ impl Step for Rustc { macro_rules! lint_any { ($( - $name:ident, $path:expr, $tool_name:expr + $name:ident, $path:expr, $readable_name:expr $(,is_external_tool = $external:expr)* $(,is_unstable_tool = $unstable:expr)* $(,allow_features = $allow_features:expr)? @@ -276,6 +277,15 @@ macro_rules! lint_any { &[], ); + let _guard = builder.msg_tool( + Kind::Clippy, + Mode::ToolRustc, + $readable_name, + compiler.stage, + &compiler.host, + &target, + ); + run_cargo( builder, cargo, diff --git a/src/bootstrap/src/core/build_steps/tool.rs b/src/bootstrap/src/core/build_steps/tool.rs index 8d1ff2fcb245f..45b1d5a05f35c 100644 --- a/src/bootstrap/src/core/build_steps/tool.rs +++ b/src/bootstrap/src/core/build_steps/tool.rs @@ -36,8 +36,9 @@ struct ToolBuild { impl Builder<'_> { #[track_caller] - fn msg_tool( + pub(crate) fn msg_tool( &self, + kind: Kind, mode: Mode, tool: &str, build_stage: u32, @@ -47,7 +48,7 @@ impl Builder<'_> { match mode { // depends on compiler stage, different to host compiler Mode::ToolRustc => self.msg_sysroot_tool( - Kind::Build, + kind, build_stage, format_args!("tool {tool}"), *host, @@ -100,6 +101,7 @@ impl Step for ToolBuild { cargo.allow_features(self.allow_features); } let _guard = builder.msg_tool( + Kind::Build, self.mode, self.tool, self.compiler.stage, @@ -481,6 +483,7 @@ impl Step for Rustdoc { ); let _guard = builder.msg_tool( + Kind::Build, Mode::ToolRustc, "rustdoc", build_compiler.stage, From 489e5d0ad0894cdfdf0e639d723a2cff88f7610a Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 22 Mar 2024 18:43:06 +0300 Subject: [PATCH 4/8] for clippy, skip output handling in `run_cargo` Signed-off-by: onur-ozkan --- src/bootstrap/src/core/build_steps/compile.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs index 9420e40d6c2b2..b2ee2c8f1f798 100644 --- a/src/bootstrap/src/core/build_steps/compile.rs +++ b/src/bootstrap/src/core/build_steps/compile.rs @@ -2012,7 +2012,7 @@ pub fn run_cargo( crate::exit!(1); } - if builder.config.dry_run() { + if builder.config.dry_run() || builder.kind == Kind::Clippy { return Vec::new(); } From bbacfe0cb64c4541097ed21f3cb0d3511bf0028f Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 22 Mar 2024 18:52:37 +0300 Subject: [PATCH 5/8] add simple top-level doc-comment for build_steps/clippy Signed-off-by: onur-ozkan --- src/bootstrap/src/core/build_steps/clippy.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/bootstrap/src/core/build_steps/clippy.rs b/src/bootstrap/src/core/build_steps/clippy.rs index 21ac6c505c309..7374c2b1cf290 100644 --- a/src/bootstrap/src/core/build_steps/clippy.rs +++ b/src/bootstrap/src/core/build_steps/clippy.rs @@ -1,3 +1,5 @@ +//! Implementation of running clippy on the compiler, standard library and various tools. + use std::path::Path; use crate::builder::Builder; From a01897345cead40cd3c349529a67e796a4395a09 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 22 Mar 2024 19:00:48 +0300 Subject: [PATCH 6/8] fix sysroot bug and update step message format Signed-off-by: onur-ozkan --- src/bootstrap/src/core/build_steps/clippy.rs | 25 +++++--------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/clippy.rs b/src/bootstrap/src/core/build_steps/clippy.rs index 7374c2b1cf290..15d676bea2050 100644 --- a/src/bootstrap/src/core/build_steps/clippy.rs +++ b/src/bootstrap/src/core/build_steps/clippy.rs @@ -141,10 +141,8 @@ impl Step for Std { cargo.arg("-p").arg(krate); } - let _guard = builder.msg_clippy( - format_args!("library artifacts{}", crate_description(&self.crates)), - target, - ); + let _guard = + builder.msg_clippy(format_args!("library{}", crate_description(&self.crates)), target); run_cargo( builder, @@ -187,17 +185,8 @@ impl Step for Rustc { let compiler = builder.compiler(builder.top_stage, builder.config.build); let target = self.target; - if compiler.stage != 0 { - // If we're not in stage 0, then we won't have a std from the beta - // compiler around. That means we need to make sure there's one in - // the sysroot for the compiler to find. Otherwise, we're going to - // fail when building crates that need to generate code (e.g., build - // scripts and their dependencies). - builder.ensure(compile::Std::new(compiler, compiler.host)); - builder.ensure(compile::Std::new(compiler, target)); - } else { - builder.ensure(check::Std::new(target)); - } + builder.ensure(compile::Std::new(compiler, compiler.host)); + builder.ensure(compile::Std::new(compiler, target)); let mut cargo = builder::Cargo::new( builder, @@ -217,10 +206,8 @@ impl Step for Rustc { cargo.arg("-p").arg(krate); } - let _guard = builder.msg_clippy( - format_args!("compiler artifacts{}", crate_description(&self.crates)), - target, - ); + let _guard = + builder.msg_clippy(format_args!("compiler{}", crate_description(&self.crates)), target); run_cargo( builder, From 5253fe4520dc14934f9cd99859da42a18686550f Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 22 Mar 2024 19:02:02 +0300 Subject: [PATCH 7/8] update `mingw-check` clippy invocation Previously this command was linting compiler and library together. As we no longer run clippy on the entire tree unless it's explicitly requested, we need to update this command by adding `library` path. Signed-off-by: onur-ozkan --- src/ci/docker/host-x86_64/mingw-check/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ci/docker/host-x86_64/mingw-check/Dockerfile b/src/ci/docker/host-x86_64/mingw-check/Dockerfile index 6918574814fff..ae8dfadec738a 100644 --- a/src/ci/docker/host-x86_64/mingw-check/Dockerfile +++ b/src/ci/docker/host-x86_64/mingw-check/Dockerfile @@ -46,7 +46,7 @@ ENV SCRIPT python3 ../x.py --stage 2 test src/tools/expand-yaml-anchors && \ # We also skip the x86_64-unknown-linux-gnu target as it is well-tested by other jobs. python3 ../x.py check --stage 0 --set build.optimized-compiler-builtins=false core alloc std --target=aarch64-unknown-linux-gnu,i686-pc-windows-msvc,i686-unknown-linux-gnu,x86_64-apple-darwin,x86_64-pc-windows-gnu,x86_64-pc-windows-msvc && \ python3 ../x.py check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu && \ - python3 ../x.py clippy compiler -Aclippy::all -Dclippy::correctness && \ + python3 ../x.py clippy compiler library -Aclippy::all -Dclippy::correctness && \ python3 ../x.py build --stage 0 src/tools/build-manifest && \ python3 ../x.py test --stage 0 src/tools/compiletest && \ python3 ../x.py test --stage 0 core alloc std test proc_macro && \ From 16cf0e660737e23a32e48c2459c219ff433a9518 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 22 Mar 2024 20:19:24 +0300 Subject: [PATCH 8/8] allow running clippy on most of the in-tree tools Signed-off-by: onur-ozkan --- src/bootstrap/src/core/build_steps/check.rs | 4 +- src/bootstrap/src/core/build_steps/clippy.rs | 49 +++++++++++++++---- src/bootstrap/src/core/build_steps/compile.rs | 2 +- src/bootstrap/src/core/builder.rs | 22 ++++++++- 4 files changed, 63 insertions(+), 14 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/check.rs b/src/bootstrap/src/core/build_steps/check.rs index 654ef1ec6a7f8..927d72e8ccb4b 100644 --- a/src/bootstrap/src/core/build_steps/check.rs +++ b/src/bootstrap/src/core/build_steps/check.rs @@ -13,7 +13,9 @@ use std::path::{Path, PathBuf}; pub fn cargo_subcommand(kind: Kind) -> &'static str { match kind { - Kind::Check | Kind::Clippy => "check", + Kind::Check + // We ensure check steps for both std and rustc from build_steps/clippy, so handle `Kind::Clippy` as well. + | Kind::Clippy => "check", Kind::Fix => "fix", _ => unreachable!(), } diff --git a/src/bootstrap/src/core/build_steps/clippy.rs b/src/bootstrap/src/core/build_steps/clippy.rs index 15d676bea2050..33323ec1e5de1 100644 --- a/src/bootstrap/src/core/build_steps/clippy.rs +++ b/src/bootstrap/src/core/build_steps/clippy.rs @@ -185,8 +185,17 @@ impl Step for Rustc { let compiler = builder.compiler(builder.top_stage, builder.config.build); let target = self.target; - builder.ensure(compile::Std::new(compiler, compiler.host)); - builder.ensure(compile::Std::new(compiler, target)); + if compiler.stage != 0 { + // If we're not in stage 0, then we won't have a std from the beta + // compiler around. That means we need to make sure there's one in + // the sysroot for the compiler to find. Otherwise, we're going to + // fail when building crates that need to generate code (e.g., build + // scripts and their dependencies). + builder.ensure(compile::Std::new(compiler, compiler.host)); + builder.ensure(compile::Std::new(compiler, target)); + } else { + builder.ensure(check::Std::new(target)); + } let mut cargo = builder::Cargo::new( builder, @@ -197,7 +206,7 @@ impl Step for Rustc { "clippy", ); - rustc_cargo(builder, &mut cargo, target, compiler.stage); + rustc_cargo(builder, &mut cargo, target, &compiler); // Explicitly pass -p for all compiler crates -- this will force cargo // to also lint the tests/benches/examples for these crates, rather @@ -224,9 +233,7 @@ impl Step for Rustc { macro_rules! lint_any { ($( $name:ident, $path:expr, $readable_name:expr - $(,is_external_tool = $external:expr)* - $(,is_unstable_tool = $unstable:expr)* - $(,allow_features = $allow_features:expr)? + $(,lint_by_default = $lint_by_default:expr)* ; )+) => { $( @@ -238,6 +245,7 @@ macro_rules! lint_any { impl Step for $name { type Output = (); + const DEFAULT: bool = if false $(|| $lint_by_default)* { true } else { false }; fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { run.path($path) @@ -275,11 +283,15 @@ macro_rules! lint_any { &target, ); + let stamp = builder + .cargo_out(compiler, Mode::ToolRustc, target) + .join(format!(".{}-check.stamp", stringify!($name).to_lowercase())); + run_cargo( builder, cargo, lint_args(builder, &[]), - &libstd_stamp(builder, compiler, target), + &stamp, vec![], true, false, @@ -293,9 +305,26 @@ macro_rules! lint_any { lint_any!( Bootstrap, "src/bootstrap", "bootstrap"; BuildHelper, "src/tools/build_helper", "build_helper"; - CoverageDump, "src/tools/coverage-dump", "coverage-dump"; - Tidy, "src/tools/tidy", "tidy"; + BuildManifest, "src/tools/build-manifest", "build-manifest"; + CargoMiri, "src/tools/miri/cargo-miri", "cargo-miri"; + Clippy, "src/tools/clippy", "clippy"; + CollectLicenseMetadata, "src/tools/collect-license-metadata", "collect-license-metadata"; Compiletest, "src/tools/compiletest", "compiletest"; - RemoteTestServer, "src/tools/remote-test-server", "remote-test-server"; + CoverageDump, "src/tools/coverage-dump", "coverage-dump"; + Jsondocck, "src/tools/jsondocck", "jsondocck"; + Jsondoclint, "src/tools/jsondoclint", "jsondoclint"; + LintDocs, "src/tools/lint-docs", "lint-docs"; + LlvmBitcodeLinker, "src/tools/llvm-bitcode-linker", "llvm-bitcode-linker"; + Miri, "src/tools/miri", "miri"; + MiroptTestTools, "src/tools/miropt-test-tools", "miropt-test-tools"; + OptDist, "src/tools/opt-dist", "opt-dist"; RemoteTestClient, "src/tools/remote-test-client", "remote-test-client"; + RemoteTestServer, "src/tools/remote-test-server", "remote-test-server"; + Rls, "src/tools/rls", "rls"; + RustAnalyzer, "src/tools/rust-analyzer", "rust-analyzer"; + RustDemangler, "src/tools/rust-demangler", "rust-demangler"; + Rustdoc, "src/tools/rustdoc", "clippy"; + Rustfmt, "src/tools/rustfmt", "rustfmt"; + RustInstaller, "src/tools/rust-installer", "rust-installer"; + Tidy, "src/tools/tidy", "tidy"; ); diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs index b2ee2c8f1f798..9420e40d6c2b2 100644 --- a/src/bootstrap/src/core/build_steps/compile.rs +++ b/src/bootstrap/src/core/build_steps/compile.rs @@ -2012,7 +2012,7 @@ pub fn run_cargo( crate::exit!(1); } - if builder.config.dry_run() || builder.kind == Kind::Clippy { + if builder.config.dry_run() { return Vec::new(); } diff --git a/src/bootstrap/src/core/builder.rs b/src/bootstrap/src/core/builder.rs index 9d2352135c2f6..499a74be6b151 100644 --- a/src/bootstrap/src/core/builder.rs +++ b/src/bootstrap/src/core/builder.rs @@ -732,10 +732,28 @@ impl<'a> Builder<'a> { clippy::Rustc, clippy::Bootstrap, clippy::BuildHelper, + clippy::BuildManifest, + clippy::CargoMiri, + clippy::Clippy, + clippy::CollectLicenseMetadata, + clippy::Compiletest, clippy::CoverageDump, - clippy::Tidy, - clippy::RemoteTestServer, + clippy::Jsondocck, + clippy::Jsondoclint, + clippy::LintDocs, + clippy::LlvmBitcodeLinker, + clippy::Miri, + clippy::MiroptTestTools, + clippy::OptDist, clippy::RemoteTestClient, + clippy::RemoteTestServer, + clippy::Rls, + clippy::RustAnalyzer, + clippy::RustDemangler, + clippy::Rustdoc, + clippy::Rustfmt, + clippy::RustInstaller, + clippy::Tidy, ), Kind::Check | Kind::Fix => describe!( check::Std,