If a command fails, wait for all running commands to terminate before we do

Previously, if a command fails, the fate of the other child processes
running in parallel was inadequately controlled.  On POSIX platforms,
the processes were orphaned.  Normally they would run to completion,
but were liable to being killed by a SIGPIPE.  On Windows, the child
processes would terminate with the parent.  The cleanup-on-interrupt
patch caused the SubprocessSet and Builder destructors to clean
up after themselves by killing any running child processes and
deleting their output files, making the behaviour more predictable
and consistent across platforms.

If the build is interrupted by the user, this is correct behaviour.
But in the case where the build is stopped by a failed command, this
would be inconsistent with user expectations.  In the latter case,
we now let any remaining child processes run to completion before
leaving the main loop in Builder::Build.

4 files changed, 22 insertions, 21 deletions

diff --git a/src/build.cc b/src/build.cc
index 4c15de9..265c805 100644
--- a/src/build.cc
+++ b/src/build.cc
@@ -538,7 +538,7 @@ bool Builder::Build(string* err) {
 
   status_->PlanHasTotalEdges(plan_.command_edge_count());
   int pending_commands = 0;
-  int failures_allowed = config_.swallow_failures;
+  int failures_allowed = config_.failures_allowed;
 
   // This main loop runs the entire build process.
   // It is structured like this:
@@ -549,7 +549,7 @@ bool Builder::Build(string* err) {
   // an error.
   while (plan_.more_to_do()) {
     // See if we can start any more commands.
-    if (command_runner_->CanRunMore()) {
+    if (failures_allowed && command_runner_->CanRunMore()) {
       if (Edge* edge = plan_.FindWork()) {
         if (!StartEdge(edge, err)) {
           status_->BuildFinished();
@@ -576,13 +576,8 @@ bool Builder::Build(string* err) {
         --pending_commands;
         FinishEdge(edge, success, output);
         if (!success) {
-          if (failures_allowed-- == 0) {
-            if (config_.swallow_failures != 0)
-              *err = "subcommands failed";
-            else
-              *err = "subcommand failed";
-            return false;
-          }
+          if (failures_allowed)
+            failures_allowed--;
         }
 
         // We made some progress; start the main loop over.
@@ -605,13 +600,17 @@ bool Builder::Build(string* err) {
 
     // If we get here, we cannot make any more progress.
     status_->BuildFinished();
-    if (failures_allowed < config_.swallow_failures) {
+    if (failures_allowed == 0) {
+      if (config_.failures_allowed > 1)
+        *err = "subcommands failed";
+      else
+        *err = "subcommand failed";
+    } else if (failures_allowed < config_.failures_allowed)
       *err = "cannot make progress due to previous errors";
-      return false;
-    } else {
+    else 
       *err = "stuck [this is a bug]";
-      return false;
-    }
+
+    return false;
   }
 
   status_->BuildFinished();
diff --git a/src/build.h b/src/build.h
index c75bde1..778d59d 100644
--- a/src/build.h
+++ b/src/build.h
@@ -98,7 +98,7 @@ struct CommandRunner {
 /// Options (e.g. verbosity, parallelism) passed to a build.
 struct BuildConfig {
   BuildConfig() : verbosity(NORMAL), dry_run(false), parallelism(1),
-                  swallow_failures(0) {}
+                  failures_allowed(1) {}
 
   enum Verbosity {
     NORMAL,
@@ -108,7 +108,7 @@ struct BuildConfig {
   Verbosity verbosity;
   bool dry_run;
   int parallelism;
-  int swallow_failures;
+  int failures_allowed;
 };
 
 /// Builder wraps the build process: starting commands, updating status.
diff --git a/src/build_test.cc b/src/build_test.cc
index de5ddc1..7c3a729 100644
--- a/src/build_test.cc
+++ b/src/build_test.cc
@@ -661,7 +661,7 @@ TEST_F(BuildTest, SwallowFailures) {
 "build all: phony out1 out2 out3\n"));
 
   // Swallow two failures, die on the third.
-  config_.swallow_failures = 2;
+  config_.failures_allowed = 3;
 
   string err;
   EXPECT_TRUE(builder_.AddTarget("all", &err));
@@ -682,7 +682,7 @@ TEST_F(BuildTest, SwallowFailuresLimit) {
 "build final: cat out1 out2 out3\n"));
 
   // Swallow ten failures; we should stop before building final.
-  config_.swallow_failures = 10;
+  config_.failures_allowed = 11;
 
   string err;
   EXPECT_TRUE(builder_.AddTarget("final", &err));
diff --git a/src/ninja.cc b/src/ninja.cc
index 1822e54..6cc2276 100644
--- a/src/ninja.cc
+++ b/src/ninja.cc
@@ -13,6 +13,7 @@
 // limitations under the License.
 
 #include <errno.h>
+#include <limits.h>
 #include <stdio.h>
 #include <string.h>
 #include <sys/stat.h>
@@ -613,9 +614,10 @@ int main(int argc, char** argv) {
         if (*end != 0)
           Fatal("-k parameter not numeric; did you mean -k0?");
 
-        // We want to go until N jobs fail, which means we should ignore
-        // the first N-1 that fail and then stop.
-        globals.config.swallow_failures = value - 1;
+        // We want to go until N jobs fail, which means we should allow
+        // N failures and then stop.  For N <= 0, INT_MAX is close enough
+        // to infinite for most sane builds.
+        globals.config.failures_allowed = value > 0 ? value : INT_MAX;
         break;
       }
       case 'n':