Merge pull request #626 from nix-community/notashelf/push-zlyknnslyqoz

Fix output visibility for nix copy commands after subprocess update

Author: NotAShelf

Cargo.toml

@@ -48,7 +48,7 @@ serde_json = "1.0.145"
 serial_test = "3.2.0"
 shlex = "1.3.0"
 signal-hook = "0.4.1"
-subprocess = "1.0.0"
+subprocess = "1.0.3"
 supports-hyperlinks = "3.1.0"
 system-configuration = "0.7.0"
 tempfile = "3.23.0"

crates/nh-core/src/command.rs

@@ -2,6 +2,7 @@ use std::{
   collections::HashMap,
   convert::Infallible,
   ffi::{OsStr, OsString},
+  io::{Read, Write},
   path::PathBuf,
   str::FromStr,
   sync::{Mutex, OnceLock},
@@ -19,6 +20,118 @@ use which::which;
 
 use crate::{args::NixBuildPassthroughArgs, installable::Installable};
 
+/// Execute a command, streaming output to stdout/stderr while optionally
+/// capturing it for error reporting.
+///
+/// # Arguments
+///
+/// * `capture_output` - When `true`, stdout and stderr are accumulated and
+///   returned as strings. When `false`, output is streamed but not captured.
+///
+/// # Returns
+///
+/// Returns the exit status and captured stdout/stderr (or empty strings if
+/// `capture_output` is `false`), or an error.
+///
+/// # Errors
+///
+/// Returns an error if:
+///
+/// - The command fails to start
+/// - stdout or stderr cannot be captured
+/// - The command fails to complete
+/// - Either output thread panics
+pub fn exec_with_streaming(
+  cmd: Exec,
+  capture_output: bool,
+) -> Result<(subprocess::ExitStatus, String, String)> {
+  let mut job = cmd
+    .stdout(Redirection::Pipe)
+    .start()
+    .wrap_err("Failed to start command")?;
+
+  let stdout_pipe = job
+    .stdout
+    .take()
+    .ok_or_else(|| eyre::eyre!("Failed to capture stdout"))?;
+
+  let stdout_thread = std::thread::spawn(move || {
+    let mut stdout_reader = std::io::BufReader::new(stdout_pipe);
+    let mut stdout_bytes = Vec::new();
+    let mut stdout_buf = [0u8; 4096];
+
+    loop {
+      match stdout_reader.read(&mut stdout_buf) {
+        Ok(0) => break,
+        Ok(n) => {
+          let _ = std::io::stdout().write_all(&stdout_buf[..n]);
+          let _ = std::io::stdout().flush();
+          if capture_output {
+            stdout_bytes.extend_from_slice(&stdout_buf[..n]);
+          }
+        },
+        Err(e) => {
+          debug!("stdout read error: {e}");
+          break;
+        },
+      }
+    }
+
+    if capture_output {
+      String::from_utf8_lossy(&stdout_bytes).into_owned()
+    } else {
+      String::new()
+    }
+  });
+
+  let stderr_thread = if let Some(stderr_pipe) = job.stderr.take() {
+    Some(std::thread::spawn(move || {
+      let mut stderr_reader = std::io::BufReader::new(stderr_pipe);
+      let mut stderr_bytes = Vec::new();
+      let mut stderr_buf = [0u8; 4096];
+
+      loop {
+        match stderr_reader.read(&mut stderr_buf) {
+          Ok(0) => break,
+          Ok(n) => {
+            let _ = std::io::stderr().write_all(&stderr_buf[..n]);
+            let _ = std::io::stderr().flush();
+            if capture_output {
+              stderr_bytes.extend_from_slice(&stderr_buf[..n]);
+            }
+          },
+          Err(e) => {
+            debug!("stderr read error: {e}");
+            break;
+          },
+        }
+      }
+
+      if capture_output {
+        String::from_utf8_lossy(&stderr_bytes).into_owned()
+      } else {
+        String::new()
+      }
+    }))
+  } else {
+    None
+  };
+
+  let exit_status = job
+    .wait()
+    .wrap_err("Failed to wait for command completion")?;
+
+  let stdout_output = stdout_thread
+    .join()
+    .map_err(|_| eyre::eyre!("Stdout thread panicked"))?;
+  let stderr_output = stderr_thread
+    .map(|t| t.join().map_err(|_| eyre::eyre!("Stderr thread panicked")))
+    .transpose()?
+    .unwrap_or_default();
+
+  Ok((exit_status, stdout_output, stderr_output))
+}
+
 static PASSWORD_CACHE: OnceLock<Mutex<HashMap<String, SecretString>>> =
   OnceLock::new();
 
@@ -729,26 +842,38 @@ impl Command {
       .message
       .clone()
       .unwrap_or_else(|| "Command failed".to_string());
-    let res = cmd.capture();
-    match res {
-      Ok(capture) => {
-        let status = &capture.exit_status;
-        if !status.success() {
-          let stderr = capture.stderr_str();
-          if stderr.trim().is_empty() {
+
+    if self.show_output {
+      let exit_status = cmd.join().wrap_err(msg.clone())?;
+      if !exit_status.success() {
+        return Err(eyre::eyre!(format!(
+          "{} (exit status {:?})",
+          msg, exit_status
+        )));
+      }
+      Ok(())
+    } else {
+      let res = cmd.capture();
+      match res {
+        Ok(capture) => {
+          let status = &capture.exit_status;
+          if !status.success() {
+            let stderr = capture.stderr_str();
+            if stderr.trim().is_empty() {
+              return Err(eyre::eyre!(format!(
+                "{} (exit status {:?})",
+                msg, status
+              )));
+            }
             return Err(eyre::eyre!(format!(
-              "{} (exit status {:?})",
-              msg, status
+              "{} (exit status {:?})\nstderr:\n{}",
+              msg, status, stderr
             )));
           }
-          return Err(eyre::eyre!(format!(
-            "{} (exit status {:?})\nstderr:\n{}",
-            msg, status, stderr
-          )));
-        }
-        Ok(())
-      },
-      Err(e) => Err(e).wrap_err(msg),
+          Ok(())
+        },
+        Err(e) => Err(e).wrap_err(msg),
+      }
     }
   }
 

crates/nh-remote/src/remote.rs

@@ -18,7 +18,12 @@ use color_eyre::{
 };
 use indicatif::{ProgressBar, ProgressStyle};
 use nh_core::{
-  command::{ElevationStrategy, cache_password, get_cached_password},
+  command::{
+    ElevationStrategy,
+    cache_password,
+    exec_with_streaming,
+    get_cached_password,
+  },
   installable::Installable,
   util::NixVariant,
 };
@@ -821,15 +826,14 @@ fn copy_closure_to(
 
   debug!(?cmd, "nix-copy-closure --to");
 
-  let capture = cmd
-    .capture()
+  let (exit_status, _stdout, _stderr) = exec_with_streaming(cmd, false)
     .wrap_err("Failed to copy closure to remote host")?;
 
-  if !capture.exit_status.success() {
+  if !exit_status.success() {
     bail!(
-      "nix-copy-closure --to '{}' failed:\n{}",
+      "nix-copy-closure --to '{}' failed (exit status: {:?})",
       host,
-      capture.stderr_str()
+      exit_status
     );
   }
 
@@ -974,15 +978,14 @@ fn copy_closure_from(
 
   debug!(?cmd, "nix-copy-closure --from");
 
-  let capture = cmd
-    .capture()
+  let (exit_status, _stdout, _stderr) = exec_with_streaming(cmd, false)
     .wrap_err("Failed to copy closure from remote host")?;
 
-  if !capture.exit_status.success() {
+  if !exit_status.success() {
     bail!(
-      "nix-copy-closure --from '{}' failed:\n{}",
+      "nix-copy-closure --from '{}' failed (exit status: {:?})",
       host,
-      capture.stderr_str()
+      exit_status
     );
   }
 
@@ -1056,17 +1059,20 @@ pub fn copy_to_remote(
   spinner.set_message(format!("Copying closure to remote host '{host}'..."));
   spinner.enable_steady_tick(Duration::from_millis(80));
 
-  let capture = cmd
-    .capture()
+  let (exit_status, _stdout, _stderr) = exec_with_streaming(cmd, false)
     .wrap_err("Failed to copy closure to remote host")?;
 
   // We finish and *clear*, because the log line needs to come next. If we try
   // to make the spinner change the text, we cannot reliably match the `info!`
   // or `error!` style.
   spinner.finish_and_clear();
-  if !capture.exit_status.success() {
+  if !exit_status.success() {
     error!("Failed to copy closure to remote host '{host}'");
-    bail!("nix copy --to '{}' failed:\n{}", host, capture.stderr_str());
+    bail!(
+      "nix copy --to '{}' failed (exit status: {:?})",
+      host,
+      exit_status
+    );
   }
   info!("Copied closure to remote host '{host}'");
 
@@ -1099,16 +1105,15 @@ fn copy_closure_between_remotes(
 
   debug!(?cmd, "nix copy between remotes");
 
-  let capture = cmd
-    .capture()
+  let (exit_status, _stdout, _stderr) = exec_with_streaming(cmd, false)
     .wrap_err("Failed to copy closure between remote hosts")?;
 
-  if !capture.exit_status.success() {
+  if !exit_status.success() {
     bail!(
-      "nix copy from '{}' to '{}' failed:\n{}",
+      "nix copy from '{}' to '{}' failed (exit status: {:?})",
       from_host,
       to_host,
-      capture.stderr_str()
+      exit_status
     );
   }