diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs index de3b938e427..c267d3b0c0c 100644 --- a/src/bootstrap/src/core/build_steps/compile.rs +++ b/src/bootstrap/src/core/build_steps/compile.rs @@ -2080,7 +2080,7 @@ pub fn stream_cargo( tail_args: Vec, cb: &mut dyn FnMut(CargoMessage<'_>), ) -> bool { - let mut cargo = BootstrapCommand::from(cargo).command; + let mut cargo = cargo.into_cmd().command; // Instruct Cargo to give us json messages on stdout, critically leaving // stderr as piped so we can get those pretty colors. let mut message_format = if builder.config.json_output { diff --git a/src/bootstrap/src/core/build_steps/doc.rs b/src/bootstrap/src/core/build_steps/doc.rs index 4a5af25b3b2..823e842693e 100644 --- a/src/bootstrap/src/core/build_steps/doc.rs +++ b/src/bootstrap/src/core/build_steps/doc.rs @@ -738,7 +738,7 @@ fn doc_std( format!("library{} in {} format", crate_description(requested_crates), format.as_str()); let _guard = builder.msg_doc(compiler, description, target); - builder.run(cargo); + builder.run(cargo.into_cmd()); builder.cp_link_r(&out_dir, out); } @@ -863,7 +863,7 @@ fn run(self, builder: &Builder<'_>) { let proc_macro_out_dir = builder.stage_out(compiler, Mode::Rustc).join("doc"); symlink_dir_force(&builder.config, &out, &proc_macro_out_dir); - builder.run(cargo); + builder.run(cargo.into_cmd()); if !builder.config.dry_run() { // Sanity check on linked compiler crates @@ -996,7 +996,7 @@ fn run(self, builder: &Builder<'_>) { symlink_dir_force(&builder.config, &out, &proc_macro_out_dir); let _guard = builder.msg_doc(compiler, stringify!($tool).to_lowercase(), target); - builder.run(cargo); + builder.run(cargo.into_cmd()); if !builder.config.dry_run() { // Sanity check on linked doc directories diff --git a/src/bootstrap/src/core/build_steps/format.rs b/src/bootstrap/src/core/build_steps/format.rs index 4583eb1c6cd..0372360c3cb 100644 --- a/src/bootstrap/src/core/build_steps/format.rs +++ b/src/bootstrap/src/core/build_steps/format.rs @@ -160,16 +160,15 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) { override_builder.add(&format!("!{ignore}")).expect(&ignore); } } - let git_available = build - .run(helpers::git(None).print_on_failure().allow_failure().arg("--version")) - .is_success(); + let git_available = + build.run(helpers::git(None).capture().allow_failure().arg("--version")).is_success(); let mut adjective = None; if git_available { let in_working_tree = build .run( helpers::git(Some(&build.src)) - .print_on_failure() + .capture() .allow_failure() .arg("rev-parse") .arg("--is-inside-work-tree"), diff --git a/src/bootstrap/src/core/build_steps/run.rs b/src/bootstrap/src/core/build_steps/run.rs index 22d5efa5d95..496fe446d72 100644 --- a/src/bootstrap/src/core/build_steps/run.rs +++ b/src/bootstrap/src/core/build_steps/run.rs @@ -158,7 +158,7 @@ fn run(self, builder: &Builder<'_>) { // after another --, so this must be at the end. miri.args(builder.config.args()); - builder.run(miri); + builder.run(miri.into_cmd()); } } diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 918e4e2b907..9fc337dfd50 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -26,7 +26,7 @@ use crate::core::config::flags::get_completion; use crate::core::config::flags::Subcommand; use crate::core::config::TargetSelection; -use crate::utils::exec::{BootstrapCommand, OutputMode}; +use crate::utils::exec::BootstrapCommand; 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, @@ -150,16 +150,13 @@ fn run(self, builder: &Builder<'_>) { builder.default_doc(&[]); // Build the linkchecker before calling `msg`, since GHA doesn't support nested groups. - let mut linkchecker = builder.tool_cmd(Tool::Linkchecker); + let linkchecker = builder.tool_cmd(Tool::Linkchecker); // Run the linkchecker. let _guard = builder.msg(Kind::Test, compiler.stage, "Linkcheck", bootstrap_host, bootstrap_host); let _time = helpers::timeit(builder); - builder.run( - BootstrapCommand::from(linkchecker.arg(builder.out.join(host.triple).join("doc"))) - .delay_failure(), - ); + builder.run(linkchecker.delay_failure().arg(builder.out.join(host.triple).join("doc"))); } fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { @@ -217,10 +214,7 @@ fn run(self, builder: &Builder<'_>) { )); builder.run( - BootstrapCommand::from( - builder.tool_cmd(Tool::HtmlChecker).arg(builder.doc_out(self.target)), - ) - .delay_failure(), + builder.tool_cmd(Tool::HtmlChecker).delay_failure().arg(builder.doc_out(self.target)), ); } } @@ -260,14 +254,13 @@ fn run(self, builder: &Builder<'_>) { let _time = helpers::timeit(builder); let mut cmd = builder.tool_cmd(Tool::CargoTest); - let cmd = cmd - .arg(&cargo) + cmd.arg(&cargo) .arg(&out_dir) .args(builder.config.test_args()) .env("RUSTC", builder.rustc(compiler)) .env("RUSTDOC", builder.rustdoc(compiler)); - add_rustdoc_cargo_linker_args(cmd, builder, compiler.host, LldThreads::No); - builder.run(BootstrapCommand::from(cmd).delay_failure()); + add_rustdoc_cargo_linker_args(&mut cmd, builder, compiler.host, LldThreads::No); + builder.run(cmd.delay_failure()); } } @@ -763,12 +756,12 @@ fn run(self, builder: &Builder<'_>) { cargo.env("HOST_LIBS", host_libs); cargo.add_rustc_lib_path(builder); - let mut cargo = prepare_cargo_test(cargo, &[], &[], "clippy", compiler, host, builder); + let cargo = prepare_cargo_test(cargo, &[], &[], "clippy", compiler, host, builder); let _guard = builder.msg_sysroot_tool(Kind::Test, compiler.stage, "clippy", host, host); // Clippy reports errors if it blessed the outputs - if builder.run(BootstrapCommand::from(&mut cargo).allow_failure()).is_success() { + if builder.run(cargo.allow_failure()).is_success() { // The tests succeeded; nothing to do. return; } @@ -821,7 +814,7 @@ fn run(self, builder: &Builder<'_>) { .env("RUSTC_BOOTSTRAP", "1"); cmd.args(linker_args(builder, self.compiler.host, LldThreads::No)); - builder.run(BootstrapCommand::from(&mut cmd).delay_failure()); + builder.run(cmd.delay_failure()); } } @@ -1099,7 +1092,7 @@ fn run(self, builder: &Builder<'_>) { } builder.info("tidy check"); - builder.run(BootstrapCommand::from(&mut cmd).delay_failure()); + builder.run(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"] @@ -1306,7 +1299,7 @@ fn run(self, builder: &Builder<'_>) -> PathBuf { &[], ); - builder.run(cargo); + builder.run(cargo.into_cmd()); let lib_name = "librun_make_support.rlib"; let lib = builder.tools_dir(self.compiler).join(lib_name); @@ -2187,7 +2180,7 @@ fn run_ext_doc(self, builder: &Builder<'_>) { compiler.host, ); let _time = helpers::timeit(builder); - let cmd = BootstrapCommand::from(&mut rustbook_cmd).delay_failure(); + let cmd = rustbook_cmd.delay_failure(); let toolstate = if builder.run(cmd).is_success() { ToolState::TestPass } else { ToolState::TestFail }; builder.save_toolstate(self.name, toolstate); @@ -2318,7 +2311,7 @@ fn run(self, builder: &Builder<'_>) { let guard = builder.msg(Kind::Test, compiler.stage, "error-index", compiler.host, compiler.host); let _time = helpers::timeit(builder); - builder.run(BootstrapCommand::from(&mut tool).output_mode(OutputMode::OnlyOnFailure)); + builder.run(tool.capture()); drop(guard); // The tests themselves need to link to std, so make sure it is // available. @@ -2349,7 +2342,7 @@ fn markdown_test(builder: &Builder<'_>, compiler: Compiler, markdown: &Path) -> cmd = cmd.delay_failure(); if !builder.config.verbose_tests { - cmd = cmd.print_on_failure(); + cmd = cmd.capture(); } builder.run(cmd).is_success() } @@ -2375,10 +2368,13 @@ fn run(self, builder: &Builder<'_>) { builder.update_submodule(&relative_path); let src = builder.src.join(relative_path); - let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook); - let cmd = BootstrapCommand::from(rustbook_cmd.arg("linkcheck").arg(&src)).delay_failure(); - let toolstate = - if builder.run(cmd).is_success() { ToolState::TestPass } else { ToolState::TestFail }; + let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook).delay_failure(); + rustbook_cmd.arg("linkcheck").arg(&src); + let toolstate = if builder.run(rustbook_cmd).is_success() { + ToolState::TestPass + } else { + ToolState::TestFail + }; builder.save_toolstate("rustc-dev-guide", toolstate); } } @@ -3347,7 +3343,7 @@ fn run(self, builder: &Builder<'_>) { .arg("testsuite.extended_sysroot"); cargo.args(builder.config.test_args()); - builder.run(cargo); + builder.run(cargo.into_cmd()); } } @@ -3472,6 +3468,6 @@ fn run(self, builder: &Builder<'_>) { .arg("--std-tests"); cargo.args(builder.config.test_args()); - builder.run(cargo); + builder.run(cargo.into_cmd()); } } diff --git a/src/bootstrap/src/core/build_steps/tool.rs b/src/bootstrap/src/core/build_steps/tool.rs index b3464043912..20c7a30f1a8 100644 --- a/src/bootstrap/src/core/build_steps/tool.rs +++ b/src/bootstrap/src/core/build_steps/tool.rs @@ -603,7 +603,7 @@ fn run(self, builder: &Builder<'_>) -> PathBuf { &self.compiler.host, &target, ); - builder.run(cargo); + builder.run(cargo.into_cmd()); // Cargo adds a number of paths to the dylib search path on windows, which results in // the wrong rustdoc being executed. To avoid the conflicting rustdocs, we name the "tool" @@ -858,7 +858,7 @@ fn run(self, builder: &Builder<'_>) -> PathBuf { &self.extra_features, ); - builder.run(cargo); + builder.run(cargo.into_cmd()); let tool_out = builder .cargo_out(self.compiler, Mode::ToolRustc, self.target) diff --git a/src/bootstrap/src/core/builder.rs b/src/bootstrap/src/core/builder.rs index 58d6e7a58e3..35de91c41d4 100644 --- a/src/bootstrap/src/core/builder.rs +++ b/src/bootstrap/src/core/builder.rs @@ -2398,6 +2398,10 @@ pub fn new( cargo } + pub fn into_cmd(self) -> BootstrapCommand { + self.into() + } + /// Same as `Cargo::new` except this one doesn't configure the linker with `Cargo::configure_linker` pub fn new_for_mir_opt_tests( builder: &Builder<'_>, @@ -2622,9 +2626,3 @@ fn from(mut cargo: Cargo) -> BootstrapCommand { cargo.command } } - -impl From for Command { - fn from(cargo: Cargo) -> Command { - BootstrapCommand::from(cargo).command - } -} diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index ae2982ea56f..ae3d18e8774 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -555,10 +555,7 @@ pub(crate) fn update_submodule(&self, relative_path: &Path) { // Save any local changes, but avoid running `git stash pop` if there are none (since it will exit with an error). // diff-index reports the modifications through the exit status let has_local_modifications = self - .run( - BootstrapCommand::from(submodule_git().args(["diff-index", "--quiet", "HEAD"])) - .allow_failure(), - ) + .run(submodule_git().allow_failure().args(["diff-index", "--quiet", "HEAD"])) .is_failure(); if has_local_modifications { self.run(submodule_git().args(["stash", "push"])); @@ -582,7 +579,7 @@ pub fn update_existing_submodules(&self) { let output = self .run( helpers::git(Some(&self.src)) - .quiet() + .capture() .args(["config", "--file"]) .arg(self.config.src.join(".gitmodules")) .args(["--get-regexp", "path"]), @@ -937,69 +934,71 @@ fn rustc_snapshot_sysroot(&self) -> &Path { /// Execute a command and return its output. /// This method should be used for all command executions in bootstrap. - fn run>(&self, command: C) -> CommandOutput { + fn run>(&self, mut command: C) -> CommandOutput { if self.config.dry_run() { return CommandOutput::default(); } - let mut command = command.into(); + let command = command.as_mut(); self.verbose(|| println!("running: {command:?}")); - 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, bool) = match output_mode { - mode @ (OutputMode::All | OutputMode::OnlyOutput) => ( - command.command.status().map(|status| status.into()), - matches!(mode, OutputMode::All), - ), - mode @ (OutputMode::OnlyOnFailure | OutputMode::Quiet) => ( - command.command.output().map(|o| o.into()), - matches!(mode, OutputMode::OnlyOnFailure), - ), + let output: io::Result = match command.output_mode { + OutputMode::Print => command.command.status().map(|status| status.into()), + OutputMode::CaptureAll => command.command.output().map(|o| o.into()), + OutputMode::CaptureStdout => { + command.command.stderr(Stdio::inherit()); + command.command.output().map(|o| o.into()) + } }; let output = match output { Ok(output) => output, - Err(e) => fail(&format!("failed to execute command: {:?}\nerror: {}", command, e)), + Err(e) => fail(&format!("failed to execute command: {command:?}\nerror: {e}")), }; if !output.is_success() { - if print_error { - println!( - "\n\nCommand did not execute successfully.\ - \nExpected success, got: {}", - output.status(), - ); + use std::fmt::Write; - if !self.is_verbose() { - println!("Add `-v` to see more details.\n"); + // Here we build an error message, and below we decide if it should be printed or not. + let mut message = String::new(); + writeln!( + message, + "\n\nCommand {command:?} did not execute successfully.\ + \nExpected success, got: {}", + output.status(), + ) + .unwrap(); + + // If the output mode is OutputMode::Print, the output has already been printed to + // stdout/stderr, and we thus don't have anything captured to print anyway. + if matches!(command.output_mode, OutputMode::CaptureAll | OutputMode::CaptureStdout) { + writeln!(message, "\nSTDOUT ----\n{}", output.stdout().trim()).unwrap(); + + // Stderr is added to the message only if it was captured + if matches!(command.output_mode, OutputMode::CaptureAll) { + writeln!(message, "\nSTDERR ----\n{}", output.stderr().trim()).unwrap(); } - - self.verbose(|| { - println!( - "\nSTDOUT ----\n{}\n\ - STDERR ----\n{}\n", - output.stdout(), - output.stderr(), - ) - }); } match command.failure_behavior { BehaviorOnFailure::DelayFail => { if self.fail_fast { + println!("{message}"); exit!(1); } let mut failures = self.delayed_failures.borrow_mut(); - failures.push(format!("{command:?}")); + failures.push(message); } BehaviorOnFailure::Exit => { + println!("{message}"); exit!(1); } - BehaviorOnFailure::Ignore => {} + BehaviorOnFailure::Ignore => { + // If failures are allowed, either the error has been printed already + // (OutputMode::Print) or the user used a capture output mode and wants to + // handle the error output on their own. + } } } output @@ -1490,7 +1489,7 @@ fn extract_beta_rev_from_file>(version_file: P) -> Option // (Note that we use a `..` range, not the `...` symmetric difference.) self.run( helpers::git(Some(&self.src)) - .quiet() + .capture() .arg("rev-list") .arg("--count") .arg("--merges") diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs index dc30876601c..5f9e1648161 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -13,18 +13,19 @@ pub enum BehaviorOnFailure { Ignore, } -/// How should the output of the command be handled. +/// How should the output of the command be handled (whether it should be captured or printed). #[derive(Debug, Copy, Clone)] pub enum OutputMode { - /// Print both the output (by inheriting stdout/stderr) and also the command itself, if it - /// fails. - All, - /// Print the output (by inheriting stdout/stderr). - OnlyOutput, - /// Suppress the output if the command succeeds, otherwise print the output. - OnlyOnFailure, - /// Suppress the output of the command. - Quiet, + /// Prints the stdout/stderr of the command to stdout/stderr of bootstrap (by inheriting these + /// streams). + /// Corresponds to calling `cmd.status()`. + Print, + /// Captures the stdout and stderr of the command into memory. + /// Corresponds to calling `cmd.output()`. + CaptureAll, + /// Captures the stdout of the command into memory, inherits stderr. + /// Corresponds to calling `cmd.output()`. + CaptureStdout, } /// Wrapper around `std::process::Command`. @@ -34,10 +35,10 @@ pub enum OutputMode { /// If you want to delay failures until the end of bootstrap, use [delay_failure]. /// /// By default, the command will print its stdout/stderr to stdout/stderr of bootstrap -/// ([OutputMode::OnlyOutput]). If bootstrap uses verbose mode, then it will also print the -/// command itself in case of failure ([OutputMode::All]). -/// If you want to handle the output programmatically, use `output_mode(OutputMode::OnlyOnFailure)`, -/// which will print the output only if the command fails. +/// ([OutputMode::Print]). +/// If you want to handle the output programmatically, use [BootstrapCommand::capture]. +/// +/// Bootstrap will print a debug log to stdout if the command fails and failure is not allowed. /// /// [allow_failure]: BootstrapCommand::allow_failure /// [delay_failure]: BootstrapCommand::delay_failure @@ -45,12 +46,16 @@ pub enum OutputMode { pub struct BootstrapCommand { pub command: Command, pub failure_behavior: BehaviorOnFailure, - pub output_mode: Option, + pub output_mode: OutputMode, } impl BootstrapCommand { pub fn new>(program: S) -> Self { - Command::new(program).into() + Self { + command: Command::new(program), + failure_behavior: BehaviorOnFailure::Exit, + output_mode: OutputMode::Print, + } } pub fn arg>(&mut self, arg: S) -> &mut Self { @@ -106,52 +111,22 @@ pub fn allow_failure(self) -> Self { Self { failure_behavior: BehaviorOnFailure::Ignore, ..self } } - /// Do not print the output of the command, unless it fails. - pub fn print_on_failure(self) -> Self { - self.output_mode(OutputMode::OnlyOnFailure) + /// Capture the output of the command, do not print it. + pub fn capture(self) -> Self { + Self { output_mode: OutputMode::CaptureAll, ..self } } - /// Do not print the output of the command. - pub fn quiet(self) -> Self { - self.output_mode(OutputMode::Quiet) - } - - pub fn output_mode(self, output_mode: OutputMode) -> Self { - Self { output_mode: Some(output_mode), ..self } + /// Capture stdout of the command, do not print it. + pub fn capture_stdout(self) -> Self { + Self { output_mode: OutputMode::CaptureStdout, ..self } } } -/// FIXME: This implementation is temporary, until all `Command` invocations are migrated to -/// `BootstrapCommand`. -impl<'a> From<&'a mut BootstrapCommand> for BootstrapCommand { - fn from(command: &'a mut BootstrapCommand) -> Self { - // This is essentially a manual `Command::clone` - let mut cmd = Command::new(command.command.get_program()); - if let Some(dir) = command.command.get_current_dir() { - cmd.current_dir(dir); - } - cmd.args(command.command.get_args()); - for (key, value) in command.command.get_envs() { - match value { - Some(value) => { - cmd.env(key, value); - } - None => { - cmd.env_remove(key); - } - } - } - Self { - command: cmd, - output_mode: command.output_mode, - failure_behavior: command.failure_behavior, - } - } -} - -impl From for BootstrapCommand { - fn from(command: Command) -> Self { - Self { command, failure_behavior: BehaviorOnFailure::Exit, output_mode: None } +/// This implementation exists to make it possible to pass both [BootstrapCommand] and +/// `&mut BootstrapCommand` to `Build.run()`. +impl AsMut for BootstrapCommand { + fn as_mut(&mut self) -> &mut BootstrapCommand { + self } } @@ -176,6 +151,10 @@ pub fn stdout(&self) -> String { String::from_utf8(self.0.stdout.clone()).expect("Cannot parse process stdout as UTF-8") } + pub fn stdout_if_ok(&self) -> Option { + if self.is_success() { Some(self.stdout()) } else { None } + } + pub fn stderr(&self) -> String { String::from_utf8(self.0.stderr.clone()).expect("Cannot parse process stderr as UTF-8") }