Merge pull request #5212 from cgwalters/cmdutils-run

tree-wide: Import and use cmdutils from bootc

Cargo.toml

@@ -94,6 +94,9 @@ xmlrpc = "0.15.1"
 termcolor = "1.4.1"
 shlex = "1.3.0"
 
+[dev-dependencies]
+similar-asserts = "1.6.0"
+
 [build-dependencies]
 anyhow = "1.0"
 system-deps = "7.0"

rust/src/bwrap.rs

@@ -2,6 +2,7 @@
 
 // SPDX-License-Identifier: Apache-2.0 OR MIT
 
+use crate::cmdutils::CommandRunExt;
 use crate::cxxrsutil::*;
 use crate::ffi::BubblewrapMutability;
 use crate::normalization;
@@ -99,10 +100,7 @@ impl RoFilesMount {
                 .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e))
             });
         }
-        let status = c.status()?;
-        if !status.success() {
-            return Err(anyhow::anyhow!("{}", status));
-        }
+        c.run()?;
         Ok(Self {
             tempdir: Some(tempdir),
         })

rust/src/cmdutils.rs

@@ -0,0 +1,209 @@
+//! Helpers intended for [`std::process::Command`] and related structures.
+//! This is copied from bootc, please edit there and re-sync!
+
+// SPDX-License-Identifier: Apache-2.0 OR MIT
+
+use std::{
+    io::{Read, Seek},
+    os::unix::process::CommandExt,
+    process::Command,
+};
+
+use anyhow::{Context, Result};
+
+#[allow(dead_code)]
+/// Helpers intended for [`std::process::Command`].
+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<()>;
+
+    /// 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
+    /// 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, parsing its stdout as JSON. This uses `run` 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>;
+}
+
+/// Helpers intended for [`std::process::ExitStatus`].
+pub trait ExitStatusExt {
+    /// If the exit status signals it was not successful, return an error.
+    /// 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<()>;
+}
+
+/// Parse the last chunk (e.g. 1024 bytes) from the provided file,
+/// ensure it's UTF-8, and return that value. This function is infallible;
+/// if the file cannot be read for some reason, a copy of a static string
+/// is returned.
+fn last_utf8_content_from_file(mut f: std::fs::File) -> String {
+    // u16 since we truncate to just the trailing bytes here
+    // to avoid pathological error messages
+    const MAX_STDERR_BYTES: u16 = 1024;
+    let size = f
+        .metadata()
+        .map_err(|e| {
+            tracing::warn!("failed to fstat: {e}");
+        })
+        .map(|m| m.len().try_into().unwrap_or(u16::MAX))
+        .unwrap_or(0);
+    let size = size.min(MAX_STDERR_BYTES);
+    let seek_offset = -(size as i32);
+    let mut stderr_buf = Vec::with_capacity(size.into());
+    // We should never fail to seek()+read() really, but let's be conservative
+    let r = match f
+        .seek(std::io::SeekFrom::End(seek_offset.into()))
+        .and_then(|_| f.read_to_end(&mut stderr_buf))
+    {
+        Ok(_) => String::from_utf8_lossy(&stderr_buf),
+        Err(e) => {
+            tracing::warn!("failed seek+read: {e}");
+            "<failed to read stderr>".into()
+        }
+    };
+    (&*r).to_owned()
+}
+
+impl ExitStatusExt for std::process::ExitStatus {
+    fn check_status(&mut self, stderr: std::fs::File) -> Result<()> {
+        let stderr_buf = last_utf8_content_from_file(stderr);
+        if self.success() {
+            return Ok(());
+        }
+        anyhow::bail!(format!("Subprocess failed: {self:?}\n{stderr_buf}"))
+    }
+}
+
+impl CommandRunExt for Command {
+    /// Synchronously execute the child, and return an error if the child exited unsuccessfully.
+    fn run(&mut self) -> Result<()> {
+        let stderr = tempfile::tempfile()?;
+        self.stderr(stderr.try_clone()?);
+        tracing::trace!("exec: {self:?}");
+        self.status()?.check_status(stderr)
+    }
+
+    #[allow(unsafe_code)]
+    fn lifecycle_bind(&mut self) -> &mut Self {
+        // SAFETY: This API is safe to call in a forked child.
+        unsafe {
+            self.pre_exec(|| {
+                rustix::process::set_parent_process_death_signal(Some(
+                    rustix::process::Signal::Term,
+                ))
+                .map_err(Into::into)
+            })
+        }
+    }
+
+    /// Output a debug-level log message with this command.
+    fn log_debug(&mut self) -> &mut Self {
+        // We unconditionally log at trace level, so avoid double logging
+        if !tracing::enabled!(tracing::Level::TRACE) {
+            tracing::debug!("exec: {self:?}");
+        }
+        self
+    }
+
+    fn run_get_output(&mut self) -> Result<Box<dyn std::io::BufRead>> {
+        let mut stdout = tempfile::tempfile()?;
+        self.stdout(stdout.try_clone()?);
+        self.run()?;
+        stdout.seek(std::io::SeekFrom::Start(0)).context("seek")?;
+        Ok(Box::new(std::io::BufReader::new(stdout)))
+    }
+
+    /// Synchronously execute the child, and parse its stdout as JSON.
+    fn run_and_parse_json<T: serde::de::DeserializeOwned>(&mut self) -> Result<T> {
+        let output = self.run_get_output()?;
+        serde_json::from_reader(output).map_err(Into::into)
+    }
+}
+
+/// Helpers intended for [`tokio::process::Command`].
+#[allow(async_fn_in_trait)]
+#[allow(dead_code)]
+pub trait AsyncCommandRunExt {
+    /// Asynchronously execute the child, and return an error if the child exited unsuccessfully.
+    async fn run(&mut self) -> Result<()>;
+}
+
+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)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn command_run_ext() {
+        // The basics
+        Command::new("true").run().unwrap();
+        assert!(Command::new("false").run().is_err());
+
+        // Verify we capture stderr
+        let e = Command::new("/bin/sh")
+            .args(["-c", "echo expected-this-oops-message 1>&2; exit 1"])
+            .run()
+            .err()
+            .unwrap();
+        similar_asserts::assert_eq!(
+            e.to_string(),
+            "Subprocess failed: ExitStatus(unix_wait_status(256))\nexpected-this-oops-message\n"
+        );
+
+        // Ignoring invalid UTF-8
+        let e = Command::new("/bin/sh")
+            .args([
+                "-c",
+                r"echo -e 'expected\xf5\x80\x80\x80\x80-foo\xc0bar\xc0\xc0' 1>&2; exit 1",
+            ])
+            .run()
+            .err()
+            .unwrap();
+        similar_asserts::assert_eq!(
+            e.to_string(),
+            "Subprocess failed: ExitStatus(unix_wait_status(256))\nexpected�����-foo�bar��\n"
+        );
+    }
+
+    #[test]
+    fn command_run_ext_json() {
+        #[derive(serde::Deserialize)]
+        struct Foo {
+            a: String,
+            b: u32,
+        }
+        let v: Foo = Command::new("echo")
+            .arg(r##"{"a": "somevalue", "b": 42}"##)
+            .run_and_parse_json()
+            .unwrap();
+        assert_eq!(v.a, "somevalue");
+        assert_eq!(v.b, 42);
+    }
+
+    #[tokio::test]
+    async fn async_command_run_ext() {
+        use tokio::process::Command as AsyncCommand;
+        let mut success = AsyncCommand::new("true");
+        let mut fail = AsyncCommand::new("false");
+        // Run these in parallel just because we can
+        let (success, fail) = tokio::join!(success.run(), fail.run(),);
+        success.unwrap();
+        assert!(fail.is_err());
+    }
+}

