Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bootstrap command refactoring: refactor BootstrapCommand (step 1) #126731

Merged
merged 8 commits into from
Jun 22, 2024
Merged

Bootstrap command refactoring: refactor BootstrapCommand (step 1) #126731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a12f541
Implement new command execution logic
Kobzol Jun 20, 2024
949e667
Remove `run_quiet`
Kobzol Jun 20, 2024
e933cfb
Remove `run_quiet_delaying_failure`
Kobzol Jun 20, 2024
0de7b92
Remove `run_delaying_failure`
Kobzol Jun 20, 2024
5c4318d
Implement `run_cmd` in terms of `run_tracked`
Kobzol Jun 20, 2024
c152934
Remove unused import
Kobzol Jun 20, 2024
3fe4d13
Appease `clippy`
Kobzol Jun 20, 2024
250586c
Wrap std `Output` in `CommandOutput`
Kobzol Jun 22, 2024
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
43 changes: 26 additions & 17 deletions src/bootstrap/src/core/build_steps/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use crate::core::builder::{Builder, Compiler, Kind, RunConfig, ShouldRun, Step};
use crate::core::config::flags::get_completion;
use crate::core::config::flags::Subcommand;
use crate::core::config::TargetSelection;
use crate::utils::exec::BootstrapCommand;
use crate::utils::exec::{BootstrapCommand, OutputMode};
use crate::utils::helpers::{
self, add_link_lib_path, add_rustdoc_cargo_linker_args, dylib_path, dylib_path_var,
linker_args, linker_flags, output, t, target_supports_cranelift_backend, up_to_date,
Expand Down Expand Up @@ -156,7 +156,10 @@ You can skip linkcheck with --skip src/tools/linkchecker"
let _guard =
builder.msg(Kind::Test, compiler.stage, "Linkcheck", bootstrap_host, bootstrap_host);
let _time = helpers::timeit(builder);
builder.run_delaying_failure(linkchecker.arg(builder.out.join(host.triple).join("doc")));
builder.run_tracked(
BootstrapCommand::from(linkchecker.arg(builder.out.join(host.triple).join("doc")))
.delay_failure(),
);
}

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
Expand Down Expand Up @@ -213,8 +216,11 @@ impl Step for HtmlCheck {
builder,
));

builder.run_delaying_failure(
builder.tool_cmd(Tool::HtmlChecker).arg(builder.doc_out(self.target)),
builder.run_tracked(
BootstrapCommand::from(
builder.tool_cmd(Tool::HtmlChecker).arg(builder.doc_out(self.target)),
)
.delay_failure(),
);
}
}
Expand Down Expand Up @@ -261,7 +267,7 @@ impl Step for Cargotest {
.env("RUSTC", builder.rustc(compiler))
.env("RUSTDOC", builder.rustdoc(compiler));
add_rustdoc_cargo_linker_args(cmd, builder, compiler.host, LldThreads::No);
builder.run_delaying_failure(cmd);
builder.run_tracked(BootstrapCommand::from(cmd).delay_failure());
}
}

Expand Down Expand Up @@ -813,7 +819,7 @@ impl Step for RustdocTheme {
.env("RUSTC_BOOTSTRAP", "1");
cmd.args(linker_args(builder, self.compiler.host, LldThreads::No));

builder.run_delaying_failure(&mut cmd);
builder.run_tracked(BootstrapCommand::from(&mut cmd).delay_failure());
}
}

Expand Down Expand Up @@ -1093,7 +1099,7 @@ HELP: to skip test's attempt to check tidiness, pass `--skip src/tools/tidy` to
}

builder.info("tidy check");
builder.run_delaying_failure(&mut cmd);
builder.run_tracked(BootstrapCommand::from(&mut cmd).delay_failure());

