Skip to content

Commit

Permalink
Auto merge of rust-lang#82347 - the8472:parallelize-tidy, r=Mark-Simu…
Browse files Browse the repository at this point in the history
…lacrum

Parallelize tidy

Split off from rust-lang#81833

While that PR brings wall time of `x.py test tidy` down to 0m2.847s adding this one on top should bring it down to 0m1.673s.

r? `@Mark-Simulacrum`

Previous concerns can be found at rust-lang#81833 (comment) and rust-lang#81833 (comment)
  • Loading branch information
bors committed Apr 4, 2021
2 parents 88e7862 + 0513ba4 commit f98135b
Show file tree
Hide file tree
Showing 5 changed files with 189 additions and 104 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5308,6 +5308,7 @@ name = "tidy"
version = "0.1.0"
dependencies = [
"cargo_metadata 0.11.1",
"crossbeam-utils 0.8.0",
"lazy_static",
"regex",
"walkdir",
Expand Down
1 change: 1 addition & 0 deletions src/bootstrap/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,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");
}
Expand Down
1 change: 1 addition & 0 deletions src/tools/tidy/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ cargo_metadata = "0.11"
regex = "1"
lazy_static = "1"
walkdir = "2"
crossbeam-utils = "0.8.0"

[[bin]]
name = "rust-tidy"
Expand Down
169 changes: 102 additions & 67 deletions src/tools/tidy/src/bins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,91 +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<bool> {
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.
let mut temp_path = path.join("tidy-test-file");
match fs::File::create(&temp_path).or_else(|_| {
temp_path = output.join("tidy-test-file");
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;
}
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.

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);
}
};
}
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;
}

panic!("unable to create temporary file `{:?}`: {:?}", temp_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;
}
#[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());
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());
}
}
}
},
)
},
)
}
}
121 changes: 84 additions & 37 deletions src/tools/tidy/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,61 +6,108 @@

use tidy::*;

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() {
let root_path: PathBuf = env::args_os().nth(1).expect("need path to root of repo").into();
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");
let compiler_path = root_path.join("compiler");

let args: Vec<String> = 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);

// Checks that only make sense for the compiler.
errors::check(&compiler_path, &mut bad);
error_codes_check::check(&src_path, &mut bad);

// Checks that only make sense for the std libs.
pal::check(&library_path, &mut bad);

// 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);

bins::check(&src_path, &output_directory, &mut bad);
bins::check(&compiler_path, &output_directory, &mut bad);
bins::check(&library_path, &output_directory, &mut bad);

style::check(&src_path, &mut bad);
style::check(&compiler_path, &mut bad);
style::check(&library_path, &mut bad);

edition::check(&src_path, &mut bad);
edition::check(&compiler_path, &mut bad);
edition::check(&library_path, &mut bad);

let collected = features::check(&src_path, &compiler_path, &library_path, &mut bad, verbose);
unstable_book::check(&src_path, collected, &mut bad);

// Checks that are done on the cargo workspace.
deps::check(&root_path, &cargo, &mut bad);
extdeps::check(&root_path, &mut bad);

if bad {
let bad = std::sync::Arc::new(AtomicBool::new(false));

scope(|s| {
let mut handles: VecDeque<ScopedJoinHandle<'_, ()>> =
VecDeque::with_capacity(concurrency.get());

macro_rules! check {
($p:ident $(, $args:expr)* ) => {
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);
}
}

// Checks that are done on the cargo workspace.
check!(deps, &root_path, &cargo);
check!(extdeps, &root_path);

// Checks over tests.
check!(debug_artifacts, &src_path);
check!(ui_tests, &src_path);

// Checks that only make sense for the compiler.
check!(errors, &compiler_path);
check!(error_codes_check, &src_path);

// Checks that only make sense for the std libs.
check!(pal, &library_path);

// 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);

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);
check!(style, &library_path);

check!(edition, &src_path);
check!(edition, &compiler_path);
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 {
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);
}
Expand Down

0 comments on commit f98135b

Please sign in to comment.