From e1ca8b51f259422e6f12d4d564f25a67e80025f2 Mon Sep 17 00:00:00 2001
From: Evan Martin <martine@danga.com>
Date: Tue, 30 Nov 2010 11:01:39 -0800
Subject: [PATCH] refactor process exit

---
 build.cc           | 17 ++++++++++++-----
 build.h            |  2 +-
 build_test.cc      |  5 +++--
 ninja.cc           |  2 +-
 subprocess.cc      | 13 ++++++++-----
 subprocess.h       |  4 +---
 subprocess_test.cc |  2 ++
 7 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/build.cc b/build.cc
index abc35cc..a7d2418 100644
--- a/build.cc
+++ b/build.cc
@@ -89,7 +89,7 @@ struct RealCommandRunner : public CommandRunner {
   virtual bool CanRunMore();
   virtual bool StartCommand(Edge* edge);
   virtual void WaitForCommands();
-  virtual Edge* NextFinishedCommand();
+  virtual Edge* NextFinishedCommand(bool* success);
 
   SubprocessSet subprocs_;
   map<Subprocess*, Edge*> subproc_to_edge_;
@@ -118,11 +118,13 @@ void RealCommandRunner::WaitForCommands() {
   }
 }
 
-Edge* RealCommandRunner::NextFinishedCommand() {
+Edge* RealCommandRunner::NextFinishedCommand(bool* success) {
   Subprocess* subproc = subprocs_.NextFinished();
   if (!subproc)
     return NULL;
 
+  *success = subproc->Finish();
+
   if (!subproc->stdout_.buf_.empty())
     printf("%s\n", subproc->stdout_.buf_.c_str());
   if (!subproc->stderr_.buf_.empty())
@@ -133,7 +135,6 @@ Edge* RealCommandRunner::NextFinishedCommand() {
   subproc_to_edge_.erase(i);
 
   delete subproc;
-  // XXX extract exit code, etc.
   return edge;
 }
 
@@ -183,10 +184,16 @@ bool Builder::Build(string* err) {
         return false;
     }
 
-    if (Edge* edge = command_runner_->NextFinishedCommand())
+    bool success;
+    if (Edge* edge = command_runner_->NextFinishedCommand(&success)) {
+      if (!success) {
+        *err = "subcommand failed";
+        return false;
+      }
       FinishEdge(edge);
-    else
+    } else {
       command_runner_->WaitForCommands();
+    }
   }
 
   return true;
diff --git a/build.h b/build.h
index 6143922..cf17ccc 100644
--- a/build.h
+++ b/build.h
@@ -49,7 +49,7 @@ struct CommandRunner {
   virtual bool CanRunMore() = 0;
   virtual bool StartCommand(Edge* edge) = 0;
   virtual void WaitForCommands() = 0;
-  virtual Edge* NextFinishedCommand() = 0;
+  virtual Edge* NextFinishedCommand(bool* success) = 0;
 };
 
 struct Builder {
diff --git a/build_test.cc b/build_test.cc
index 7c8ae92..b2a8746 100644
--- a/build_test.cc
+++ b/build_test.cc
@@ -166,7 +166,7 @@ struct BuildTest : public StateTestWithBuiltinRules,
   virtual bool CanRunMore();
   virtual bool StartCommand(Edge* edge);
   virtual void WaitForCommands();
-  virtual Edge* NextFinishedCommand();
+  virtual Edge* NextFinishedCommand(bool* success);
 
   // DiskInterface
   virtual int Stat(const string& path) {
@@ -234,8 +234,9 @@ bool BuildTest::StartCommand(Edge* edge) {
 void BuildTest::WaitForCommands() {
 }
 
-Edge* BuildTest::NextFinishedCommand() {
+Edge* BuildTest::NextFinishedCommand(bool* success) {
   if (Edge* edge = last_command_) {
+    *success = true;
     last_command_ = NULL;
     return edge;
   }
diff --git a/ninja.cc b/ninja.cc
index 7d0381f..f39c77f 100644
--- a/ninja.cc
+++ b/ninja.cc
@@ -96,7 +96,7 @@ int main(int argc, char** argv) {
 
   bool success = builder.Build(&err);
   if (!err.empty()) {
-    printf("%s\n", err.c_str());
+    printf("build stopped: %s.\n", err.c_str());
   }
 
   return success ? 0 : 1;
diff --git a/subprocess.cc b/subprocess.cc
index 70e5a65..0d47523 100644
--- a/subprocess.cc
+++ b/subprocess.cc
@@ -18,8 +18,11 @@ Subprocess::Stream::~Stream() {
     close(fd_);
 }
 
-Subprocess::Subprocess() : pid_(-1), status_(-1) {}
+Subprocess::Subprocess() : pid_(-1) {}
 Subprocess::~Subprocess() {
+  // Reap child if forgotten.
+  if (pid_ != -1)
+    Finish();
 }
 
 bool Subprocess::Start(const string& command) {
@@ -73,12 +76,13 @@ void Subprocess::OnFDReady(int fd) {
 
 bool Subprocess::Finish() {
   assert(pid_ != -1);
-  if (waitpid(pid_, &status_, 0) < 0)
+  int status;
+  if (waitpid(pid_, &status, 0) < 0)
     Fatal("waitpid(%d): %s", pid_, strerror(errno));
   pid_ = -1;
 
-  if (WIFEXITED(status_)) {
-    int exit = WEXITSTATUS(status_);
+  if (WIFEXITED(status)) {
+    int exit = WEXITSTATUS(status);
     if (exit == 0)
       return true;
   }
@@ -127,7 +131,6 @@ void SubprocessSet::DoWork() {
       if (fds[i].revents) {
         subproc->OnFDReady(fds[i].fd);
         if (subproc->done()) {
-          subproc->Finish();
           finished_.push(subproc);
           std::remove(running_.begin(), running_.end(), subproc);
           running_.resize(running_.size() - 1);
diff --git a/subprocess.h b/subprocess.h
index 22eb132..2c91f4a 100644
--- a/subprocess.h
+++ b/subprocess.h
@@ -12,8 +12,7 @@ struct Subprocess {
   ~Subprocess();
   bool Start(const string& command);
   void OnFDReady(int fd);
-  // Returns true on successful process exit;
-  // can also examine status_.
+  // Returns true on successful process exit.
   bool Finish();
 
   bool done() const {
@@ -28,7 +27,6 @@ struct Subprocess {
   };
   Stream stdout_, stderr_;
   pid_t pid_;
-  int status_;
 };
 
 // SubprocessSet runs a poll() loop around a set of Subprocesses.
diff --git a/subprocess_test.cc b/subprocess_test.cc
index e1f16bd..51d4a11 100644
--- a/subprocess_test.cc
+++ b/subprocess_test.cc
@@ -35,6 +35,7 @@ TEST(SubprocessSet, Single) {
   while (!ls->done()) {
     subprocs.DoWork();
   }
+  ASSERT_TRUE(ls->Finish());
   ASSERT_NE("", ls->stdout_.buf_);
 
   ASSERT_EQ(1, subprocs.finished_.size());
@@ -72,6 +73,7 @@ TEST(SubprocessSet, Multi) {
   ASSERT_EQ(3, subprocs.finished_.size());
 
   for (int i = 0; i < 3; ++i) {
+    ASSERT_TRUE(processes[i]->Finish());
     ASSERT_NE("", processes[i]->stdout_.buf_);
     ASSERT_EQ("", processes[i]->stderr_.buf_);
     delete processes[i];
-- 
2.7.4