Fix formatting command

The formatting command previously had two issues:
- if rustfmt failed, it would print the command invocation. this is
  unnecessarily noisy
- there was a race condition that lead to orphan rustfmts that would
  print their output after bootstrap exited

We fix this by
- removing the printing, it's not really useful
- threading failure through properly instead of just yoloing exit(1)
This commit is contained in:
Noratrieb 2025-01-02 17:26:46 +01:00
parent bf6f8a4d32
commit 8b73fc5e27

View File

@ -14,7 +14,19 @@ use crate::core::builder::Builder;
use crate::utils::exec::command;
use crate::utils::helpers::{self, program_out_of_date, t};
fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl FnMut(bool) -> bool {
#[must_use]
enum RustfmtStatus {
InProgress,
Ok,
Failed,
}
fn rustfmt(
src: &Path,
rustfmt: &Path,
paths: &[PathBuf],
check: bool,
) -> impl FnMut(bool) -> RustfmtStatus {
let mut cmd = Command::new(rustfmt);
// Avoid the submodule config paths from coming into play. We only allow a single global config
// for the workspace for now.
@ -26,30 +38,20 @@ fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl F
cmd.arg("--check");
}
cmd.args(paths);
let cmd_debug = format!("{cmd:?}");
let mut cmd = cmd.spawn().expect("running rustfmt");
// Poor man's async: return a closure that might wait for rustfmt's completion (depending on
// the value of the `block` argument).
move |block: bool| -> bool {
move |block: bool| -> RustfmtStatus {
let status = if !block {
match cmd.try_wait() {
Ok(Some(status)) => Ok(status),
Ok(None) => return false,
Ok(None) => return RustfmtStatus::InProgress,
Err(err) => Err(err),
}
} else {
cmd.wait()
};
if !status.unwrap().success() {
eprintln!(
"fmt error: Running `{}` failed.\nIf you're running `tidy`, \
try again with `--bless`. Or, if you just want to format \
code, run `./x.py fmt` instead.",
cmd_debug,
);
crate::exit!(1);
}
true
if status.unwrap().success() { RustfmtStatus::Ok } else { RustfmtStatus::Failed }
}
}
@ -240,6 +242,8 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
// Spawn child processes on a separate thread so we can batch entries we have received from
// ignore.
let thread = std::thread::spawn(move || {
let mut result = Ok(());
let mut children = VecDeque::new();
while let Ok(path) = rx.recv() {
// Try getting more paths from the channel to amortize the overhead of spawning
@ -251,22 +255,38 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
// Poll completion before waiting.
for i in (0..children.len()).rev() {
if children[i](false) {
children.swap_remove_back(i);
break;
match children[i](false) {
RustfmtStatus::InProgress => {}
RustfmtStatus::Failed => {
result = Err(());
children.swap_remove_back(i);
break;
}
RustfmtStatus::Ok => {
children.swap_remove_back(i);
break;
}
}
}
if children.len() >= max_processes {
// Await oldest child.
children.pop_front().unwrap()(true);
match children.pop_front().unwrap()(true) {
RustfmtStatus::InProgress | RustfmtStatus::Ok => {}
RustfmtStatus::Failed => result = Err(()),
}
}
}
// Await remaining children.
for mut child in children {
child(true);
match child(true) {
RustfmtStatus::InProgress | RustfmtStatus::Ok => {}
RustfmtStatus::Failed => result = Err(()),
}
}
result
});
let formatted_paths = Mutex::new(Vec::new());
@ -299,7 +319,12 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
drop(tx);
thread.join().unwrap();
let result = thread.join().unwrap();
if result.is_err() {
crate::exit!(1);
}
if !check {
update_rustfmt_version(build);
}