From 858d886661a7ea1f10b9e496066b915a3ad5f1c7 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Thu, 25 Jul 2024 17:25:13 -0400
Subject: [PATCH] utils: Add a little CommandRunExt helper trait

When I added the `Task` stuff it wasn't with the idea
that we'd use it for *all* subprocess invocations.

I think in some cases we really just do want a `Command` instance
without the extra wrappering.

Add an implementation (there are many of variants of this out
there in lots of Rust codebases)

This makes sense to use instead of `Task` where we're using
`.quiet()` so that we don't print the description, and especially
where we don't expect the task to fail usually, so it's OK
if the error message is odd.

Signed-off-by: Colin Walters <walters@verbum.org>
---
 lib/src/install.rs |  7 +++----
 lib/src/utils.rs   | 25 +++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/lib/src/install.rs b/lib/src/install.rs
index cd7e068dc..a3029af6a 100644
--- a/lib/src/install.rs
+++ b/lib/src/install.rs
@@ -46,7 +46,7 @@ use crate::mount::Filesystem;
 use crate::spec::ImageReference;
 use crate::store::Storage;
 use crate::task::Task;
-use crate::utils::sigpolicy_from_opts;
+use crate::utils::{sigpolicy_from_opts, CommandRunExt};
 
 /// The default "stateroot" or "osname"; see https://github.com/ostreedev/ostree/issues/2794
 const STATEROOT_DEFAULT: &str = "default";
@@ -584,10 +584,9 @@ async fn initialize_ostree_root(state: &State, root_setup: &RootSetup) -> Result
         ("sysroot.bootprefix", "true"),
         ("sysroot.readonly", "true"),
     ] {
-        Task::new("Configuring ostree repo", "ostree")
+        Command::new("ostree")
             .args(["config", "--repo", "ostree/repo", "set", k, v])
-            .cwd(rootfs_dir)?
-            .quiet()
+            .cwd_dir(rootfs_dir.try_clone()?)
             .run()?;
     }
     Task::new("Initializing sysroot", "ostree")
diff --git a/lib/src/utils.rs b/lib/src/utils.rs
index e704ca48c..1b1d0cd9d 100644
--- a/lib/src/utils.rs
+++ b/lib/src/utils.rs
@@ -10,6 +10,25 @@ use ostree::glib;
 use ostree_ext::container::SignatureSource;
 use ostree_ext::ostree;
 
+/// Helpers intended for [`std::process::Command`].
+pub(crate) trait CommandRunExt {
+    fn run(&mut self) -> Result<()>;
+}
+
+impl CommandRunExt for Command {
+    /// Synchronously execute the child, and return an error if the child exited unsuccessfully.
+    fn run(&mut self) -> Result<()> {
+        let st = self.status()?;
+        if !st.success() {
+            // 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.
+            anyhow::bail!(format!("Subprocess failed: {st:?}"))
+        }
+        Ok(())
+    }
+}
+
 /// Try to look for keys injected by e.g. rpm-ostree requesting machine-local
 /// changes; if any are present, return `true`.
 pub(crate) fn origin_has_rpmostree_stuff(kf: &glib::KeyFile) -> bool {
@@ -190,3 +209,9 @@ fn test_sigpolicy_from_opts() {
         SignatureSource::ContainerPolicyAllowInsecure
     );
 }
+
+#[test]
+fn command_run_ext() {
+    Command::new("true").run().unwrap();
+    assert!(Command::new("false").run().is_err());
+}