From 77d1185f00699236c4caec6504d758644d747453 Mon Sep 17 00:00:00 2001 From: The8472 Date: Sat, 20 Feb 2021 22:45:30 +0100 Subject: [PATCH 1/3] parallelize tidy checks --- Cargo.lock | 1 + src/tools/tidy/Cargo.toml | 1 + src/tools/tidy/src/bins.rs | 7 +++- src/tools/tidy/src/lib.rs | 7 ++++ src/tools/tidy/src/main.rs | 81 +++++++++++++++++++++++++------------- 5 files changed, 67 insertions(+), 30 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6e95fd6af27..2f2d9065511 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5239,6 +5239,7 @@ name = "tidy" version = "0.1.0" dependencies = [ "cargo_metadata 0.11.1", + "crossbeam-utils 0.8.0", "lazy_static", "regex", "walkdir", diff --git a/src/tools/tidy/Cargo.toml b/src/tools/tidy/Cargo.toml index 777d7be8fdc..58c32993cb6 100644 --- a/src/tools/tidy/Cargo.toml +++ b/src/tools/tidy/Cargo.toml @@ -10,6 +10,7 @@ cargo_metadata = "0.11" regex = "1" lazy_static = "1" walkdir = "2" +crossbeam-utils = "0.8.0" [[bin]] name = "rust-tidy" diff --git a/src/tools/tidy/src/bins.rs b/src/tools/tidy/src/bins.rs index 62cfa85577f..f4e203cac40 100644 --- a/src/tools/tidy/src/bins.rs +++ b/src/tools/tidy/src/bins.rs @@ -32,9 +32,12 @@ pub fn check(path: &Path, output: &Path, bad: &mut bool) { // readily create a file there to test. // // See #36706 and #74753 for context. - let mut temp_path = path.join("tidy-test-file"); + // + // We also add the thread ID to avoid threads trampling on each others files. + let file_name = format!("t{}.tidy-test-file", std::thread::current().id().as_u64()); + let mut temp_path = path.join(&file_name); match fs::File::create(&temp_path).or_else(|_| { - temp_path = output.join("tidy-test-file"); + temp_path = output.join(&file_name); fs::File::create(&temp_path) }) { Ok(file) => { diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index 11d36751f67..45cc169470b 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -4,6 +4,7 @@ //! to be used by tools. #![cfg_attr(bootstrap, feature(str_split_once))] +#![feature(thread_id_value)] use std::fs::File; use std::io::Read; @@ -54,6 +55,12 @@ pub mod unit_tests; pub mod unstable_book; fn filter_dirs(path: &Path) -> bool { + // Filter out temporary files used by the bins module to probe the filesystem + match path.extension() { + Some(ext) if ext == "tidy-test-file" => return true, + _ => {} + } + let skip = [ "compiler/rustc_codegen_cranelift", "src/llvm-project", diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index 2ac96e404ac..9151d2bb406 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -6,9 +6,11 @@ use tidy::*; +use crossbeam_utils::thread::scope; use std::env; use std::path::PathBuf; use std::process; +use std::sync::atomic::{AtomicBool, Ordering}; fn main() { let root_path: PathBuf = env::args_os().nth(1).expect("need path to root of repo").into(); @@ -22,45 +24,68 @@ fn main() { let args: Vec = env::args().skip(1).collect(); - let mut bad = false; let verbose = args.iter().any(|s| *s == "--verbose"); - // Checks over tests. - debug_artifacts::check(&src_path, &mut bad); - ui_tests::check(&src_path, &mut bad); + let bad = std::sync::Arc::new(AtomicBool::new(false)); - // Checks that only make sense for the compiler. - errors::check(&compiler_path, &mut bad); - error_codes_check::check(&src_path, &mut bad); + scope(|s| { + macro_rules! check { + ($p:ident $(, $args:expr)* ) => { + s.spawn(|_| { + let mut flag = false; + $p::check($($args),* , &mut flag); + if (flag) { + bad.store(true, Ordering::Relaxed); + } + }); + } + } - // Checks that only make sense for the std libs. - pal::check(&library_path, &mut bad); + // Checks that are done on the cargo workspace. + check!(deps, &root_path, &cargo); + check!(extdeps, &root_path); - // Checks that need to be done for both the compiler and std libraries. - unit_tests::check(&src_path, &mut bad); - unit_tests::check(&compiler_path, &mut bad); - unit_tests::check(&library_path, &mut bad); + // Checks over tests. + check!(debug_artifacts, &src_path); + check!(ui_tests, &src_path); - bins::check(&src_path, &output_directory, &mut bad); - bins::check(&compiler_path, &output_directory, &mut bad); - bins::check(&library_path, &output_directory, &mut bad); + // Checks that only make sense for the compiler. + check!(errors, &compiler_path); + check!(error_codes_check, &src_path); - style::check(&src_path, &mut bad); - style::check(&compiler_path, &mut bad); - style::check(&library_path, &mut bad); + // Checks that only make sense for the std libs. + check!(pal, &library_path); - edition::check(&src_path, &mut bad); - edition::check(&compiler_path, &mut bad); - edition::check(&library_path, &mut bad); + // Checks that need to be done for both the compiler and std libraries. + check!(unit_tests, &src_path); + check!(unit_tests, &compiler_path); + check!(unit_tests, &library_path); - let collected = features::check(&src_path, &compiler_path, &library_path, &mut bad, verbose); - unstable_book::check(&src_path, collected, &mut bad); + check!(bins, &src_path, &output_directory); + check!(bins, &compiler_path, &output_directory); + check!(bins, &library_path, &output_directory); - // Checks that are done on the cargo workspace. - deps::check(&root_path, &cargo, &mut bad); - extdeps::check(&root_path, &mut bad); + check!(style, &src_path); + check!(style, &compiler_path); + check!(style, &library_path); - if bad { + check!(edition, &src_path); + check!(edition, &compiler_path); + check!(edition, &library_path); + + let collected = { + let mut flag = false; + let r = features::check(&src_path, &compiler_path, &library_path, &mut flag, verbose); + if flag { + bad.store(true, Ordering::Relaxed); + } + r + }; + check!(unstable_book, &src_path, collected); + }) + .unwrap(); + + if bad.load(Ordering::Relaxed) { eprintln!("some tidy checks failed"); process::exit(1); } From 207104010a5c60ac31d52a834f9d45094b927c48 Mon Sep 17 00:00:00 2001 From: The8472 Date: Sat, 13 Feb 2021 20:28:18 +0100 Subject: [PATCH 2/3] limit tidy parallelism by taking -j into account --- src/bootstrap/test.rs | 1 + src/tools/tidy/src/main.rs | 21 +++++++++++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index c0cd24dd81f..af4263d85c3 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -782,6 +782,7 @@ impl Step for Tidy { cmd.arg(&builder.src); cmd.arg(&builder.initial_cargo); cmd.arg(&builder.out); + cmd.arg(builder.jobs().to_string()); if builder.is_verbose() { cmd.arg("--verbose"); } diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index 9151d2bb406..2d19db38799 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -6,10 +6,13 @@ use tidy::*; -use crossbeam_utils::thread::scope; +use crossbeam_utils::thread::{scope, ScopedJoinHandle}; +use std::collections::VecDeque; use std::env; +use std::num::NonZeroUsize; use std::path::PathBuf; use std::process; +use std::str::FromStr; use std::sync::atomic::{AtomicBool, Ordering}; fn main() { @@ -17,6 +20,9 @@ fn main() { let cargo: PathBuf = env::args_os().nth(2).expect("need path to cargo").into(); let output_directory: PathBuf = env::args_os().nth(3).expect("need path to output directory").into(); + let concurrency: NonZeroUsize = + FromStr::from_str(&env::args().nth(4).expect("need concurrency")) + .expect("concurrency must be a number"); let src_path = root_path.join("src"); let library_path = root_path.join("library"); @@ -29,15 +35,23 @@ fn main() { let bad = std::sync::Arc::new(AtomicBool::new(false)); scope(|s| { + let mut handles: VecDeque> = + VecDeque::with_capacity(concurrency.get()); + macro_rules! check { ($p:ident $(, $args:expr)* ) => { - s.spawn(|_| { + while handles.len() >= concurrency.get() { + handles.pop_front().unwrap().join().unwrap(); + } + + let handle = s.spawn(|_| { let mut flag = false; $p::check($($args),* , &mut flag); if (flag) { bad.store(true, Ordering::Relaxed); } }); + handles.push_back(handle); } } @@ -74,6 +88,9 @@ fn main() { check!(edition, &library_path); let collected = { + while handles.len() >= concurrency.get() { + handles.pop_front().unwrap().join().unwrap(); + } let mut flag = false; let r = features::check(&src_path, &compiler_path, &library_path, &mut flag, verbose); if flag { From 0513ba4d65b953ab637fbafd979a9bd002b93e5c Mon Sep 17 00:00:00 2001 From: The8472 Date: Sun, 21 Feb 2021 12:08:04 +0100 Subject: [PATCH 3/3] perform filesystem probe once before running bins checks concurrently this avoids concurrent write attempts to the output directory --- src/tools/tidy/src/bins.rs | 176 ++++++++++++++++++++++--------------- src/tools/tidy/src/lib.rs | 7 -- src/tools/tidy/src/main.rs | 11 ++- 3 files changed, 112 insertions(+), 82 deletions(-) diff --git a/src/tools/tidy/src/bins.rs b/src/tools/tidy/src/bins.rs index f4e203cac40..1d5ec5c31c6 100644 --- a/src/tools/tidy/src/bins.rs +++ b/src/tools/tidy/src/bins.rs @@ -5,94 +5,126 @@ //! huge amount of bloat to the Git history, so it's good to just ensure we //! don't do that again. -use std::path::Path; +pub use os_impl::*; // All files are executable on Windows, so just check on Unix. #[cfg(windows)] -pub fn check(_path: &Path, _output: &Path, _bad: &mut bool) {} +mod os_impl { + use std::path::Path; + + pub fn check_filesystem_support(_sources: &[&Path], _output: &Path) -> bool { + return false; + } + + pub fn check(_path: &Path, _bad: &mut bool) {} +} #[cfg(unix)] -pub fn check(path: &Path, output: &Path, bad: &mut bool) { +mod os_impl { use std::fs; use std::os::unix::prelude::*; + use std::path::Path; use std::process::{Command, Stdio}; + enum FilesystemSupport { + Supported, + Unsupported, + ReadOnlyFs, + } + + use FilesystemSupport::*; + fn is_executable(path: &Path) -> std::io::Result { Ok(path.metadata()?.mode() & 0o111 != 0) } - // We want to avoid false positives on filesystems that do not support the - // executable bit. This occurs on some versions of Window's linux subsystem, - // for example. - // - // We try to create the temporary file first in the src directory, which is - // the preferred location as it's most likely to be on the same filesystem, - // and then in the output (`build`) directory if that fails. Sometimes we - // see the source directory mounted as read-only which means we can't - // readily create a file there to test. - // - // See #36706 and #74753 for context. - // - // We also add the thread ID to avoid threads trampling on each others files. - let file_name = format!("t{}.tidy-test-file", std::thread::current().id().as_u64()); - let mut temp_path = path.join(&file_name); - match fs::File::create(&temp_path).or_else(|_| { - temp_path = output.join(&file_name); - fs::File::create(&temp_path) - }) { - Ok(file) => { - let exec = is_executable(&temp_path).unwrap_or(false); - std::mem::drop(file); - std::fs::remove_file(&temp_path).expect("Deleted temp file"); - if exec { - // If the file is executable, then we assume that this - // filesystem does not track executability, so skip this check. - return; - } - } - Err(e) => { - // If the directory is read-only or we otherwise don't have rights, - // just don't run this check. - // - // 30 is the "Read-only filesystem" code at least in one CI - // environment. - if e.raw_os_error() == Some(30) { - eprintln!("tidy: Skipping binary file check, read-only filesystem"); - return; - } + pub fn check_filesystem_support(sources: &[&Path], output: &Path) -> bool { + // We want to avoid false positives on filesystems that do not support the + // executable bit. This occurs on some versions of Window's linux subsystem, + // for example. + // + // We try to create the temporary file first in the src directory, which is + // the preferred location as it's most likely to be on the same filesystem, + // and then in the output (`build`) directory if that fails. Sometimes we + // see the source directory mounted as read-only which means we can't + // readily create a file there to test. + // + // See #36706 and #74753 for context. - panic!("unable to create temporary file `{:?}`: {:?}", temp_path, e); + fn check_dir(dir: &Path) -> FilesystemSupport { + let path = dir.join("tidy-test-file"); + match fs::File::create(&path) { + Ok(file) => { + let exec = is_executable(&path).unwrap_or(false); + std::mem::drop(file); + std::fs::remove_file(&path).expect("Deleted temp file"); + // If the file is executable, then we assume that this + // filesystem does not track executability, so skip this check. + return if exec { Unsupported } else { Supported }; + } + Err(e) => { + // If the directory is read-only or we otherwise don't have rights, + // just don't run this check. + // + // 30 is the "Read-only filesystem" code at least in one CI + // environment. + if e.raw_os_error() == Some(30) { + eprintln!("tidy: Skipping binary file check, read-only filesystem"); + return ReadOnlyFs; + } + + panic!("unable to create temporary file `{:?}`: {:?}", path, e); + } + }; } + + for &source_dir in sources { + match check_dir(source_dir) { + Unsupported => return false, + ReadOnlyFs => { + return match check_dir(output) { + Supported => true, + _ => false, + }; + } + _ => {} + } + } + + return true; } - super::walk_no_read( - path, - &mut |path| super::filter_dirs(path) || path.ends_with("src/etc"), - &mut |entry| { - let file = entry.path(); - let filename = file.file_name().unwrap().to_string_lossy(); - let extensions = [".py", ".sh"]; - if extensions.iter().any(|e| filename.ends_with(e)) { - return; - } - - if t!(is_executable(&file), file) { - let rel_path = file.strip_prefix(path).unwrap(); - let git_friendly_path = rel_path.to_str().unwrap().replace("\\", "/"); - let output = Command::new("git") - .arg("ls-files") - .arg(&git_friendly_path) - .current_dir(path) - .stderr(Stdio::null()) - .output() - .unwrap_or_else(|e| { - panic!("could not run git ls-files: {}", e); - }); - let path_bytes = rel_path.as_os_str().as_bytes(); - if output.status.success() && output.stdout.starts_with(path_bytes) { - tidy_error!(bad, "binary checked into source: {}", file.display()); + #[cfg(unix)] + pub fn check(path: &Path, bad: &mut bool) { + crate::walk_no_read( + path, + &mut |path| crate::filter_dirs(path) || path.ends_with("src/etc"), + &mut |entry| { + let file = entry.path(); + let filename = file.file_name().unwrap().to_string_lossy(); + let extensions = [".py", ".sh"]; + if extensions.iter().any(|e| filename.ends_with(e)) { + return; } - } - }, - ) + + if t!(is_executable(&file), file) { + let rel_path = file.strip_prefix(path).unwrap(); + let git_friendly_path = rel_path.to_str().unwrap().replace("\\", "/"); + let output = Command::new("git") + .arg("ls-files") + .arg(&git_friendly_path) + .current_dir(path) + .stderr(Stdio::null()) + .output() + .unwrap_or_else(|e| { + panic!("could not run git ls-files: {}", e); + }); + let path_bytes = rel_path.as_os_str().as_bytes(); + if output.status.success() && output.stdout.starts_with(path_bytes) { + tidy_error!(bad, "binary checked into source: {}", file.display()); + } + } + }, + ) + } } diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index 45cc169470b..11d36751f67 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -4,7 +4,6 @@ //! to be used by tools. #![cfg_attr(bootstrap, feature(str_split_once))] -#![feature(thread_id_value)] use std::fs::File; use std::io::Read; @@ -55,12 +54,6 @@ pub mod unit_tests; pub mod unstable_book; fn filter_dirs(path: &Path) -> bool { - // Filter out temporary files used by the bins module to probe the filesystem - match path.extension() { - Some(ext) if ext == "tidy-test-file" => return true, - _ => {} - } - let skip = [ "compiler/rustc_codegen_cranelift", "src/llvm-project", diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index 2d19db38799..f190a9e57ce 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -75,9 +75,14 @@ fn main() { check!(unit_tests, &compiler_path); check!(unit_tests, &library_path); - check!(bins, &src_path, &output_directory); - check!(bins, &compiler_path, &output_directory); - check!(bins, &library_path, &output_directory); + if bins::check_filesystem_support( + &[&src_path, &compiler_path, &library_path], + &output_directory, + ) { + check!(bins, &src_path); + check!(bins, &compiler_path); + check!(bins, &library_path); + } check!(style, &src_path); check!(style, &compiler_path);