From d8d5a4555b44d0a67925a17a44397564cc7ff8be Mon Sep 17 00:00:00 2001
From: Adrian Sampson <asampson@cs.cornell.edu>
Date: Mon, 27 May 2024 13:19:39 -0400
Subject: [PATCH] Check Ninja exit status and report errors (#2058)

Concretely, this improves behavior in a few ways:
* better error messages when Ninja fails
* don't attempt to print stuff to stdout when Ninja fails
* clean up .fud2 temp directory even when Ninja fails
---
 fud2/fud-core/src/run.rs | 43 ++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/fud2/fud-core/src/run.rs b/fud2/fud-core/src/run.rs
index 6f8b2ce551..2d6efc0f11 100644
--- a/fud2/fud-core/src/run.rs
+++ b/fud2/fud-core/src/run.rs
@@ -4,35 +4,44 @@ use crate::utils::relative_path;
 use camino::{Utf8Path, Utf8PathBuf};
 use std::collections::{HashMap, HashSet};
 use std::io::Write;
-use std::process::Command;
+use std::process::{Command, ExitStatus};
 
-/// An error that arises while emitting the Ninja file.
+/// An error that arises while emitting the Ninja file or executing Ninja.
 #[derive(Debug)]
-pub enum EmitError {
+pub enum RunError {
+    /// An IO error when writing the Ninja file.
     Io(std::io::Error),
+
+    /// A required configuration key was missing.
     MissingConfig(String),
+
+    /// The Ninja process exited with nonzero status.
+    NinjaFailed(ExitStatus),
 }
 
-impl From<std::io::Error> for EmitError {
+impl From<std::io::Error> for RunError {
     fn from(e: std::io::Error) -> Self {
         Self::Io(e)
     }
 }
 
-impl std::fmt::Display for EmitError {
+impl std::fmt::Display for RunError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match &self {
-            EmitError::Io(e) => write!(f, "{}", e),
-            EmitError::MissingConfig(s) => {
+            RunError::Io(e) => write!(f, "{}", e),
+            RunError::MissingConfig(s) => {
                 write!(f, "missing required config key: {}", s)
             }
+            RunError::NinjaFailed(c) => {
+                write!(f, "ninja exited with {}", c)
+            }
         }
     }
 }
 
-impl std::error::Error for EmitError {}
+impl std::error::Error for RunError {}
 
-pub type EmitResult = std::result::Result<(), EmitError>;
+pub type EmitResult = std::result::Result<(), RunError>;
 
 /// Code to emit a Ninja `build` command.
 pub trait EmitBuild {
@@ -214,10 +223,10 @@ impl<'a> Run<'a> {
             // When we're printing to stdout, suppress Ninja's output by default.
             cmd.stdout(std::process::Stdio::null());
         }
-        cmd.status()?;
+        let status = cmd.status()?;
 
-        // Emit stdout.
-        if self.plan.stdout {
+        // Emit stdout, only when Ninja succeeded.
+        if status.success() && self.plan.stdout {
             let stdout_file =
                 std::fs::File::open(self.plan.workdir.join(self.plan.end()))?;
             std::io::copy(
@@ -231,7 +240,11 @@ impl<'a> Run<'a> {
             std::fs::remove_dir_all(dir)?;
         }
 
-        Ok(())
+        if status.success() {
+            Ok(())
+        } else {
+            Err(RunError::NinjaFailed(status))
+        }
     }
 
     pub fn emit<T: Write + 'a>(&self, out: T) -> EmitResult {
@@ -300,10 +313,10 @@ impl<'a> Emitter<'a> {
     }
 
     /// Fetch a configuration value, or panic if it's missing.
-    pub fn config_val(&self, key: &str) -> Result<String, EmitError> {
+    pub fn config_val(&self, key: &str) -> Result<String, RunError> {
         self.config_data
             .extract_inner::<String>(key)
-            .map_err(|_| EmitError::MissingConfig(key.to_string()))
+            .map_err(|_| RunError::MissingConfig(key.to_string()))
     }
 
     /// Fetch a configuration value, using a default if it's missing.