chore(vdev): Revamp capture_output() to propagate errors (#16981)

* chore(vdev): Revamp capture_output() to propagate errors

* chore(vdev): Revamp capture_output() to propagate errors

* check fmt

vdev/src/app.rs

@@ -1,6 +1,8 @@
 use std::ffi::{OsStr, OsString};
 pub use std::process::Command;
-use std::{borrow::Cow, env, path::PathBuf, process::ExitStatus, time::Duration};
+use std::{
+    borrow::Cow, env, io::Read, path::PathBuf, process::ExitStatus, process::Stdio, time::Duration,
+};
 
 use anyhow::{bail, Context as _, Result};
 use indicatif::{ProgressBar, ProgressStyle};
@@ -66,7 +68,7 @@ pub fn version() -> Result<String> {
 pub trait CommandExt {
     fn script(script: &str) -> Self;
     fn in_repo(&mut self) -> &mut Self;
-    fn capture_output(&mut self) -> Result<String>;
+    fn check_output(&mut self) -> Result<String>;
     fn check_run(&mut self) -> Result<()>;
     fn run(&mut self) -> Result<ExitStatus>;
     fn wait(&mut self, message: impl Into<Cow<'static, str>>) -> Result<()>;
@@ -95,9 +97,35 @@ impl CommandExt for Command {
     }
 
     /// Run the command and capture its output.
-    fn capture_output(&mut self) -> Result<String> {
+    fn check_output(&mut self) -> Result<String> {
+        // Set up the command's stdout to be piped, so we can capture it
         self.pre_exec();
-        Ok(String::from_utf8(self.output()?.stdout)?)
+        self.stdout(Stdio::piped());
+
+        // Spawn the process
+        let mut child = self.spawn()?;
+
+        // Read the output from child.stdout into a buffer
+        let mut buffer = Vec::new();
+        child.stdout.take().unwrap().read_to_end(&mut buffer)?;
+
+        // Catch the exit code
+        let status = child.wait()?;
+        // There are commands that might fail with stdout, but we probably do not
+        // want to capture
+        // If the exit code is non-zero, return an error with the command, exit code, and stderr output
+        if !status.success() {
+            let stdout = String::from_utf8_lossy(&buffer);
+            bail!(
+                "Command: {:?}\nfailed with exit code: {}\n\noutput:\n{}",
+                self,
+                status.code().unwrap(),
+                stdout
+            );
+        }
+
+        // If the command exits successfully, return the output as a string
+        Ok(String::from_utf8(buffer)?)
     }
 
     /// Run the command and catch its exit code.

vdev/src/commands/crate_versions.rs

@@ -28,7 +28,7 @@ impl Cli {
         for line in Command::new("cargo")
             .arg("tree")
             .features(&self.feature)
-            .capture_output()?
+            .check_output()?
             .lines()
         {
             if let Some(captures) = re_crate.captures(line) {

vdev/src/git.rs

@@ -5,7 +5,7 @@ use anyhow::Result;
 use crate::app::CommandExt as _;
 
 pub fn current_branch() -> Result<String> {
-    let output = capture_output(&["rev-parse", "--abbrev-ref", "HEAD"])?;
+    let output = check_output(&["rev-parse", "--abbrev-ref", "HEAD"])?;
     Ok(output.trim_end().to_string())
 }
 
@@ -19,17 +19,17 @@ pub fn checkout_or_create_branch(branch_name: &str) -> Result<()> {
 }
 
 pub fn merge_branch(branch_name: &str) -> Result<()> {
-    let _output = capture_output(&["merge", "--ff", branch_name])?;
+    let _output = check_output(&["merge", "--ff", branch_name])?;
     Ok(())
 }
 
 pub fn tag_version(version: &str) -> Result<()> {
-    let _output = capture_output(&["tag", "--annotate", version, "--message", version])?;
+    let _output = check_output(&["tag", "--annotate", version, "--message", version])?;
     Ok(())
 }
 
 pub fn push_branch(branch_name: &str) -> Result<()> {
-    let _output = capture_output(&["push", "origin", branch_name])?;
+    let _output = check_output(&["push", "origin", branch_name])?;
     Ok(())
 }
 
@@ -39,7 +39,7 @@ pub fn changed_files() -> Result<Vec<String>> {
     // Committed e.g.:
     // A   relative/path/to/file.added
     // M   relative/path/to/file.modified
-    let output = capture_output(&["diff", "--name-status", "origin/master..."])?;
+    let output = check_output(&["diff", "--name-status", "origin/master..."])?;
     for line in output.lines() {
         if !is_warning_line(line) {
             if let Some((_, path)) = line.split_once('\t') {
@@ -49,15 +49,15 @@ pub fn changed_files() -> Result<Vec<String>> {
     }
 
     // Tracked
-    let output = capture_output(&["diff", "--name-only", "HEAD"])?;
+    let output = check_output(&["diff", "--name-only", "HEAD"])?;
     for line in output.lines() {
         if !is_warning_line(line) {
             files.insert(line.to_string());
         }
     }
 
     // Untracked
-    let output = capture_output(&["ls-files", "--others", "--exclude-standard"])?;
+    let output = check_output(&["ls-files", "--others", "--exclude-standard"])?;
     for line in output.lines() {
         files.insert(line.to_string());
     }
@@ -69,14 +69,14 @@ pub fn changed_files() -> Result<Vec<String>> {
 }
 
 pub fn list_files() -> Result<Vec<String>> {
-    Ok(capture_output(&["ls-files"])?
+    Ok(check_output(&["ls-files"])?
         .lines()
         .map(str::to_owned)
         .collect())
 }
 
 pub fn get_git_sha() -> Result<String> {
-    capture_output(&["rev-parse", "--short", "HEAD"]).map(|output| output.trim_end().to_string())
+    check_output(&["rev-parse", "--short", "HEAD"]).map(|output| output.trim_end().to_string())
 }
 
 // Get a list of files that have been modified, as a vector of strings
@@ -88,26 +88,26 @@ pub fn get_modified_files() -> Result<Vec<String>> {
         "--others",
         "--exclude-standard",
     ];
-    Ok(capture_output(&args)?.lines().map(str::to_owned).collect())
+    Ok(check_output(&args)?.lines().map(str::to_owned).collect())
 }
 
 pub fn branch_exists(branch_name: &str) -> Result<bool> {
-    let output = capture_output(&["rev-parse", "--verify", branch_name])?;
+    let output = check_output(&["rev-parse", "--verify", branch_name])?;
     Ok(!output.is_empty())
 }
 
 pub fn checkout_branch(branch_name: &str) -> Result<()> {
-    let _output = capture_output(&["checkout", branch_name])?;
+    let _output = check_output(&["checkout", branch_name])?;
     Ok(())
 }
 
 pub fn create_branch(branch_name: &str) -> Result<()> {
-    let _output = capture_output(&["checkout", "-b", branch_name])?;
+    let _output = check_output(&["checkout", "-b", branch_name])?;
     Ok(())
 }
 
-fn capture_output(args: &[&str]) -> Result<String> {
-    Command::new("git").in_repo().args(args).capture_output()
+fn check_output(args: &[&str]) -> Result<String> {
+    Command::new("git").in_repo().args(args).check_output()
 }
 
 fn is_warning_line(line: &str) -> bool {

vdev/src/testing/runner.rs

@@ -104,7 +104,7 @@ pub trait ContainerTestRunner: TestRunner {
         let mut command = dockercmd(["ps", "-a", "--format", "{{.Names}} {{.State}}"]);
         let container_name = self.container_name();
 
-        for line in command.capture_output()?.lines() {
+        for line in command.check_output()?.lines() {
             if let Some((name, state)) = line.split_once(' ') {
                 if name == container_name {
                     return Ok(if state == "created" {
@@ -157,7 +157,7 @@ pub trait ContainerTestRunner: TestRunner {
         volumes.insert(VOLUME_TARGET);
         volumes.insert(VOLUME_CARGO_GIT);
         volumes.insert(VOLUME_CARGO_REGISTRY);
-        for volume in command.capture_output()?.lines() {
+        for volume in command.check_output()?.lines() {
             volumes.take(volume);
         }
 
@@ -324,7 +324,7 @@ impl IntegrationTestRunner {
             let mut command = dockercmd(["network", "ls", "--format", "{{.Name}}"]);
 
             if command
-                .capture_output()?
+                .check_output()?
                 .lines()
                 .any(|network| network == network_name)
             {