diff options
| -rw-r--r-- | src/tools/tidy/src/diagnostics.rs | 102 | ||||
| -rw-r--r-- | src/tools/tidy/src/lib.rs | 8 | ||||
| -rw-r--r-- | src/tools/tidy/src/main.rs | 129 | ||||
| -rw-r--r-- | src/tools/tidy/src/style.rs | 37 |
4 files changed, 187 insertions, 89 deletions
diff --git a/src/tools/tidy/src/diagnostics.rs b/src/tools/tidy/src/diagnostics.rs new file mode 100644 index 00000000000..a410938400e --- /dev/null +++ b/src/tools/tidy/src/diagnostics.rs @@ -0,0 +1,102 @@ +use std::collections::HashSet; +use std::fmt::Display; +use std::sync::{Arc, Mutex}; + +use crate::tidy_error; + +/// Collects diagnostics from all tidy steps, and contains shared information +/// that determines how should message and logs be presented. +/// +/// Since checks are executed in parallel, the context is internally synchronized, to avoid +/// all checks to lock it explicitly. +#[derive(Clone)] +pub struct DiagCtx(Arc<Mutex<DiagCtxInner>>); + +impl DiagCtx { + pub fn new(verbose: bool) -> Self { + Self(Arc::new(Mutex::new(DiagCtxInner { + running_checks: Default::default(), + finished_checks: Default::default(), + verbose, + }))) + } + + pub fn start_check<T: Display>(&self, name: T) -> RunningCheck { + let name = name.to_string(); + + let mut ctx = self.0.lock().unwrap(); + ctx.start_check(&name); + RunningCheck { name, bad: false, ctx: self.0.clone() } + } + + pub fn into_conclusion(self) -> bool { + let ctx = self.0.lock().unwrap(); + assert!(ctx.running_checks.is_empty(), "Some checks are still running"); + ctx.finished_checks.iter().any(|c| c.bad) + } +} + +struct DiagCtxInner { + running_checks: HashSet<String>, + finished_checks: HashSet<FinishedCheck>, + verbose: bool, +} + +impl DiagCtxInner { + fn start_check(&mut self, name: &str) { + if self.has_check(name) { + panic!("Starting a check named {name} for the second time"); + } + self.running_checks.insert(name.to_string()); + } + + fn finish_check(&mut self, check: FinishedCheck) { + assert!( + self.running_checks.remove(&check.name), + "Finishing check {} that was not started", + check.name + ); + self.finished_checks.insert(check); + } + + fn has_check(&self, name: &str) -> bool { + self.running_checks + .iter() + .chain(self.finished_checks.iter().map(|c| &c.name)) + .any(|c| c == name) + } +} + +#[derive(PartialEq, Eq, Hash, Debug)] +struct FinishedCheck { + name: String, + bad: bool, +} + +/// Represents a single tidy check, identified by its `name`, running. +pub struct RunningCheck { + name: String, + bad: bool, + ctx: Arc<Mutex<DiagCtxInner>>, +} + +impl RunningCheck { + /// Immediately output an error and mark the check as failed. + pub fn error<T: Display>(&mut self, t: T) { + self.mark_as_bad(); + tidy_error(&t.to_string()).expect("failed to output error"); + } + + fn mark_as_bad(&mut self) { + self.bad = true; + } +} + +impl Drop for RunningCheck { + fn drop(&mut self) { + self.ctx + .lock() + .unwrap() + .finish_check(FinishedCheck { name: self.name.clone(), bad: self.bad }) + } +} diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index f7920e3205a..953d6991990 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -50,13 +50,6 @@ macro_rules! tidy_error { }); } -macro_rules! tidy_error_ext { - ($tidy_error:path, $bad:expr, $($fmt:tt)*) => ({ - $tidy_error(&format_args!($($fmt)*).to_string()).expect("failed to output error"); - *$bad = true; - }); -} - fn tidy_error(args: &str) -> std::io::Result<()> { use std::io::Write; @@ -250,6 +243,7 @@ pub mod alphabetical; pub mod bins; pub mod debug_artifacts; pub mod deps; +pub mod diagnostics; pub mod edition; pub mod error_codes; pub mod extdeps; diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index f9e82341b7a..708daffe657 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -9,9 +9,11 @@ use std::num::NonZeroUsize; use std::path::PathBuf; use std::str::FromStr; use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, Mutex}; use std::thread::{self, ScopedJoinHandle, scope}; use std::{env, process}; +use tidy::diagnostics::DiagCtx; use tidy::*; fn main() { @@ -54,6 +56,8 @@ fn main() { let ci_info = CiInfo::new(&mut bad); let bad = std::sync::Arc::new(AtomicBool::new(bad)); + let mut diag_ctx = DiagCtx::new(verbose); + let drain_handles = |handles: &mut VecDeque<ScopedJoinHandle<'_, ()>>| { // poll all threads for completion before awaiting the oldest one for i in (0..handles.len()).rev() { @@ -87,76 +91,73 @@ fn main() { (@ $p:ident, name=$name:expr $(, $args:expr)* ) => { drain_handles(&mut handles); + let diag_ctx = diag_ctx.clone(); let handle = thread::Builder::new().name($name).spawn_scoped(s, || { - let mut flag = false; - $p::check($($args, )* &mut flag); - if (flag) { - bad.store(true, Ordering::Relaxed); - } + $p::check($($args, )* diag_ctx); }).unwrap(); handles.push_back(handle); } } - check!(target_specific_tests, &tests_path); + // check!(target_specific_tests, &tests_path); // Checks that are done on the cargo workspace. - check!(deps, &root_path, &cargo, bless); - check!(extdeps, &root_path); + // check!(deps, &root_path, &cargo, bless); + // check!(extdeps, &root_path); // Checks over tests. - check!(tests_placement, &root_path); - check!(tests_revision_unpaired_stdout_stderr, &tests_path); - check!(debug_artifacts, &tests_path); - check!(ui_tests, &root_path, bless); - check!(mir_opt_tests, &tests_path, bless); - 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!(known_bug, &crashes_path); - check!(unknown_revision, &tests_path); + // check!(tests_placement, &root_path); + // check!(tests_revision_unpaired_stdout_stderr, &tests_path); + // check!(debug_artifacts, &tests_path); + // check!(ui_tests, &root_path, bless); + // check!(mir_opt_tests, &tests_path, bless); + // 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!(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], verbose, &ci_info); - check!(fluent_alphabetical, &compiler_path, bless); - check!(fluent_period, &compiler_path); - check!(fluent_lowercase, &compiler_path); - check!(target_policy, &root_path); - check!(gcc_submodule, &root_path, &compiler_path); + // check!(error_codes, &root_path, &[&compiler_path, &librustdoc_path], verbose, &ci_info); + // check!(fluent_alphabetical, &compiler_path, bless); + // check!(fluent_period, &compiler_path); + // check!(fluent_lowercase, &compiler_path); + // check!(target_policy, &root_path); + // check!(gcc_submodule, &root_path, &compiler_path); // Checks that only make sense for the std libs. - check!(pal, &library_path); + // check!(pal, &library_path); // Checks that need to be done for both the compiler and std libraries. - check!(unit_tests, &src_path, false); - check!(unit_tests, &compiler_path, false); - check!(unit_tests, &library_path, true); - - if bins::check_filesystem_support(&[&root_path], &output_directory) { - check!(bins, &root_path); - } + // check!(unit_tests, &src_path, false); + // check!(unit_tests, &compiler_path, false); + // check!(unit_tests, &library_path, true); + // + // if bins::check_filesystem_support(&[&root_path], &output_directory) { + // check!(bins, &root_path); + // } check!(style, &src_path); check!(style, &tests_path); check!(style, &compiler_path); check!(style, &library_path); - check!(edition, &src_path); - check!(edition, &compiler_path); - check!(edition, &library_path); - - check!(alphabetical, &root_manifest); - check!(alphabetical, &src_path); - check!(alphabetical, &tests_path); - check!(alphabetical, &compiler_path); - check!(alphabetical, &library_path); - - check!(x_version, &root_path, &cargo); - - check!(triagebot, &root_path); - - check!(filenames, &root_path); + // check!(edition, &src_path); + // check!(edition, &compiler_path); + // check!(edition, &library_path); + // + // check!(alphabetical, &root_manifest); + // check!(alphabetical, &src_path); + // check!(alphabetical, &tests_path); + // check!(alphabetical, &compiler_path); + // check!(alphabetical, &library_path); + // + // check!(x_version, &root_path, &cargo); + // + // check!(triagebot, &root_path); + // + // check!(filenames, &root_path); let collected = { drain_handles(&mut handles); @@ -175,24 +176,24 @@ fn main() { } r }; - check!(unstable_book, &src_path, collected); - - check!( - extra_checks, - &root_path, - &output_directory, - &ci_info, - &librustdoc_path, - &tools_path, - &npm, - &cargo, - bless, - extra_checks, - pos_args - ); + // check!(unstable_book, &src_path, collected); + // + // check!( + // extra_checks, + // &root_path, + // &output_directory, + // &ci_info, + // &librustdoc_path, + // &tools_path, + // &npm, + // &cargo, + // bless, + // extra_checks, + // pos_args + // ); }); - if bad.load(Ordering::Relaxed) { + if diag_ctx.into_conclusion() { eprintln!("some tidy checks failed"); process::exit(1); } diff --git a/src/tools/tidy/src/style.rs b/src/tools/tidy/src/style.rs index fca097c091b..7d6ebed397f 100644 --- a/src/tools/tidy/src/style.rs +++ b/src/tools/tidy/src/style.rs @@ -24,6 +24,7 @@ use std::sync::LazyLock; use regex::RegexSetBuilder; use rustc_hash::FxHashMap; +use crate::diagnostics::DiagCtx; use crate::walk::{filter_dirs, walk}; #[cfg(test)] @@ -338,7 +339,9 @@ fn is_unexplained_ignore(extension: &str, line: &str) -> bool { true } -pub fn check(path: &Path, bad: &mut bool) { +pub fn check(path: &Path, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check(format!("style {}", path.display())); + fn skip(path: &Path, is_dir: bool) -> bool { if path.file_name().is_some_and(|name| name.to_string_lossy().starts_with(".#")) { // vim or emacs temporary file @@ -391,7 +394,7 @@ pub fn check(path: &Path, bad: &mut bool) { }); if contents.is_empty() { - tidy_error!(bad, "{}: empty file", file.display()); + check.error(format!("{}: empty file", file.display())); } let extension = file.extension().unwrap().to_string_lossy(); @@ -467,7 +470,7 @@ pub fn check(path: &Path, bad: &mut bool) { } let mut err = |msg: &str| { - tidy_error!(bad, "{}:{}: {}", file.display(), i + 1, msg); + check.error(format!("{}:{}: {msg}", file.display(), i + 1)); }; if trimmed.contains("dbg!") @@ -611,7 +614,7 @@ pub fn check(path: &Path, bad: &mut bool) { && backtick_count % 2 == 1 { let mut err = |msg: &str| { - tidy_error!(bad, "{}:{start_line}: {msg}", file.display()); + check.error(format!("{}:{start_line}: {msg}", file.display())); }; let block_len = (i + 1) - start_line; if block_len == 1 { @@ -632,12 +635,12 @@ pub fn check(path: &Path, bad: &mut bool) { } if leading_new_lines { let mut err = |_| { - tidy_error!(bad, "{}: leading newline", file.display()); + check.error(format!("{}: leading newline", file.display())); }; suppressible_tidy_err!(err, skip_leading_newlines, "missing leading newline"); } let mut err = |msg: &str| { - tidy_error!(bad, "{}: {}", file.display(), msg); + check.error(format!("{}: {}", file.display(), msg)); }; match trailing_new_lines { 0 => suppressible_tidy_err!(err, skip_trailing_newlines, "missing trailing newline"), @@ -650,38 +653,36 @@ pub fn check(path: &Path, bad: &mut bool) { }; if lines > LINES { let mut err = |_| { - tidy_error!( - bad, - "{}: too many lines ({}) (add `// \ + check.error(format!( + "{}: too many lines ({lines}) (add `// \ ignore-tidy-filelength` to the file to suppress this error)", file.display(), - lines - ); + )); }; suppressible_tidy_err!(err, skip_file_length, ""); } if let Directive::Ignore(false) = skip_cr { - tidy_error!(bad, "{}: ignoring CR characters unnecessarily", file.display()); + check.error(format!("{}: ignoring CR characters unnecessarily", file.display())); } if let Directive::Ignore(false) = skip_tab { - tidy_error!(bad, "{}: ignoring tab characters unnecessarily", file.display()); + check.error(format!("{}: ignoring tab characters unnecessarily", file.display())); } if let Directive::Ignore(false) = skip_end_whitespace { - tidy_error!(bad, "{}: ignoring trailing whitespace unnecessarily", file.display()); + check.error(format!("{}: ignoring trailing whitespace unnecessarily", file.display())); } if let Directive::Ignore(false) = skip_trailing_newlines { - tidy_error!(bad, "{}: ignoring trailing newlines unnecessarily", file.display()); + check.error(format!("{}: ignoring trailing newlines unnecessarily", file.display())); } if let Directive::Ignore(false) = skip_leading_newlines { - tidy_error!(bad, "{}: ignoring leading newlines unnecessarily", file.display()); + check.error(format!("{}: ignoring leading newlines unnecessarily", file.display())); } if let Directive::Ignore(false) = skip_copyright { - tidy_error!(bad, "{}: ignoring copyright unnecessarily", file.display()); + check.error(format!("{}: ignoring copyright unnecessarily", file.display())); } // We deliberately do not warn about these being unnecessary, // that would just lead to annoying churn. let _unused = skip_line_length; let _unused = skip_file_length; - }) + }); } |