Add STDOUT to lockfile generation error message (#1428)

Previously only the STDERR was printed whenever lockfile generation
encountered a non-zero exit code from the package manager, however some
package managers like `dotnet` print their errors to STDOUT.

This patch reworks the output in these error cases to print both STDERR
and STDOUT and formats things a little nicer to make it easy to keep the
two apart.

CHANGELOG.md

@@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 - Sandboxed processes sticking around after CLI is killed with a signal
 - Lockfiles with local versions breaking the pip parser
+- Lockfile generation not emitting errors for tools writing them to STDOUT
 
 ## 6.3.0 - 2024-04-18
 

lockfile_generator/src/cargo.rs

@@ -18,8 +18,7 @@ impl Generator for Cargo {
 
         // Ensure command was successful.
         if !output.status.success() {
-            let stderr = String::from_utf8_lossy(&output.stderr);
-            return Err(Error::NonZeroExit(output.status.code(), stderr.into()));
+            return Err(Error::NonZeroExit(output));
         }
 
         // Parse metadata output.

lockfile_generator/src/lib.rs

@@ -1,6 +1,8 @@
 use std::ffi::OsString;
+use std::fmt::{self, Display, Formatter};
+use std::os::unix::process::ExitStatusExt;
 use std::path::{Path, PathBuf, StripPrefixError};
-use std::process::{Command, Stdio};
+use std::process::{Command, Output, Stdio};
 use std::string::FromUtf8Error;
 use std::{fs, io};
 
@@ -76,9 +78,7 @@ pub trait Generator {
 
         // Ensure generation was successful.
         if !output.status.success() {
-            let code = output.status.code();
-            let stderr = String::from_utf8_lossy(&output.stderr);
-            return Err(Error::NonZeroExit(code, stderr.into()));
+            return Err(Error::NonZeroExit(output));
         }
 
         // Ensure lockfile was created.
@@ -135,28 +135,63 @@ pub type Result<T> = std::result::Result<T, Error>;
 /// Lockfile generation error.
 #[derive(thiserror::Error, Debug)]
 pub enum Error {
-    #[error("{0}")]
     Anyhow(#[from] anyhow::Error),
-    #[error("invalid manifest path: {0:?}")]
     InvalidManifest(PathBuf),
-    #[error("utf8 parsing error")]
     InvalidUtf8(#[from] FromUtf8Error),
-    #[error("I/O error")]
     Io(#[from] io::Error),
-    #[error("json parsing error")]
     Json(#[from] JsonError),
-    #[error("package manager quit unexpectedly (code: {0:?}):\n\n{1}")]
-    NonZeroExit(Option<i32>, String),
-    #[error("unsupported pip report version {1:?}, expected {0:?}")]
+    NonZeroExit(Output),
     PipReportVersionMismatch(&'static str, String),
-    #[error("failed to spawn command {0}: Is {1} installed?")]
     ProcessCreation(String, String, io::Error),
-    #[error("could not strip path prefix")]
     StripPrefix(#[from] StripPrefixError),
-    #[error("unexpected output for {0:?}: {1}")]
-    UnexpectedOutput(&'static str, String),
-    #[error("unsupported {0:?} version {2:?}, expected {1:?}")]
     UnsupportedCommandVersion(&'static str, &'static str, String),
-    #[error("no lockfile was generated")]
     NoLockfileGenerated,
 }
+
+impl Display for Error {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        match self {
+            Self::Anyhow(err) => write!(f, "{err}"),
+            Self::InvalidManifest(path) => write!(f, "invalid manifest path: {path:?}"),
+            Self::InvalidUtf8(_) => write!(f, "utf8 parsing error"),
+            Self::Io(_) => write!(f, "I/O error"),
+            Self::Json(_) => write!(f, "json parsing error"),
+            Self::NonZeroExit(output) => {
+                write!(
+                    f,
+                    "package manager quit unexpectedly (code: {:?}, signal: {:?})",
+                    output.status.code(),
+                    output.status.signal()
+                )?;
+
+                if !output.stderr.is_empty() {
+                    write!(f, "\n    STDERR:")?;
+                    for line in String::from_utf8_lossy(&output.stderr).lines() {
+                        write!(f, "\n        {line}")?;
+                    }
+                }
+
+                if !output.stdout.is_empty() {
+                    write!(f, "\n    STDOUT:")?;
+                    for line in String::from_utf8_lossy(&output.stdout).lines() {
+                        write!(f, "\n        {line}")?;
+                    }
+                }
+
+                Ok(())
+            },
+            Self::PipReportVersionMismatch(expected, version) => {
+                write!(f, "unsupported pip report version {version:?}, expected {expected:?}")
+            },
+            Self::ProcessCreation(command, tool_name, _) => {
+                write!(f, "failed to spawn command {command}: Is {tool_name} installed?")
+            },
+            Self::StripPrefix(_) => write!(f, "could not strip path prefix"),
+            Self::UnsupportedCommandVersion(command, expected_version, version) => write!(
+                f,
+                "unsupported {command:?} version {version:?}, expected {expected_version:?}"
+            ),
+            Self::NoLockfileGenerated => write!(f, "no lockfile was generated"),
+        }
+    }
+}

lockfile_generator/src/pip.rs

@@ -79,9 +79,7 @@ impl Generator for Pip {
 
         // Ensure generation was successful.
         if !output.status.success() {
-            let code = output.status.code();
-            let stderr = String::from_utf8_lossy(&output.stderr);
-            return Err(Error::NonZeroExit(code, stderr.into()));
+            return Err(Error::NonZeroExit(output));
         }
 
         // Parse pip install report STDOUT.
@@ -122,9 +120,7 @@ fn check_pip_version(project_path: &Path) -> Result<()> {
 
     // Report errors with `pip` version check.
     if !version_output.status.success() {
-        let exit_code = version_output.status.code();
-        let version_stderr = String::from_utf8_lossy(&version_output.stderr).trim().to_owned();
-        return Err(Error::NonZeroExit(exit_code, version_stderr));
+        return Err(Error::NonZeroExit(version_output));
     }
 
     let version_stdout = String::from_utf8(version_output.stdout)?.trim().to_owned();

lockfile_generator/src/yarn.rs

@@ -51,7 +51,6 @@ fn yarn_version(manifest_path: &Path) -> Result<String> {
     if output.status.success() {
         Ok(String::from_utf8_lossy(&output.stdout).into())
     } else {
-        let stderr = String::from_utf8_lossy(&output.stderr);
-        Err(Error::NonZeroExit(output.status.code(), stderr.into()))
+        Err(Error::NonZeroExit(output))
     }
 }