From 3f3f12c341c7e84294f4b0a548c58fa9ba9d307a Mon Sep 17 00:00:00 2001 From: bit-aloo Date: Thu, 3 Jul 2025 13:59:46 +0530 Subject: [PATCH 1/5] add streaming command struct for (spawn + piping scenario) --- src/bootstrap/src/utils/exec.rs | 74 ++++++++++++++++++++++++++++----- 1 file changed, 64 insertions(+), 10 deletions(-) diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs index d092765ef762f..23296c534bb09 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -13,7 +13,9 @@ use std::fmt::{Debug, Formatter}; use std::hash::Hash; use std::panic::Location; use std::path::Path; -use std::process::{Child, Command, CommandArgs, CommandEnvs, ExitStatus, Output, Stdio}; +use std::process::{ + Child, ChildStderr, ChildStdout, Command, CommandArgs, CommandEnvs, ExitStatus, Output, Stdio, +}; use std::sync::{Arc, Mutex}; use build_helper::ci::CiEnv; @@ -209,15 +211,22 @@ impl<'a> BootstrapCommand { exec_ctx.as_ref().start(self, OutputMode::Capture, OutputMode::Print) } - /// Provides access to the stdlib Command inside. - /// FIXME: This function should be eventually removed from bootstrap. - pub fn as_command_mut(&mut self) -> &mut Command { - // We proactively mark this command as executed since we can't be certain how the returned - // command will be handled. Caching must also be avoided here, as the inner command could be - // modified externally without us being aware. - self.mark_as_executed(); - self.do_not_cache(); - &mut self.command + /// Spawn the command in background, while capturing and returns a handle to stream the output. + #[track_caller] + pub fn stream_capture( + &'a mut self, + exec_ctx: impl AsRef, + ) -> Option { + exec_ctx.as_ref().stream(self, OutputMode::Capture, OutputMode::Capture) + } + + /// Spawn the command in background, while capturing and returning stdout, and printing stderr. + #[track_caller] + pub fn stream_capture_stdout( + &'a mut self, + exec_ctx: impl AsRef, + ) -> Option { + exec_ctx.as_ref().stream(self, OutputMode::Capture, OutputMode::Print) } /// Mark the command as being executed, disarming the drop bomb. @@ -449,6 +458,12 @@ enum CommandState<'a> { }, } +pub struct StreamingCommand { + child: Child, + pub stdout: Option, + pub stderr: Option, +} + #[must_use] pub struct DeferredCommand<'a> { state: CommandState<'a>, @@ -617,6 +632,39 @@ impl ExecutionContext { } exit!(1); } + +<<<<<<< HEAD + pub fn stream<'a>( +======= + /// Spawns the command with configured stdout and stderr handling. + /// + /// Returns `None` if in dry-run mode and the command is not allowed to run. + /// + /// Panics if the command fails to spawn. + pub fn stream( +>>>>>>> c2e83361cec (add comment to exec) + &self, + command: &'a mut BootstrapCommand, + stdout: OutputMode, + stderr: OutputMode, + ) -> Option { + command.mark_as_executed(); + if !command.run_in_dry_run && self.dry_run() { + return None; + } + let cmd = &mut command.command; + cmd.stdout(stdout.stdio()); + cmd.stderr(stderr.stdio()); + let child = cmd.spawn(); + let mut child = match child { + Ok(child) => child, + Err(e) => panic!("failed to execute command: {cmd:?}\nERROR: {e}"), + }; + + let stdout = child.stdout.take(); + let stderr = child.stderr.take(); + return Some(StreamingCommand { child, stdout, stderr }); + } } impl AsRef for ExecutionContext { @@ -625,6 +673,12 @@ impl AsRef for ExecutionContext { } } +impl StreamingCommand { + pub fn wait(mut self) -> Result { + self.child.wait() + } +} + impl<'a> DeferredCommand<'a> { pub fn wait_for_output(self, exec_ctx: impl AsRef) -> CommandOutput { match self.state { From b891d9add1200f29496c66334775a1636cd32151 Mon Sep 17 00:00:00 2001 From: bit-aloo Date: Thu, 3 Jul 2025 14:00:20 +0530 Subject: [PATCH 2/5] migrate cargo streaming to new bootstrap command streaming API's --- src/bootstrap/src/core/build_steps/compile.rs | 24 ++++++++----------- src/bootstrap/src/utils/exec.rs | 1 + 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs index c3a3eddd16128..431c242608b70 100644 --- a/src/bootstrap/src/core/build_steps/compile.rs +++ b/src/bootstrap/src/core/build_steps/compile.rs @@ -12,7 +12,6 @@ use std::ffi::OsStr; use std::io::BufReader; use std::io::prelude::*; use std::path::{Path, PathBuf}; -use std::process::Stdio; use std::{env, fs, str}; use serde_derive::Deserialize; @@ -2507,7 +2506,6 @@ pub fn stream_cargo( #[cfg(feature = "tracing")] let _run_span = crate::trace_cmd!(cmd); - let cargo = cmd.as_command_mut(); // 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 { @@ -2519,27 +2517,24 @@ pub fn stream_cargo( message_format.push_str(",json-diagnostic-"); message_format.push_str(s); } - cargo.arg("--message-format").arg(message_format).stdout(Stdio::piped()); + cmd.arg("--message-format").arg(message_format); for arg in tail_args { - cargo.arg(arg); + cmd.arg(arg); } - builder.verbose(|| println!("running: {cargo:?}")); + builder.verbose(|| println!("running: {cmd:?}")); - if builder.config.dry_run() { - return true; - } + let streaming_command = cmd.stream_capture_stdout(&builder.config.exec_ctx); - let mut child = match cargo.spawn() { - Ok(child) => child, - Err(e) => panic!("failed to execute command: {cargo:?}\nERROR: {e}"), + let Some(mut streaming_command) = streaming_command else { + return true; }; // Spawn Cargo slurping up its JSON output. We'll start building up the // `deps` array of all files it generated along with a `toplevel` array of // files we need to probe for later. - let stdout = BufReader::new(child.stdout.take().unwrap()); + let stdout = BufReader::new(streaming_command.stdout.take().unwrap()); for line in stdout.lines() { let line = t!(line); match serde_json::from_str::>(&line) { @@ -2556,13 +2551,14 @@ pub fn stream_cargo( } // Make sure Cargo actually succeeded after we read all of its stdout. - let status = t!(child.wait()); + let status = t!(streaming_command.wait()); if builder.is_verbose() && !status.success() { eprintln!( - "command did not execute successfully: {cargo:?}\n\ + "command did not execute successfully: {cmd:?}\n\ expected success, got: {status}" ); } + status.success() } diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs index 23296c534bb09..d1822752f9206 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -221,6 +221,7 @@ impl<'a> BootstrapCommand { } /// Spawn the command in background, while capturing and returning stdout, and printing stderr. + /// Returns None in dry-mode #[track_caller] pub fn stream_capture_stdout( &'a mut self, From 0a57612e63e5110d8a93cc000755b0395fca018d Mon Sep 17 00:00:00 2001 From: bit-aloo Date: Thu, 3 Jul 2025 14:00:46 +0530 Subject: [PATCH 3/5] migrate render test to new bootstrap command streaming API's --- src/bootstrap/src/utils/render_tests.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/bootstrap/src/utils/render_tests.rs b/src/bootstrap/src/utils/render_tests.rs index 051d7dd9fd4dd..283563f3aca92 100644 --- a/src/bootstrap/src/utils/render_tests.rs +++ b/src/bootstrap/src/utils/render_tests.rs @@ -7,7 +7,7 @@ //! to reimplement all the rendering logic in this module because of that. use std::io::{BufRead, BufReader, Read, Write}; -use std::process::{ChildStdout, Stdio}; +use std::process::ChildStdout; use std::time::Duration; use termcolor::{Color, ColorSpec, WriteColor}; @@ -52,32 +52,28 @@ pub(crate) fn try_run_tests( } fn run_tests(builder: &Builder<'_>, cmd: &mut BootstrapCommand, stream: bool) -> bool { - let cmd = cmd.as_command_mut(); - cmd.stdout(Stdio::piped()); - builder.verbose(|| println!("running: {cmd:?}")); - let mut process = cmd.spawn().unwrap(); + let mut streaming_command = cmd.stream_capture_stdout(&builder.config.exec_ctx).unwrap(); // This runs until the stdout of the child is closed, which means the child exited. We don't // run this on another thread since the builder is not Sync. - let renderer = Renderer::new(process.stdout.take().unwrap(), builder); + let renderer = Renderer::new(streaming_command.stdout.take().unwrap(), builder); if stream { renderer.stream_all(); } else { renderer.render_all(); } - let result = process.wait_with_output().unwrap(); - if !result.status.success() && builder.is_verbose() { + let status = streaming_command.wait().unwrap(); + if !status.success() && builder.is_verbose() { println!( "\n\ncommand did not execute successfully: {cmd:?}\n\ - expected success, got: {}", - result.status + expected success, got: {status}", ); } - result.status.success() + status.success() } struct Renderer<'a> { From 8199c950e83b1ae7913f5c1a9b5608065b299039 Mon Sep 17 00:00:00 2001 From: bit-aloo Date: Fri, 4 Jul 2025 20:51:17 +0530 Subject: [PATCH 4/5] make mark_as_executed private --- src/bootstrap/src/utils/exec.rs | 13 ++----------- src/bootstrap/src/utils/render_tests.rs | 9 +++------ 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs index d1822752f9206..c5b3c322be4de 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -211,15 +211,6 @@ impl<'a> BootstrapCommand { exec_ctx.as_ref().start(self, OutputMode::Capture, OutputMode::Print) } - /// Spawn the command in background, while capturing and returns a handle to stream the output. - #[track_caller] - pub fn stream_capture( - &'a mut self, - exec_ctx: impl AsRef, - ) -> Option { - exec_ctx.as_ref().stream(self, OutputMode::Capture, OutputMode::Capture) - } - /// Spawn the command in background, while capturing and returning stdout, and printing stderr. /// Returns None in dry-mode #[track_caller] @@ -232,7 +223,7 @@ impl<'a> BootstrapCommand { /// Mark the command as being executed, disarming the drop bomb. /// If this method is not called before the command is dropped, its drop will panic. - pub fn mark_as_executed(&mut self) { + fn mark_as_executed(&mut self) { self.drop_bomb.defuse(); } @@ -664,7 +655,7 @@ impl ExecutionContext { let stdout = child.stdout.take(); let stderr = child.stderr.take(); - return Some(StreamingCommand { child, stdout, stderr }); + Some(StreamingCommand { child, stdout, stderr }) } } diff --git a/src/bootstrap/src/utils/render_tests.rs b/src/bootstrap/src/utils/render_tests.rs index 283563f3aca92..73a22ec282604 100644 --- a/src/bootstrap/src/utils/render_tests.rs +++ b/src/bootstrap/src/utils/render_tests.rs @@ -34,11 +34,6 @@ pub(crate) fn try_run_tests( cmd: &mut BootstrapCommand, stream: bool, ) -> bool { - if builder.config.dry_run() { - cmd.mark_as_executed(); - return true; - } - if !run_tests(builder, cmd, stream) { if builder.fail_fast { crate::exit!(1); @@ -54,7 +49,9 @@ pub(crate) fn try_run_tests( fn run_tests(builder: &Builder<'_>, cmd: &mut BootstrapCommand, stream: bool) -> bool { builder.verbose(|| println!("running: {cmd:?}")); - let mut streaming_command = cmd.stream_capture_stdout(&builder.config.exec_ctx).unwrap(); + let Some(mut streaming_command) = cmd.stream_capture_stdout(&builder.config.exec_ctx) else { + return true; + }; // This runs until the stdout of the child is closed, which means the child exited. We don't // run this on another thread since the builder is not Sync. From 312de35d3aa33c2c58386ca1544046c54c6d4da2 Mon Sep 17 00:00:00 2001 From: bit-aloo Date: Fri, 4 Jul 2025 20:54:09 +0530 Subject: [PATCH 5/5] restructure try_run_tests --- src/bootstrap/src/utils/exec.rs | 12 +++--------- src/bootstrap/src/utils/render_tests.rs | 19 ++++++++++--------- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs index c5b3c322be4de..487077835ac6c 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -223,7 +223,7 @@ impl<'a> BootstrapCommand { /// Mark the command as being executed, disarming the drop bomb. /// If this method is not called before the command is dropped, its drop will panic. - fn mark_as_executed(&mut self) { + pub fn mark_as_executed(&mut self) { self.drop_bomb.defuse(); } @@ -625,18 +625,12 @@ impl ExecutionContext { exit!(1); } -<<<<<<< HEAD - pub fn stream<'a>( -======= /// Spawns the command with configured stdout and stderr handling. /// - /// Returns `None` if in dry-run mode and the command is not allowed to run. - /// - /// Panics if the command fails to spawn. + /// Returns None if in dry-run mode or Panics if the command fails to spawn. pub fn stream( ->>>>>>> c2e83361cec (add comment to exec) &self, - command: &'a mut BootstrapCommand, + command: &mut BootstrapCommand, stdout: OutputMode, stderr: OutputMode, ) -> Option { diff --git a/src/bootstrap/src/utils/render_tests.rs b/src/bootstrap/src/utils/render_tests.rs index 73a22ec282604..934699d736b46 100644 --- a/src/bootstrap/src/utils/render_tests.rs +++ b/src/bootstrap/src/utils/render_tests.rs @@ -34,16 +34,17 @@ pub(crate) fn try_run_tests( cmd: &mut BootstrapCommand, stream: bool, ) -> bool { - if !run_tests(builder, cmd, stream) { - if builder.fail_fast { - crate::exit!(1); - } else { - builder.config.exec_ctx().add_to_delay_failure(format!("{cmd:?}")); - false - } - } else { - true + if run_tests(builder, cmd, stream) { + return true; } + + if builder.fail_fast { + crate::exit!(1); + } + + builder.config.exec_ctx().add_to_delay_failure(format!("{cmd:?}")); + + false } fn run_tests(builder: &Builder<'_>, cmd: &mut BootstrapCommand, stream: bool) -> bool {