diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index f0fe1c03e7e14..f223ecf49507c 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -1320,6 +1320,9 @@ impl Step for Tidy { if builder.config.cmd.bless() { cmd.arg("--bless"); } + if builder.config.is_running_on_ci() { + cmd.arg("--ci=true"); + } if let Some(s) = builder.config.cmd.extra_checks().or(builder.config.tidy_extra_checks.as_deref()) { diff --git a/src/tools/tidy/src/alphabetical/tests.rs b/src/tools/tidy/src/alphabetical/tests.rs index 5fa0dd751b647..c1192986e63f2 100644 --- a/src/tools/tidy/src/alphabetical/tests.rs +++ b/src/tools/tidy/src/alphabetical/tests.rs @@ -5,7 +5,7 @@ use crate::diagnostics::{TidyCtx, TidyFlags}; #[track_caller] fn test(lines: &str, name: &str, expected_msg: &str, expected_bad: bool) { - let tidy_ctx = TidyCtx::new(Path::new("/"), false, TidyFlags::default()); + let tidy_ctx = TidyCtx::new(Path::new("/"), false, None, TidyFlags::default()); let mut check = tidy_ctx.start_check("alphabetical-test"); check_lines(Path::new(name), lines, &tidy_ctx, &mut check); @@ -37,7 +37,7 @@ fn bless_test(before: &str, after: &str) { let temp_path = tempfile::Builder::new().tempfile().unwrap().into_temp_path(); std::fs::write(&temp_path, before).unwrap(); - let tidy_ctx = TidyCtx::new(Path::new("/"), false, TidyFlags::new(true)); + let tidy_ctx = TidyCtx::new(Path::new("/"), false, None, TidyFlags::new(true)); let mut check = tidy_ctx.start_check("alphabetical-test"); check_lines(&temp_path, before, &tidy_ctx, &mut check); diff --git a/src/tools/tidy/src/arg_parser.rs b/src/tools/tidy/src/arg_parser.rs index 8041f739308d4..04502ac6266b6 100644 --- a/src/tools/tidy/src/arg_parser.rs +++ b/src/tools/tidy/src/arg_parser.rs @@ -15,6 +15,7 @@ pub struct TidyArgParser { pub npm: PathBuf, pub verbose: bool, pub bless: bool, + pub ci: Option, pub extra_checks: Option>, pub pos_args: Vec, } @@ -59,6 +60,7 @@ impl TidyArgParser { ) .arg(Arg::new("verbose").help("verbose").long("verbose").action(ArgAction::SetTrue)) .arg(Arg::new("bless").help("target files are modified").long("bless").action(ArgAction::SetTrue)) + .arg(Arg::new("ci").help("ci flag").long("ci").default_missing_value("true").num_args(0..=1).value_parser(value_parser!(bool))) .arg( Arg::new("extra_checks") .help("extra checks") @@ -78,6 +80,7 @@ impl TidyArgParser { npm: matches.get_one::("npm").unwrap().clone(), verbose: *matches.get_one::("verbose").unwrap(), bless: *matches.get_one::("bless").unwrap(), + ci: matches.get_one::("ci").cloned(), extra_checks: None, pos_args: vec![], }; diff --git a/src/tools/tidy/src/arg_parser/tests.rs b/src/tools/tidy/src/arg_parser/tests.rs index c5e7aed21c1a0..3aa6162f5c7d6 100644 --- a/src/tools/tidy/src/arg_parser/tests.rs +++ b/src/tools/tidy/src/arg_parser/tests.rs @@ -19,6 +19,7 @@ fn test_tidy_parser_full() { "yarn", "--verbose", "--bless", + "--ci", "--extra-checks", "if-installed:auto:js,auto:if-installed:py,if-installed:auto:cpp,if-installed:auto:spellcheck", "--", // pos_args @@ -38,6 +39,8 @@ fn test_tidy_parser_full() { assert_eq!(parsed_args.npm, PathBuf::from("yarn")); assert!(parsed_args.verbose); assert!(parsed_args.bless); + assert!(parsed_args.ci.is_some()); + assert!(parsed_args.ci.unwrap()); assert_eq!( parsed_args.extra_checks, Some(vec![ @@ -166,3 +169,46 @@ fn test_tidy_parser_missing_npm_path() { let cmd = TidyArgParser::command(); assert!(cmd.try_get_matches_from(args).is_err()); } + +// --ci has some variations +#[test] +fn test_tidy_parse_ci_flag() { + // They are requried + let base_args = vec![ + "rust-tidy", + "--root-path", + "/home/user/rust", + "--cargo-path", + "/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo", + "--output-dir", + "/home/user/rust/build", + "--concurrency", + "16", + "--npm-path", + "yarn", + ]; + + // No --ci + let parsed_args = TidyArgParser::build(TidyArgParser::command().get_matches_from(&base_args)); + assert!(parsed_args.ci.is_none()); + + // --ci + let mut args1 = base_args.clone(); + args1.push("--ci"); + let parsed_args = TidyArgParser::build(TidyArgParser::command().get_matches_from(args1)); + assert!(parsed_args.ci.is_some()); + + // --ci=true + let mut args2 = base_args.clone(); + args2.extend_from_slice(&["--ci", "true"]); + let parsed_args = TidyArgParser::build(TidyArgParser::command().get_matches_from(args2)); + assert!(parsed_args.ci.is_some()); + assert!(parsed_args.ci.unwrap()); + + // --ci=false + let mut args2 = base_args.clone(); + args2.extend_from_slice(&["--ci", "false"]); + let parsed_args = TidyArgParser::build(TidyArgParser::command().get_matches_from(args2)); + assert!(parsed_args.ci.is_some()); + assert!(!parsed_args.ci.unwrap()); +} diff --git a/src/tools/tidy/src/deps.rs b/src/tools/tidy/src/deps.rs index 24c610b41f3a4..2879df6670e81 100644 --- a/src/tools/tidy/src/deps.rs +++ b/src/tools/tidy/src/deps.rs @@ -6,7 +6,6 @@ use std::fs::{File, read_dir}; use std::io::Write; use std::path::Path; -use build_helper::ci::CiEnv; use cargo_metadata::semver::Version; use cargo_metadata::{Metadata, Package, PackageId}; @@ -637,7 +636,7 @@ pub fn check(root: &Path, cargo: &Path, tidy_ctx: TidyCtx) { check_proc_macro_dep_list(root, cargo, bless, &mut check); for &WorkspaceInfo { path, exceptions, crates_and_deps, submodules } in WORKSPACES { - if has_missing_submodule(root, submodules) { + if has_missing_submodule(root, submodules, tidy_ctx.is_running_on_ci()) { continue; } @@ -757,8 +756,8 @@ pub static CRATES: &[&str] = &[ /// Used to skip a check if a submodule is not checked out, and not in a CI environment. /// /// This helps prevent enforcing developers to fetch submodules for tidy. -pub fn has_missing_submodule(root: &Path, submodules: &[&str]) -> bool { - !CiEnv::is_ci() +pub fn has_missing_submodule(root: &Path, submodules: &[&str], is_ci: bool) -> bool { + !is_ci && submodules.iter().any(|submodule| { let path = root.join(submodule); !path.exists() diff --git a/src/tools/tidy/src/diagnostics.rs b/src/tools/tidy/src/diagnostics.rs index 4e6c316f5e18e..3f93316f63606 100644 --- a/src/tools/tidy/src/diagnostics.rs +++ b/src/tools/tidy/src/diagnostics.rs @@ -4,6 +4,9 @@ use std::io; use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; +use build_helper::ci::CiEnv; +use build_helper::git::{GitConfig, get_closest_upstream_commit}; +use build_helper::stage0_parser::{Stage0Config, parse_stage0_file}; use termcolor::Color; /// CLI flags used by tidy. @@ -28,11 +31,24 @@ impl TidyFlags { pub struct TidyCtx { tidy_flags: TidyFlags, diag_ctx: Arc>, + ci_env: CiEnv, + pub base_commit: Option, } impl TidyCtx { - pub fn new(root_path: &Path, verbose: bool, tidy_flags: TidyFlags) -> Self { - Self { + pub fn new( + root_path: &Path, + verbose: bool, + ci_flag: Option, + tidy_flags: TidyFlags, + ) -> Self { + let ci_env = match ci_flag { + Some(true) => CiEnv::GitHubActions, + Some(false) => CiEnv::None, + None => CiEnv::current(), + }; + + let mut tidy_ctx = Self { diag_ctx: Arc::new(Mutex::new(DiagCtxInner { running_checks: Default::default(), finished_checks: Default::default(), @@ -40,13 +56,22 @@ impl TidyCtx { verbose, })), tidy_flags, - } + ci_env, + base_commit: None, + }; + tidy_ctx.base_commit = find_base_commit(&tidy_ctx); + + tidy_ctx } pub fn is_bless_enabled(&self) -> bool { self.tidy_flags.bless } + pub fn is_running_on_ci(&self) -> bool { + self.ci_env.is_running_in_ci() + } + pub fn start_check>(&self, id: Id) -> RunningCheck { let mut id = id.into(); @@ -75,6 +100,46 @@ impl TidyCtx { } } +fn find_base_commit(tidy_ctx: &TidyCtx) -> Option { + let mut check = tidy_ctx.start_check("CI history"); + + let stage0 = parse_stage0_file(); + let Stage0Config { nightly_branch, git_merge_commit_email, .. } = stage0.config; + + let base_commit = match get_closest_upstream_commit( + None, + &GitConfig { + nightly_branch: &nightly_branch, + git_merge_commit_email: &git_merge_commit_email, + }, + tidy_ctx.ci_env, + ) { + Ok(Some(commit)) => Some(commit), + Ok(None) => { + error_if_in_ci("no base commit found", tidy_ctx.is_running_on_ci(), &mut check); + None + } + Err(error) => { + error_if_in_ci( + &format!("failed to retrieve base commit: {error}"), + tidy_ctx.is_running_on_ci(), + &mut check, + ); + None + } + }; + + base_commit +} + +fn error_if_in_ci(msg: &str, is_ci: bool, check: &mut RunningCheck) { + if is_ci { + check.error(msg); + } else { + check.warning(format!("{msg}. Some checks will be skipped.")); + } +} + struct DiagCtxInner { running_checks: HashSet, finished_checks: HashSet, @@ -175,7 +240,7 @@ impl RunningCheck { /// Useful if you want to run some functions from tidy without configuring /// diagnostics. pub fn new_noop() -> Self { - let ctx = TidyCtx::new(Path::new(""), false, TidyFlags::default()); + let ctx = TidyCtx::new(Path::new(""), false, None, TidyFlags::default()); ctx.start_check("noop") } diff --git a/src/tools/tidy/src/error_codes.rs b/src/tools/tidy/src/error_codes.rs index 185f3187a15c8..76fbf79951a9c 100644 --- a/src/tools/tidy/src/error_codes.rs +++ b/src/tools/tidy/src/error_codes.rs @@ -36,11 +36,11 @@ const IGNORE_DOCTEST_CHECK: &[&str] = &["E0464", "E0570", "E0601", "E0602", "E07 const IGNORE_UI_TEST_CHECK: &[&str] = &["E0461", "E0465", "E0514", "E0554", "E0640", "E0717", "E0729"]; -pub fn check(root_path: &Path, search_paths: &[&Path], ci_info: &crate::CiInfo, tidy_ctx: TidyCtx) { +pub fn check(root_path: &Path, search_paths: &[&Path], tidy_ctx: TidyCtx) { let mut check = tidy_ctx.start_check("error_codes"); // Check that no error code explanation was removed. - check_removed_error_code_explanation(ci_info, &mut check); + check_removed_error_code_explanation(&tidy_ctx.base_commit, &mut check); // Stage 1: create list let error_codes = extract_error_codes(root_path, &mut check); @@ -57,8 +57,8 @@ pub fn check(root_path: &Path, search_paths: &[&Path], ci_info: &crate::CiInfo, check_error_codes_used(search_paths, &error_codes, &mut check, &no_longer_emitted); } -fn check_removed_error_code_explanation(ci_info: &crate::CiInfo, check: &mut RunningCheck) { - let Some(base_commit) = &ci_info.base_commit else { +fn check_removed_error_code_explanation(base_commit: &Option, check: &mut RunningCheck) { + let Some(base_commit) = base_commit else { check.verbose_msg("Skipping error code explanation removal check"); return; }; diff --git a/src/tools/tidy/src/extdeps.rs b/src/tools/tidy/src/extdeps.rs index 19c773d12f7fa..7999386a3c298 100644 --- a/src/tools/tidy/src/extdeps.rs +++ b/src/tools/tidy/src/extdeps.rs @@ -19,7 +19,7 @@ pub fn check(root: &Path, tidy_ctx: TidyCtx) { let mut check = tidy_ctx.start_check("extdeps"); for &WorkspaceInfo { path, submodules, .. } in crate::deps::WORKSPACES { - if crate::deps::has_missing_submodule(root, submodules) { + if crate::deps::has_missing_submodule(root, submodules, tidy_ctx.is_running_on_ci()) { continue; } diff --git a/src/tools/tidy/src/extra_checks/mod.rs b/src/tools/tidy/src/extra_checks/mod.rs index 28e78b396d557..124de884637ea 100644 --- a/src/tools/tidy/src/extra_checks/mod.rs +++ b/src/tools/tidy/src/extra_checks/mod.rs @@ -23,9 +23,6 @@ use std::process::Command; use std::str::FromStr; use std::{env, fmt, fs, io}; -use build_helper::ci::CiEnv; - -use crate::CiInfo; use crate::diagnostics::TidyCtx; mod rustdoc_js; @@ -53,7 +50,6 @@ const SPELLCHECK_VER: &str = "1.38.1"; pub fn check( root_path: &Path, outdir: &Path, - ci_info: &CiInfo, librustdoc_path: &Path, tools_path: &Path, npm: &Path, @@ -67,7 +63,6 @@ pub fn check( if let Err(e) = check_impl( root_path, outdir, - ci_info, librustdoc_path, tools_path, npm, @@ -83,7 +78,6 @@ pub fn check( fn check_impl( root_path: &Path, outdir: &Path, - ci_info: &CiInfo, librustdoc_path: &Path, tools_path: &Path, npm: &Path, @@ -121,9 +115,12 @@ fn check_impl( }; lint_args.retain(|ck| ck.is_non_if_installed_or_matches(root_path, outdir)); if lint_args.iter().any(|ck| ck.auto) { - crate::files_modified_batch_filter(ci_info, &mut lint_args, |ck, path| { - ck.is_non_auto_or_matches(path) - }); + crate::files_modified_batch_filter( + &tidy_ctx.base_commit, + tidy_ctx.is_running_on_ci(), + &mut lint_args, + |ck, path| ck.is_non_auto_or_matches(path), + ); } macro_rules! extra_check { @@ -321,7 +318,7 @@ fn check_impl( } else { eprintln!("spellchecking files"); } - let res = spellcheck_runner(root_path, &outdir, &cargo, &args); + let res = spellcheck_runner(root_path, &outdir, &cargo, &args, tidy_ctx.is_running_on_ci()); if res.is_err() { rerun_with_bless("spellcheck", "fix typos"); } @@ -629,9 +626,16 @@ fn spellcheck_runner( outdir: &Path, cargo: &Path, args: &[&str], + is_ci: bool, ) -> Result<(), Error> { - let bin_path = - ensure_version_or_cargo_install(outdir, cargo, "typos-cli", "typos", SPELLCHECK_VER)?; + let bin_path = ensure_version_or_cargo_install( + outdir, + cargo, + "typos-cli", + "typos", + SPELLCHECK_VER, + is_ci, + )?; match Command::new(bin_path).current_dir(src_root).args(args).status() { Ok(status) => { if status.success() { @@ -713,6 +717,7 @@ fn ensure_version_or_cargo_install( pkg_name: &str, bin_name: &str, version: &str, + is_ci: bool, ) -> Result { if let Ok(bin_path) = ensure_version(build_dir, bin_name, version) { return Ok(bin_path); @@ -746,7 +751,7 @@ fn ensure_version_or_cargo_install( // On CI, we set opt-level flag for quicker installation. // Since lower opt-level decreases the tool's performance, // we don't set this option on local. - if CiEnv::is_ci() { + if is_ci { cmd.env("RUSTFLAGS", "-Copt-level=0"); } diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index ed41130f5d299..2cb8df782d28e 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -6,12 +6,6 @@ use std::ffi::OsStr; use std::process::Command; -use build_helper::ci::CiEnv; -use build_helper::git::{GitConfig, get_closest_upstream_commit}; -use build_helper::stage0_parser::{Stage0Config, parse_stage0_file}; - -use crate::diagnostics::{RunningCheck, TidyCtx}; - macro_rules! static_regex { ($re:literal) => {{ static RE: ::std::sync::LazyLock<::regex::Regex> = @@ -42,60 +36,6 @@ macro_rules! t { }; } -pub struct CiInfo { - pub git_merge_commit_email: String, - pub nightly_branch: String, - pub base_commit: Option, - pub ci_env: CiEnv, -} - -impl CiInfo { - pub fn new(tidy_ctx: TidyCtx) -> Self { - let mut check = tidy_ctx.start_check("CI history"); - - let stage0 = parse_stage0_file(); - let Stage0Config { nightly_branch, git_merge_commit_email, .. } = stage0.config; - - let mut info = Self { - nightly_branch, - git_merge_commit_email, - ci_env: CiEnv::current(), - base_commit: None, - }; - let base_commit = match get_closest_upstream_commit(None, &info.git_config(), info.ci_env) { - Ok(Some(commit)) => Some(commit), - Ok(None) => { - info.error_if_in_ci("no base commit found", &mut check); - None - } - Err(error) => { - info.error_if_in_ci( - &format!("failed to retrieve base commit: {error}"), - &mut check, - ); - None - } - }; - info.base_commit = base_commit; - info - } - - pub fn git_config(&self) -> GitConfig<'_> { - GitConfig { - nightly_branch: &self.nightly_branch, - git_merge_commit_email: &self.git_merge_commit_email, - } - } - - pub fn error_if_in_ci(&self, msg: &str, check: &mut RunningCheck) { - if self.ci_env.is_running_in_ci() { - check.error(msg); - } else { - check.warning(format!("{msg}. Some checks will be skipped.")); - } - } -} - pub fn git_diff>(base_commit: &str, extra_arg: S) -> Option { let output = Command::new("git").arg("diff").arg(base_commit).arg(extra_arg).output().ok()?; Some(String::from_utf8_lossy(&output.stdout).into()) @@ -107,15 +47,16 @@ pub fn git_diff>(base_commit: &str, extra_arg: S) -> Option( - ci_info: &CiInfo, + base_commit: &Option, + is_ci: bool, items: &mut Vec, pred: impl Fn(&T, &str) -> bool, ) { - if CiEnv::is_ci() { + if is_ci { // assume everything is modified on CI because we really don't want false positives there. return; } - let Some(base_commit) = &ci_info.base_commit else { + let Some(base_commit) = base_commit else { eprintln!("No base commit, assuming all files are modified"); return; }; @@ -150,9 +91,13 @@ pub fn files_modified_batch_filter( } /// Returns true if any modified file matches the predicate, if we are in CI, or if unable to list modified files. -pub fn files_modified(ci_info: &CiInfo, pred: impl Fn(&str) -> bool) -> bool { +pub fn files_modified( + base_commit: &Option, + is_ci: bool, + pred: impl Fn(&str) -> bool, +) -> bool { let mut v = vec![()]; - files_modified_batch_filter(ci_info, &mut v, |_, p| pred(p)); + files_modified_batch_filter(base_commit, is_ci, &mut v, |_, p| pred(p)); !v.is_empty() } diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index 09c08e1baf503..0e9885138d1e1 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -40,11 +40,11 @@ fn main() { let verbose = parsed_args.verbose; let bless = parsed_args.bless; + let ci = parsed_args.ci; let extra_checks = parsed_args.extra_checks; let pos_args = parsed_args.pos_args; - let tidy_ctx = TidyCtx::new(&root_path, verbose, TidyFlags::new(bless)); - let ci_info = CiInfo::new(tidy_ctx.clone()); + let tidy_ctx = TidyCtx::new(&root_path, verbose, ci, TidyFlags::new(bless)); let drain_handles = |handles: &mut VecDeque>| { // poll all threads for completion before awaiting the oldest one @@ -101,12 +101,12 @@ fn main() { check!(rustdoc_gui_tests, &tests_path); check!(rustdoc_css_themes, &librustdoc_path); check!(rustdoc_templates, &librustdoc_path); - check!(rustdoc_json, &src_path, &ci_info); + check!(rustdoc_json, &src_path); check!(known_bug, &crashes_path); check!(unknown_revision, &tests_path); // Checks that only make sense for the compiler. - check!(error_codes, &root_path, &[&compiler_path, &librustdoc_path], &ci_info); + check!(error_codes, &root_path, &[&compiler_path, &librustdoc_path]); check!(target_policy, &root_path); check!(gcc_submodule, &root_path, &compiler_path); @@ -154,7 +154,6 @@ fn main() { extra_checks, &root_path, &output_directory, - &ci_info, &librustdoc_path, &tools_path, &npm, diff --git a/src/tools/tidy/src/rustdoc_json.rs b/src/tools/tidy/src/rustdoc_json.rs index b8fb04f2d4e1f..ef7dabc8021ba 100644 --- a/src/tools/tidy/src/rustdoc_json.rs +++ b/src/tools/tidy/src/rustdoc_json.rs @@ -8,16 +8,18 @@ use crate::diagnostics::{CheckId, TidyCtx}; const RUSTDOC_JSON_TYPES: &str = "src/rustdoc-json-types"; -pub fn check(src_path: &Path, ci_info: &crate::CiInfo, tidy_ctx: TidyCtx) { +pub fn check(src_path: &Path, tidy_ctx: TidyCtx) { let mut check = tidy_ctx.start_check(CheckId::new("rustdoc_json").path(src_path)); - let Some(base_commit) = &ci_info.base_commit else { + let Some(base_commit) = &tidy_ctx.base_commit else { check.verbose_msg("No base commit, skipping rustdoc_json check"); return; }; // First we check that `src/rustdoc-json-types` was modified. - if !crate::files_modified(ci_info, |p| p.starts_with(RUSTDOC_JSON_TYPES)) { + if !crate::files_modified(&tidy_ctx.base_commit, tidy_ctx.is_running_on_ci(), |p| { + p.starts_with(RUSTDOC_JSON_TYPES) + }) { // `rustdoc-json-types` was not modified so nothing more to check here. return; }