builder.info("x.py completions check");
let [bash, zsh, fish, powershell] = ["x.py.sh", "x.py.zsh", "x.py.fish", "x.py.ps1"]
Expand Down Expand Up @@ -2179,7 +2185,8 @@ impl BookTest {
compiler.host,
);
let _time = helpers::timeit(builder);
let toolstate = if builder.run_delaying_failure(&mut rustbook_cmd) {
let cmd = BootstrapCommand::from(&mut rustbook_cmd).delay_failure();
let toolstate = if builder.run_tracked(cmd).is_success() {
ToolState::TestPass
} else {
ToolState::TestFail
Expand Down Expand Up @@ -2312,7 +2319,8 @@ impl Step for ErrorIndex {
let guard =
builder.msg(Kind::Test, compiler.stage, "error-index", compiler.host, compiler.host);
let _time = helpers::timeit(builder);
builder.run_quiet(&mut tool);
builder
.run_tracked(BootstrapCommand::from(&mut tool).output_mode(OutputMode::OnlyOnFailure));
drop(guard);
// The tests themselves need to link to std, so make sure it is
// available.
Expand Down Expand Up @@ -2341,11 +2349,11 @@ fn markdown_test(builder: &Builder<'_>, compiler: Compiler, markdown: &Path) ->
let test_args = builder.config.test_args().join(" ");
cmd.arg("--test-args").arg(test_args);

if builder.config.verbose_tests {
builder.run_delaying_failure(&mut cmd)
} else {
builder.run_quiet_delaying_failure(&mut cmd)
let mut cmd = BootstrapCommand::from(&mut cmd).delay_failure();
if !builder.config.verbose_tests {
cmd = cmd.quiet();
}
builder.run_tracked(cmd).is_success()
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
Expand All @@ -2370,7 +2378,8 @@ impl Step for RustcGuide {

let src = builder.src.join(relative_path);
let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook);
let toolstate = if builder.run_delaying_failure(rustbook_cmd.arg("linkcheck").arg(&src)) {
let cmd = BootstrapCommand::from(rustbook_cmd.arg("linkcheck").arg(&src)).delay_failure();
let toolstate = if builder.run_tracked(cmd).is_success() {
ToolState::TestPass
} else {
ToolState::TestFail
Expand Down Expand Up @@ -2984,7 +2993,7 @@ impl Step for Bootstrap {
.current_dir(builder.src.join("src/bootstrap/"));
// NOTE: we intentionally don't pass test_args here because the args for unittest and cargo test are mutually incompatible.
// Use `python -m unittest` manually if you want to pass arguments.
builder.run_delaying_failure(&mut check_bootstrap);
builder.run_tracked(BootstrapCommand::from(&mut check_bootstrap).delay_failure());

let mut cmd = Command::new(&builder.initial_cargo);
cmd.arg("test")
Expand Down Expand Up @@ -3061,7 +3070,7 @@ impl Step for TierCheck {
self.compiler.host,
self.compiler.host,
);
builder.run_delaying_failure(&mut cargo.into());
builder.run_tracked(BootstrapCommand::from(&mut cargo.into()).delay_failure());
}
}

Expand Down Expand Up @@ -3147,7 +3156,7 @@ impl Step for RustInstaller {
cmd.env("CARGO", &builder.initial_cargo);
cmd.env("RUSTC", &builder.initial_rustc);
cmd.env("TMP_DIR", &tmpdir);
builder.run_delaying_failure(&mut cmd);
builder.run_tracked(BootstrapCommand::from(&mut cmd).delay_failure());
}

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
Expand Down
127 changes: 49 additions & 78 deletions src/bootstrap/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use std::fmt::Display;
use std::fs::{self, File};
use std::io;
use std::path::{Path, PathBuf};
use std::process::{Command, Output, Stdio};
use std::process::{Command, Stdio};
use std::str;
use std::sync::OnceLock;

Expand All @@ -41,7 +41,7 @@ use crate::core::builder::Kind;
use crate::core::config::{flags, LldMode};
use crate::core::config::{DryRun, Target};
use crate::core::config::{LlvmLibunwind, TargetSelection};
use crate::utils::exec::{BehaviorOnFailure, BootstrapCommand, OutputMode};
use crate::utils::exec::{BehaviorOnFailure, BootstrapCommand, CommandOutput, OutputMode};
use crate::utils::helpers::{self, dir_is_empty, exe, libdir, mtime, output, symlink_dir};

mod core;
Expand Down Expand Up @@ -585,8 +585,8 @@ impl Build {
BootstrapCommand::from(submodule_git().args(["diff-index", "--quiet", "HEAD"]))
.allow_failure()
.output_mode(match self.is_verbose() {
true => OutputMode::PrintAll,
false => OutputMode::PrintOutput,
true => OutputMode::All,
false => OutputMode::OnlyOutput,
}),
);
if has_local_modifications {
Expand Down Expand Up @@ -958,73 +958,36 @@ impl Build {
})
}

/// Runs a command, printing out nice contextual information if it fails.
fn run(&self, cmd: &mut Command) {
self.run_cmd(BootstrapCommand::from(cmd).fail_fast().output_mode(
match self.is_verbose() {
true => OutputMode::PrintAll,
false => OutputMode::PrintOutput,
},
));
}

/// Runs a command, printing out contextual info if it fails, and delaying errors until the build finishes.
pub(crate) fn run_delaying_failure(&self, cmd: &mut Command) -> bool {
self.run_cmd(BootstrapCommand::from(cmd).delay_failure().output_mode(
match self.is_verbose() {
true => OutputMode::PrintAll,
false => OutputMode::PrintOutput,
},
))
}

/// Runs a command, printing out nice contextual information if it fails.
fn run_quiet(&self, cmd: &mut Command) {
self.run_cmd(
BootstrapCommand::from(cmd).fail_fast().output_mode(OutputMode::SuppressOnSuccess),
);
}

/// Runs a command, printing out nice contextual information if it fails.
/// Exits if the command failed to execute at all, otherwise returns its
/// `status.success()`.
fn run_quiet_delaying_failure(&self, cmd: &mut Command) -> bool {
self.run_cmd(
BootstrapCommand::from(cmd).delay_failure().output_mode(OutputMode::SuppressOnSuccess),
)
}

/// A centralized function for running commands that do not return output.
pub(crate) fn run_cmd<'a, C: Into<BootstrapCommand<'a>>>(&self, cmd: C) -> bool {
/// Execute a command and return its output.
fn run_tracked(&self, command: BootstrapCommand<'_>) -> CommandOutput {
if self.config.dry_run() {
return true;
return CommandOutput::default();
}

let command = cmd.into();
self.verbose(|| println!("running: {command:?}"));

let (output, print_error) = match command.output_mode {
mode @ (OutputMode::PrintAll | OutputMode::PrintOutput) => (
command.command.status().map(|status| Output {
status,
stdout: Vec::new(),
stderr: Vec::new(),
}),
matches!(mode, OutputMode::PrintAll),
let output_mode = command.output_mode.unwrap_or_else(|| match self.is_verbose() {
true => OutputMode::All,
false => OutputMode::OnlyOutput,
});
let (output, print_error): (io::Result<CommandOutput>, bool) = match output_mode {
mode @ (OutputMode::All | OutputMode::OnlyOutput) => (
command.command.status().map(|status| status.into()),
matches!(mode, OutputMode::All),
),
OutputMode::SuppressOnSuccess => (command.command.output(), true),
OutputMode::OnlyOnFailure => (command.command.output().map(|o| o.into()), true),
};

let output = match output {
Ok(output) => output,
Err(e) => fail(&format!("failed to execute command: {:?}\nerror: {}", command, e)),
};
let result = if !output.status.success() {
if !output.is_success() {
if print_error {
println!(
"\n\nCommand did not execute successfully.\
\nExpected success, got: {}",
output.status,
\nExpected success, got: {}",
output.status(),
);

if !self.is_verbose() {
Expand All @@ -1034,37 +997,45 @@ impl Build {
self.verbose(|| {
println!(
"\nSTDOUT ----\n{}\n\
STDERR ----\n{}\n",
String::from_utf8_lossy(&output.stdout),
String::from_utf8_lossy(&output.stderr)
STDERR ----\n{}\n",
output.stdout(),
output.stderr(),
)
});
}
Err(())
} else {
Ok(())
};

match result {
Ok(_) => true,
Err(_) => {
match command.failure_behavior {
BehaviorOnFailure::DelayFail => {
if self.fail_fast {
exit!(1);
}

let mut failures = self.delayed_failures.borrow_mut();
failures.push(format!("{command:?}"));
}
BehaviorOnFailure::Exit => {
match command.failure_behavior {
BehaviorOnFailure::DelayFail => {
if self.fail_fast {
exit!(1);
}
BehaviorOnFailure::Ignore => {}

let mut failures = self.delayed_failures.borrow_mut();
failures.push(format!("{command:?}"));
}
BehaviorOnFailure::Exit => {
exit!(1);
}
false
BehaviorOnFailure::Ignore => {}
}
}
output
}

/// Runs a command, printing out nice contextual information if it fails.
fn run(&self, cmd: &mut Command) {
self.run_cmd(BootstrapCommand::from(cmd).fail_fast().output_mode(
match self.is_verbose() {
true => OutputMode::All,
false => OutputMode::OnlyOutput,
},
));
}

/// A centralized function for running commands that do not return output.
pub(crate) fn run_cmd<'a, C: Into<BootstrapCommand<'a>>>(&self, cmd: C) -> bool {
let command = cmd.into();
self.run_tracked(command).is_success()
}

/// Check if verbosity is greater than the `level`
Expand Down
Loading
Loading