Skip to content
Merged
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
3 changes: 3 additions & 0 deletions src/bootstrap/src/core/build_steps/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
{
Expand Down
4 changes: 2 additions & 2 deletions src/tools/tidy/src/alphabetical/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions src/tools/tidy/src/arg_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ pub struct TidyArgParser {
pub npm: PathBuf,
pub verbose: bool,
pub bless: bool,
pub ci: Option<bool>,
pub extra_checks: Option<Vec<String>>,
pub pos_args: Vec<String>,
}
Expand Down Expand Up @@ -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")
Expand All @@ -78,6 +80,7 @@ impl TidyArgParser {
npm: matches.get_one::<PathBuf>("npm").unwrap().clone(),
verbose: *matches.get_one::<bool>("verbose").unwrap(),
bless: *matches.get_one::<bool>("bless").unwrap(),
ci: matches.get_one::<bool>("ci").cloned(),
extra_checks: None,
pos_args: vec![],
};
Expand Down
46 changes: 46 additions & 0 deletions src/tools/tidy/src/arg_parser/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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![
Expand Down Expand Up @@ -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());
}
7 changes: 3 additions & 4 deletions src/tools/tidy/src/deps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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()
Expand Down
73 changes: 69 additions & 4 deletions src/tools/tidy/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -28,25 +31,47 @@ impl TidyFlags {
pub struct TidyCtx {
tidy_flags: TidyFlags,
diag_ctx: Arc<Mutex<DiagCtxInner>>,
ci_env: CiEnv,
pub base_commit: Option<String>,
}

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<bool>,
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(),
root_path: root_path.to_path_buf(),
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<Id: Into<CheckId>>(&self, id: Id) -> RunningCheck {
let mut id = id.into();

Expand Down Expand Up @@ -75,6 +100,46 @@ impl TidyCtx {
}
}

fn find_base_commit(tidy_ctx: &TidyCtx) -> Option<String> {
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<CheckId>,
finished_checks: HashSet<FinishedCheck>,
Expand Down Expand Up @@ -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")
}

Expand Down
8 changes: 4 additions & 4 deletions src/tools/tidy/src/error_codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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<String>, check: &mut RunningCheck) {
let Some(base_commit) = base_commit else {
check.verbose_msg("Skipping error code explanation removal check");
return;
};
Expand Down
2 changes: 1 addition & 1 deletion src/tools/tidy/src/extdeps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
31 changes: 18 additions & 13 deletions src/tools/tidy/src/extra_checks/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -67,7 +63,6 @@ pub fn check(
if let Err(e) = check_impl(
root_path,
outdir,
ci_info,
librustdoc_path,
tools_path,
npm,
Expand All @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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");
}
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -713,6 +717,7 @@ fn ensure_version_or_cargo_install(
pkg_name: &str,
bin_name: &str,
version: &str,
is_ci: bool,
) -> Result<PathBuf, Error> {
if let Ok(bin_path) = ensure_version(build_dir, bin_name, version) {
return Ok(bin_path);
Expand Down Expand Up @@ -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");
}

Expand Down
Loading
Loading