Merge pull request #1459 from cgwalters/command-run-cleanups

cgwalters · web-flow · commit 47aad7265767 · 2025-07-30T08:34:04.000-04:00
command: Split up run() method
diff --git a/crates/blockdev/src/blockdev.rs b/crates/blockdev/src/blockdev.rs
@@ -322,7 +322,9 @@ impl LoopbackDevice {
             let _ = cleanup_handle.child.kill();
         }
 
-        Command::new("losetup").args(["-d", dev.as_str()]).run()
+        Command::new("losetup")
+            .args(["-d", dev.as_str()])
+            .run_capture_stderr()
     }
 
     /// Consume this device, unmounting it.
diff --git a/crates/lib/src/image.rs b/crates/lib/src/image.rs
@@ -178,5 +178,5 @@ pub(crate) async fn imgcmd_entrypoint(
     let mut cmd = storage.new_image_cmd()?;
     cmd.arg(arg);
     cmd.args(args);
-    cmd.run()
+    cmd.run_capture_stderr()
 }
diff --git a/crates/lib/src/imgstorage.rs b/crates/lib/src/imgstorage.rs
@@ -138,7 +138,7 @@ pub(crate) fn ensure_floating_c_storage_initialized() {
     if let Err(e) = Command::new("podman")
         .args(["system", "info"])
         .stdout(Stdio::null())
-        .run()
+        .run_capture_stderr()
     {
         // Out of conservatism we don't make this operation fatal right now.
         // If something went wrong, then we'll probably fail on a later operation
@@ -220,7 +220,7 @@ impl Storage {
             new_podman_cmd_in(&storage_root, &run)?
                 .stdout(Stdio::null())
                 .arg("images")
-                .run()
+                .run_capture_stderr()
                 .context("Initializing images")?;
             Self::ensure_labeled(&storage_root, sepolicy)?;
             drop(storage_root);
@@ -273,7 +273,7 @@ impl Storage {
         AsyncCommand::from(cmd)
             .status()
             .await?
-            .check_status(stderr)?;
+            .check_status_with_stderr(stderr)?;
         // Spawn a helper thread to avoid blocking the main thread
         // parsing JSON.
         tokio::task::spawn_blocking(move || -> Result<_> {
diff --git a/crates/lib/src/install.rs b/crates/lib/src/install.rs
@@ -670,7 +670,7 @@ async fn initialize_ostree_root(
         Command::new("ostree")
             .args(["config", "--repo", "ostree/repo", "set", k, v])
             .cwd_dir(rootfs_dir.try_clone()?)
-            .run()?;
+            .run_capture_stderr()?;
     }
 
     let sysroot = {
diff --git a/crates/lib/src/install/completion.rs b/crates/lib/src/install/completion.rs
@@ -152,7 +152,7 @@ fn bind_from_host(
             .args(["-m", "-t", "1", "--", "mount", "--bind"])
             .arg(src)
             .arg(&target)
-            .run()?;
+            .run_capture_stderr()?;
         Ok(())
     }
 
diff --git a/crates/lib/src/install/osbuild.rs b/crates/lib/src/install/osbuild.rs
@@ -36,7 +36,7 @@ fn adjust_etc_containers(tempdir: &Dir) -> Result<()> {
         .args(["-t", "overlay", "overlay", "-o", opts.as_str()])
         .arg(etc_containers)
         .cwd_dir(tempdir.try_clone()?)
-        .run()?;
+        .run_capture_stderr()?;
     Ok(())
 }
 
@@ -59,7 +59,7 @@ fn propagate_run_osbuild_containers(root: &Dir) -> Result<()> {
         .arg("--rbind")
         .args([osbuild_run_containers, relative_storage])
         .cwd_dir(root.try_clone()?)
-        .run()?;
+        .run_capture_stderr()?;
     Ok(())
 }
 
diff --git a/crates/lib/src/utils.rs b/crates/lib/src/utils.rs
@@ -119,7 +119,7 @@ pub(crate) fn spawn_editor(tmpf: &tempfile::NamedTempFile) -> Result<()> {
         .args(editor_args)
         .arg(tmpf.path())
         .lifecycle_bind()
-        .run()
+        .run_inherited()
         .with_context(|| format!("Invoking editor {editor} failed"))
 }
 
diff --git a/crates/mount/src/mount.rs b/crates/mount/src/mount.rs
@@ -117,7 +117,7 @@ pub fn is_source_mounted(path: &str, mounted_fs: &Filesystem) -> bool {
 pub fn mount(dev: &str, target: &Utf8Path) -> Result<()> {
     Command::new("mount")
         .args([dev, target.as_str()])
-        .run_with_cmd_context()
+        .run_inherited_with_cmd_context()
 }
 
 /// If the fsid of the passed path matches the fsid of the same path rooted
diff --git a/crates/ostree-ext/src/fixture.rs b/crates/ostree-ext/src/fixture.rs
@@ -769,7 +769,7 @@ impl Fixture {
                 let _ = Command::new("ostree")
                     .arg(format!("--repo={}", self.path.join("src/repo")))
                     .args(["ls", "-X", "-C", "-R", commit.as_str()])
-                    .run();
+                    .run_capture_stderr();
             });
         }
         assert_eq!(CONTENTS_CHECKSUM_V0, content_checksum.as_str());
diff --git a/crates/system-reinstall-bootc/src/main.rs b/crates/system-reinstall-bootc/src/main.rs
@@ -55,12 +55,12 @@ fn run() -> Result<()> {
     prompt::temporary_developer_protection_prompt()?;
 
     reinstall_podman_command
-        .run_with_cmd_context()
+        .run_inherited_with_cmd_context()
         .context("running reinstall command")?;
 
     prompt::reboot()?;
 
-    std::process::Command::new("reboot").run()?;
+    std::process::Command::new("reboot").run_capture_stderr()?;
 
     Ok(())
 }
diff --git a/crates/system-reinstall-bootc/src/podman.rs b/crates/system-reinstall-bootc/src/podman.rs
@@ -118,7 +118,7 @@ pub(crate) fn pull_if_not_present(image: &str) -> Result<()> {
         println!("Image {} is not present locally, pulling it now.", image);
         println!();
         pull_image_command(image)
-            .run_with_cmd_context()
+            .run_inherited_with_cmd_context()
             .context(format!("pulling image {}", image))?;
     }
 
@@ -150,7 +150,7 @@ pub(crate) fn ensure_podman_installed() -> Result<()> {
     );
 
     Command::new(podman_install_script_path())
-        .run_with_cmd_context()
+        .run_inherited_with_cmd_context()
         .context("installing podman")?;
 
     // Make sure the installation was actually successful
diff --git a/crates/utils/src/command.rs b/crates/utils/src/command.rs
@@ -14,24 +14,53 @@ pub trait CommandRunExt {
     /// Log (at debug level) the full child commandline.
     fn log_debug(&mut self) -> &mut Self;
 
-    /// Execute the child process.
-    fn run(&mut self) -> Result<()>;
+    /// Execute the child process and wait for it to exit.
+    ///
+    /// # Streams
+    ///
+    /// - stdin, stdout, stderr: All inherited
+    ///
+    /// # Errors
+    ///
+    /// An non-successful exit status will result in an error.
+    fn run_inherited(&mut self) -> Result<()>;
 
-    /// Execute the child process. In case of failure, include the command and its arguments in the
-    /// error context
-    fn run_with_cmd_context(&mut self) -> Result<()>;
+    /// Execute the child process and wait for it to exit.
+    ///
+    /// # Streams
+    ///
+    /// - stdin, stdout: Inherited
+    /// - stderr: captured and included in error
+    ///
+    /// # Errors
+    ///
+    /// An non-successful exit status will result in an error.
+    fn run_capture_stderr(&mut self) -> Result<()>;
+
+    /// Execute the child process and wait for it to exit; the
+    /// complete argument list will be included in the error.
+    ///
+    /// # Streams
+    ///
+    /// - stdin, stdout, stderr: All nherited
+    ///
+    /// # Errors
+    ///
+    /// An non-successful exit status will result in an error.
+    fn run_inherited_with_cmd_context(&mut self) -> Result<()>;
 
     /// Ensure the child does not outlive the parent.
     fn lifecycle_bind(&mut self) -> &mut Self;
 
-    /// Execute the child process and capture its output. This uses `run` internally
+    /// Execute the child process and capture its output. This uses `run_capture_stderr` internally
     /// and will return an error if the child process exits abnormally.
     fn run_get_output(&mut self) -> Result<Box<dyn std::io::BufRead>>;
 
     /// Execute the child process and capture its output as a string.
+    /// This uses `run_capture_stderr` internally.
     fn run_get_string(&mut self) -> Result<String>;
 
-    /// Execute the child process, parsing its stdout as JSON. This uses `run` internally
+    /// Execute the child process, parsing its stdout as JSON. This uses `run_capture_stderr` internally
     /// and will return an error if the child process exits abnormally.
     fn run_and_parse_json<T: serde::de::DeserializeOwned>(&mut self) -> Result<T>;
 
@@ -45,7 +74,13 @@ pub trait ExitStatusExt {
     /// Note that we intentionally *don't* include the command string
     /// in the output; we leave it to the caller to add that if they want,
     /// as it may be verbose.
-    fn check_status(&mut self, stderr: std::fs::File) -> Result<()>;
+    fn check_status(&mut self) -> Result<()>;
+
+    /// If the exit status signals it was not successful, return an error;
+    /// this also includes the contents of `stderr`.
+    ///
+    /// Otherwise this is the same as [`Self::check_status`].
+    fn check_status_with_stderr(&mut self, stderr: std::fs::File) -> Result<()>;
 }
 
 /// Parse the last chunk (e.g. 1024 bytes) from the provided file,
@@ -81,7 +116,13 @@ fn last_utf8_content_from_file(mut f: std::fs::File) -> String {
 }
 
 impl ExitStatusExt for std::process::ExitStatus {
-    fn check_status(&mut self, stderr: std::fs::File) -> Result<()> {
+    fn check_status(&mut self) -> Result<()> {
+        if self.success() {
+            return Ok(());
+        }
+        anyhow::bail!(format!("Subprocess failed: {self:?}"))
+    }
+    fn check_status_with_stderr(&mut self, stderr: std::fs::File) -> Result<()> {
         let stderr_buf = last_utf8_content_from_file(stderr);
         if self.success() {
             return Ok(());
@@ -91,12 +132,17 @@ impl ExitStatusExt for std::process::ExitStatus {
 }
 
 impl CommandRunExt for Command {
+    fn run_inherited(&mut self) -> Result<()> {
+        tracing::trace!("exec: {self:?}");
+        self.status()?.check_status()
+    }
+
     /// Synchronously execute the child, and return an error if the child exited unsuccessfully.
-    fn run(&mut self) -> Result<()> {
+    fn run_capture_stderr(&mut self) -> Result<()> {
         let stderr = tempfile::tempfile()?;
         self.stderr(stderr.try_clone()?);
         tracing::trace!("exec: {self:?}");
-        self.status()?.check_status(stderr)
+        self.status()?.check_status_with_stderr(stderr)
     }
 
     #[allow(unsafe_code)]
@@ -124,7 +170,7 @@ impl CommandRunExt for Command {
     fn run_get_output(&mut self) -> Result<Box<dyn std::io::BufRead>> {
         let mut stdout = tempfile::tempfile()?;
         self.stdout(stdout.try_clone()?);
-        self.run()?;
+        self.run_capture_stderr()?;
         stdout.seek(std::io::SeekFrom::Start(0)).context("seek")?;
         Ok(Box::new(std::io::BufReader::new(stdout)))
     }
@@ -142,7 +188,7 @@ impl CommandRunExt for Command {
         serde_json::from_reader(output).map_err(Into::into)
     }
 
-    fn run_with_cmd_context(&mut self) -> Result<()> {
+    fn run_inherited_with_cmd_context(&mut self) -> Result<()> {
         self.status()?
             .success()
             .then_some(())
@@ -176,7 +222,7 @@ impl AsyncCommandRunExt for tokio::process::Command {
     async fn run(&mut self) -> Result<()> {
         let stderr = tempfile::tempfile()?;
         self.stderr(stderr.try_clone()?);
-        self.status().await?.check_status(stderr)
+        self.status().await?.check_status_with_stderr(stderr)
     }
 }
 
@@ -185,15 +231,36 @@ mod tests {
     use super::*;
 
     #[test]
-    fn command_run_ext() {
+    fn command_run_inherited() {
+        // Test successful command
+        Command::new("true").run_inherited().unwrap();
+
+        // Test failed command
+        assert!(Command::new("false").run_inherited().is_err());
+
+        // Test that stderr is not captured (just check error format)
+        let e = Command::new("/bin/sh")
+            .args(["-c", "echo should-not-be-captured 1>&2; exit 1"])
+            .run_inherited()
+            .err()
+            .unwrap();
+        // Should not contain the stderr message since it's inherited
+        assert_eq!(
+            e.to_string(),
+            "Subprocess failed: ExitStatus(unix_wait_status(256))"
+        );
+    }
+
+    #[test]
+    fn command_run_capture_stderr() {
         // The basics
-        Command::new("true").run().unwrap();
-        assert!(Command::new("false").run().is_err());
+        Command::new("true").run_capture_stderr().unwrap();
+        assert!(Command::new("false").run_capture_stderr().is_err());
 
         // Verify we capture stderr
         let e = Command::new("/bin/sh")
             .args(["-c", "echo expected-this-oops-message 1>&2; exit 1"])
-            .run()
+            .run_capture_stderr()
             .err()
             .unwrap();
         similar_asserts::assert_eq!(
@@ -207,7 +274,7 @@ mod tests {
                 "-c",
                 r"echo -e 'expected\xf5\x80\x80\x80\x80-foo\xc0bar\xc0\xc0' 1>&2; exit 1",
             ])
-            .run()
+            .run_capture_stderr()
             .err()
             .unwrap();
         similar_asserts::assert_eq!(
@@ -216,6 +283,49 @@ mod tests {
         );
     }
 
+    #[test]
+    fn exit_status_check_status() {
+        use std::process::Command;
+
+        // Test successful exit status
+        let mut success_status = Command::new("true").status().unwrap();
+        success_status.check_status().unwrap();
+
+        // Test failed exit status
+        let mut fail_status = Command::new("false").status().unwrap();
+        let e = fail_status.check_status().err().unwrap();
+        assert_eq!(
+            e.to_string(),
+            "Subprocess failed: ExitStatus(unix_wait_status(256))"
+        );
+    }
+
+    #[test]
+    fn exit_status_check_status_with_stderr() {
+        use std::io::Write;
+        use std::process::Command;
+
+        // Test successful exit status
+        let mut success_status = Command::new("true").status().unwrap();
+        let temp_stderr = tempfile::tempfile().unwrap();
+        success_status
+            .check_status_with_stderr(temp_stderr)
+            .unwrap();
+
+        // Test failed exit status with stderr content
+        let mut fail_status = Command::new("false").status().unwrap();
+        let mut temp_stderr = tempfile::tempfile().unwrap();
+        write!(temp_stderr, "test error message").unwrap();
+        let e = fail_status
+            .check_status_with_stderr(temp_stderr)
+            .err()
+            .unwrap();
+        assert!(e
+            .to_string()
+            .contains("Subprocess failed: ExitStatus(unix_wait_status(256))"));
+        assert!(e.to_string().contains("test error message"));
+    }
+
     #[test]
     fn command_run_ext_json() {
         #[derive(serde::Deserialize)]

Original file line number	Diff line number	Diff line change
`@@ -322,7 +322,9 @@ impl LoopbackDevice {`
`322`	`322`	`let _ = cleanup_handle.child.kill();`
`323`	`323`	`}`
`324`	`324`
`325`		`- Command::new("losetup").args(["-d", dev.as_str()]).run()`
	`325`	`+ Command::new("losetup")`
	`326`	`+ .args(["-d", dev.as_str()])`
	`327`	`+ .run_capture_stderr()`
`326`	`328`	`}`
`327`	`329`
`328`	`330`	`/// Consume this device, unmounting it.`
Original file line number	Diff line number	Diff line change
`@@ -178,5 +178,5 @@ pub(crate) async fn imgcmd_entrypoint(`
`178`	`178`	`let mut cmd = storage.new_image_cmd()?;`
`179`	`179`	`cmd.arg(arg);`
`180`	`180`	`cmd.args(args);`
`181`		`- cmd.run()`
	`181`	`+ cmd.run_capture_stderr()`
`182`	`182`	`}`
Original file line number	Diff line number	Diff line change
`@@ -670,7 +670,7 @@ async fn initialize_ostree_root(`
`670`	`670`	`Command::new("ostree")`
`671`	`671`	`.args(["config", "--repo", "ostree/repo", "set", k, v])`
`672`	`672`	`.cwd_dir(rootfs_dir.try_clone()?)`
`673`		`- .run()?;`
	`673`	`+ .run_capture_stderr()?;`
`674`	`674`	`}`
`675`	`675`
`676`	`676`	`let sysroot = {`
Original file line number	Diff line number	Diff line change
`@@ -152,7 +152,7 @@ fn bind_from_host(`
`152`	`152`	`.args(["-m", "-t", "1", "--", "mount", "--bind"])`
`153`	`153`	`.arg(src)`
`154`	`154`	`.arg(&target)`
`155`		`- .run()?;`
	`155`	`+ .run_capture_stderr()?;`
`156`	`156`	`Ok(())`
`157`	`157`	`}`
`158`	`158`
Original file line number	Diff line number	Diff line change
`@@ -119,7 +119,7 @@ pub(crate) fn spawn_editor(tmpf: &tempfile::NamedTempFile) -> Result<()> {`
`119`	`119`	`.args(editor_args)`
`120`	`120`	`.arg(tmpf.path())`
`121`	`121`	`.lifecycle_bind()`
`122`		`- .run()`
	`122`	`+ .run_inherited()`
`123`	`123`	`.with_context(\|\| format!("Invoking editor {editor} failed"))`
`124`	`124`	`}`
`125`	`125`
Original file line number	Diff line number	Diff line change
`@@ -117,7 +117,7 @@ pub fn is_source_mounted(path: &str, mounted_fs: &Filesystem) -> bool {`
`117`	`117`	`pub fn mount(dev: &str, target: &Utf8Path) -> Result<()> {`
`118`	`118`	`Command::new("mount")`
`119`	`119`	`.args([dev, target.as_str()])`
`120`		`- .run_with_cmd_context()`
	`120`	`+ .run_inherited_with_cmd_context()`
`121`	`121`	`}`
`122`	`122`
`123`	`123`	`/// If the fsid of the passed path matches the fsid of the same path rooted`
Original file line number	Diff line number	Diff line change
`@@ -769,7 +769,7 @@ impl Fixture {`
`769`	`769`	`let _ = Command::new("ostree")`
`770`	`770`	`.arg(format!("--repo={}", self.path.join("src/repo")))`
`771`	`771`	`.args(["ls", "-X", "-C", "-R", commit.as_str()])`
`772`		`- .run();`
	`772`	`+ .run_capture_stderr();`
`773`	`773`	`});`
`774`	`774`	`}`
`775`	`775`	`assert_eq!(CONTENTS_CHECKSUM_V0, content_checksum.as_str());`