rust/src/compose.rs

@@ -16,6 +16,7 @@ use ostree_ext::containers_image_proxy;
 use ostree_ext::keyfileext::{map_keyfile_optional, KeyFileExt};
 use ostree_ext::{oci_spec, ostree};
 
+use crate::cmdutils::CommandRunExt;
 use crate::cxxrsutil::{CxxResult, FFIGObjectWrapper};
 
 #[derive(clap::ValueEnum, Clone, Debug)]
@@ -278,7 +279,7 @@ pub(crate) fn compose_image(args: Vec<String>) -> CxxResult<()> {
         compose_args_extra.extend(["--source-root", path.as_str()]);
     }
 
-    let s = self_command()
+    self_command()
         .args([
             "compose",
             "tree",
@@ -296,10 +297,7 @@ pub(crate) fn compose_image(args: Vec<String>) -> CxxResult<()> {
         .args(opt.offline.then_some("--cache-only"))
         .args(compose_args_extra)
         .arg(opt.manifest.as_str())
-        .status()?;
-    if !s.success() {
-        return Err(anyhow::anyhow!("compose tree failed: {:?}", s).into());
-    }
+        .run()?;
 
     if !changed_path.exists() {
         return Ok(());
@@ -335,7 +333,7 @@ pub(crate) fn compose_image(args: Vec<String>) -> CxxResult<()> {
         })
         .transpose()?;
 
-    let s = self_command()
+    self_command()
         .args(["compose", "container-encapsulate"])
         .args(label_args)
         .args(previous_arg)
@@ -346,10 +344,7 @@ pub(crate) fn compose_image(args: Vec<String>) -> CxxResult<()> {
             commitid.as_str(),
             tempdest.as_deref().unwrap_or(target_imgref.as_str()),
         ])
-        .status()?;
-    if !s.success() {
-        return Err(anyhow::anyhow!("container-encapsulate failed: {:?}", s).into());
-    }
+        .run()?;
 
     if let Some(tempdest) = tempdest {
         let mut c = Command::new("skopeo");

rust/src/composepost.rs

@@ -7,6 +7,7 @@
 
 use crate::bwrap::Bubblewrap;
 use crate::capstdext::dirbuilder_from_mode;
+use crate::cmdutils::CommandRunExt;
 use crate::cxxrsutil::*;
 use crate::ffi::BubblewrapMutability;
 use crate::ffiutil::ffi_dirfd;
@@ -1302,14 +1303,11 @@ fn rewrite_rpmdb_for_target_inner(rootfs_dfd: &Dir, normalize: bool) -> Result<(
 
     let dbpath_arg = format!("--dbpath=/proc/self/cwd/{}", RPMOSTREE_RPMDB_LOCATION);
     // Fork rpmdb from the *host* rootfs to read the rpmdb back into memory
-    let r = std::process::Command::new("rpmdb")
+    std::process::Command::new("rpmdb")
         .args([dbpath_arg.as_str(), "--exportdb"])
         .current_dir(format!("/proc/self/fd/{}", rootfs_dfd.as_raw_fd()))
         .stdout(Stdio::from(dbfd.try_clone()?))
-        .status()?;
-    if !r.success() {
-        return Err(anyhow!("Failed to execute rpmdb --exportdb: {:?}", r));
-    }
+        .run()?;
 
     // Clear out the db on disk
     rootfs_dfd.remove_all_optional(RPMOSTREE_RPMDB_LOCATION)?;