From 7d21426c56306917ebaf16671a75215672d79755 Mon Sep 17 00:00:00 2001 From: Fredrik Medley Date: Sun, 12 Jul 2015 08:53:19 +0200 Subject: Release the pool slot held by an edge whether it succeeds or fails When an edge finishes building, it should be release from its pool. Make sure that this also is the case when an edge fails to build. The bug can be shown with a pool has size N, then `ninja -k N+1` will still stop after N failing commands for that pool, even if there are many more jobs to be done for that pool: pool mypool depth = 1 rule bad_rule command = false pool = mypool build a : bad_rule build b : bad_rule Current behaviour: $ ninja -k 0 [1/2] false FAILED: false ninja: build stopped: cannot make progress due to previous errors. Expected behaviour: $ ninja -k 0 [1/2] false FAILED: false [2/2] false FAILED: false ninja: build stopped: cannot make progress due to previous errors. Signed-off-by: Fredrik Medley --- src/build.cc | 25 ++++++++++------ src/build.h | 5 ++-- src/build_test.cc | 90 ++++++++++++++++++++++++++++++++++++++++--------------- 3 files changed, 84 insertions(+), 36 deletions(-) diff --git a/src/build.cc b/src/build.cc index 2df5c1a..dae1e18 100644 --- a/src/build.cc +++ b/src/build.cc @@ -382,20 +382,25 @@ void Plan::ScheduleWork(Edge* edge) { } } -void Plan::EdgeFinished(Edge* edge) { +void Plan::EdgeFinished(Edge* edge, bool success) { map::iterator e = want_.find(edge); assert(e != want_.end()); bool directly_wanted = e->second; - if (directly_wanted) - --wanted_edges_; - want_.erase(e); - edge->outputs_ready_ = true; // See if this job frees up any delayed jobs. if (directly_wanted) edge->pool()->EdgeFinished(*edge); edge->pool()->RetrieveReadyEdges(&ready_); + // The rest of this function only applies to successful commands. + if (!success) + return; + + if (directly_wanted) + --wanted_edges_; + want_.erase(e); + edge->outputs_ready_ = true; + // Check off any nodes we were waiting for with this edge. for (vector::iterator o = edge->outputs_.begin(); o != edge->outputs_.end(); ++o) { @@ -418,7 +423,7 @@ void Plan::NodeFinished(Node* node) { } else { // We do not need to build this edge, but we might need to build one of // its dependents. - EdgeFinished(*oe); + EdgeFinished(*oe, true); } } } @@ -651,7 +656,7 @@ bool Builder::Build(string* err) { } if (edge->is_phony()) { - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); } else { ++pending_commands; } @@ -770,8 +775,10 @@ bool Builder::FinishCommand(CommandRunner::Result* result, string* err) { &start_time, &end_time); // The rest of this function only applies to successful commands. - if (!result->success()) + if (!result->success()) { + plan_.EdgeFinished(edge, false); return true; + } // Restat the edge outputs, if necessary. TimeStamp restat_mtime = 0; @@ -820,7 +827,7 @@ bool Builder::FinishCommand(CommandRunner::Result* result, string* err) { } } - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); // Delete any left over response file. string rspfile = edge->GetUnescapedRspfile(); diff --git a/src/build.h b/src/build.h index 8106faa..3a7183f 100644 --- a/src/build.h +++ b/src/build.h @@ -56,9 +56,8 @@ struct Plan { /// Dumps the current state of the plan. void Dump(); - /// Mark an edge as done building. Used internally and by - /// tests. - void EdgeFinished(Edge* edge); + /// Mark an edge as done building (whether it succeeded or failed). + void EdgeFinished(Edge* edge, bool success); /// Clean the given node during the build. /// Return false on error. diff --git a/src/build_test.cc b/src/build_test.cc index 7c6060d..261268a 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -68,14 +68,14 @@ TEST_F(PlanTest, Basic) { ASSERT_FALSE(plan_.FindWork()); - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_TRUE(edge); ASSERT_EQ("mid", edge->inputs_[0]->path()); ASSERT_EQ("out", edge->outputs_[0]->path()); - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); ASSERT_FALSE(plan_.more_to_do()); edge = plan_.FindWork(); @@ -99,11 +99,11 @@ TEST_F(PlanTest, DoubleOutputDirect) { Edge* edge; edge = plan_.FindWork(); ASSERT_TRUE(edge); // cat in - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_TRUE(edge); // cat mid1 mid2 - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_FALSE(edge); // done @@ -129,19 +129,19 @@ TEST_F(PlanTest, DoubleOutputIndirect) { Edge* edge; edge = plan_.FindWork(); ASSERT_TRUE(edge); // cat in - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_TRUE(edge); // cat a1 - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_TRUE(edge); // cat a2 - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_TRUE(edge); // cat b1 b2 - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_FALSE(edge); // done @@ -167,19 +167,19 @@ TEST_F(PlanTest, DoubleDependent) { Edge* edge; edge = plan_.FindWork(); ASSERT_TRUE(edge); // cat in - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_TRUE(edge); // cat mid - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_TRUE(edge); // cat mid - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_TRUE(edge); // cat a1 a2 - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_FALSE(edge); // done @@ -257,7 +257,7 @@ void PlanTest::TestPoolWithDepthOne(const char* test_case) { // This will be false since poolcat is serialized ASSERT_FALSE(plan_.FindWork()); - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_TRUE(edge); @@ -266,7 +266,7 @@ void PlanTest::TestPoolWithDepthOne(const char* test_case) { ASSERT_FALSE(plan_.FindWork()); - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); ASSERT_FALSE(plan_.more_to_do()); edge = plan_.FindWork(); @@ -342,7 +342,7 @@ TEST_F(PlanTest, PoolsWithDepthTwo) { ASSERT_EQ("outb3", edge->outputs_[0]->path()); // finish out1 - plan_.EdgeFinished(edges.front()); + plan_.EdgeFinished(edges.front(), true); edges.pop_front(); // out3 should be available @@ -353,19 +353,19 @@ TEST_F(PlanTest, PoolsWithDepthTwo) { ASSERT_FALSE(plan_.FindWork()); - plan_.EdgeFinished(out3); + plan_.EdgeFinished(out3, true); ASSERT_FALSE(plan_.FindWork()); for (deque::iterator it = edges.begin(); it != edges.end(); ++it) { - plan_.EdgeFinished(*it); + plan_.EdgeFinished(*it, true); } Edge* last = plan_.FindWork(); ASSERT_TRUE(last); ASSERT_EQ("allTheThings", last->outputs_[0]->path()); - plan_.EdgeFinished(last); + plan_.EdgeFinished(last, true); ASSERT_FALSE(plan_.more_to_do()); ASSERT_FALSE(plan_.FindWork()); @@ -407,7 +407,7 @@ TEST_F(PlanTest, PoolWithRedundantEdges) { edge = initial_edges[1]; // Foo first ASSERT_EQ("foo.cpp", edge->outputs_[0]->path()); - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_TRUE(edge); @@ -415,11 +415,11 @@ TEST_F(PlanTest, PoolWithRedundantEdges) { ASSERT_EQ("foo.cpp", edge->inputs_[0]->path()); ASSERT_EQ("foo.cpp", edge->inputs_[1]->path()); ASSERT_EQ("foo.cpp.obj", edge->outputs_[0]->path()); - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = initial_edges[0]; // Now for bar ASSERT_EQ("bar.cpp", edge->outputs_[0]->path()); - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_TRUE(edge); @@ -427,7 +427,7 @@ TEST_F(PlanTest, PoolWithRedundantEdges) { ASSERT_EQ("bar.cpp", edge->inputs_[0]->path()); ASSERT_EQ("bar.cpp", edge->inputs_[1]->path()); ASSERT_EQ("bar.cpp.obj", edge->outputs_[0]->path()); - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_TRUE(edge); @@ -435,20 +435,62 @@ TEST_F(PlanTest, PoolWithRedundantEdges) { ASSERT_EQ("foo.cpp.obj", edge->inputs_[0]->path()); ASSERT_EQ("bar.cpp.obj", edge->inputs_[1]->path()); ASSERT_EQ("libfoo.a", edge->outputs_[0]->path()); - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_TRUE(edge); ASSERT_FALSE(plan_.FindWork()); ASSERT_EQ("libfoo.a", edge->inputs_[0]->path()); ASSERT_EQ("all", edge->outputs_[0]->path()); - plan_.EdgeFinished(edge); + plan_.EdgeFinished(edge, true); edge = plan_.FindWork(); ASSERT_FALSE(edge); ASSERT_FALSE(plan_.more_to_do()); } +TEST_F(PlanTest, PoolWithFailingEdge) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, + "pool foobar\n" + " depth = 1\n" + "rule poolcat\n" + " command = cat $in > $out\n" + " pool = foobar\n" + "build out1: poolcat in\n" + "build out2: poolcat in\n")); + GetNode("out1")->MarkDirty(); + GetNode("out2")->MarkDirty(); + string err; + EXPECT_TRUE(plan_.AddTarget(GetNode("out1"), &err)); + ASSERT_EQ("", err); + EXPECT_TRUE(plan_.AddTarget(GetNode("out2"), &err)); + ASSERT_EQ("", err); + ASSERT_TRUE(plan_.more_to_do()); + + Edge* edge = plan_.FindWork(); + ASSERT_TRUE(edge); + ASSERT_EQ("in", edge->inputs_[0]->path()); + ASSERT_EQ("out1", edge->outputs_[0]->path()); + + // This will be false since poolcat is serialized + ASSERT_FALSE(plan_.FindWork()); + + plan_.EdgeFinished(edge, false); + + edge = plan_.FindWork(); + ASSERT_TRUE(edge); + ASSERT_EQ("in", edge->inputs_[0]->path()); + ASSERT_EQ("out2", edge->outputs_[0]->path()); + + ASSERT_FALSE(plan_.FindWork()); + + plan_.EdgeFinished(edge, false); + + ASSERT_TRUE(plan_.more_to_do()); // Jobs have failed + edge = plan_.FindWork(); + ASSERT_EQ(0, edge); +} + /// Fake implementation of CommandRunner, useful for tests. struct FakeCommandRunner : public CommandRunner { explicit FakeCommandRunner(VirtualFileSystem* fs) : -- cgit v0.12 From ff6eedb968ddd6f0f9ba252a31e5a77967edddb5 Mon Sep 17 00:00:00 2001 From: David Emett Date: Sun, 20 Sep 2015 14:43:01 +0100 Subject: Add another test case covering pool release on edge failure With this build file: pool failpool depth = 1 rule fail command = fail pool = failpool build out1: fail build out2: fail build out3: fail build final: phony out1 out2 out3 Running `ninja -k 0` should run out1..3 sequentially before failing, but until recently we would fail after just running out1. Add a test covering this case. --- src/build_test.cc | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/build_test.cc b/src/build_test.cc index 261268a..90d500c 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -1153,6 +1153,30 @@ TEST_F(BuildTest, SwallowFailuresLimit) { ASSERT_EQ("cannot make progress due to previous errors", err); } +TEST_F(BuildTest, SwallowFailuresPool) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, +"pool failpool\n" +" depth = 1\n" +"rule fail\n" +" command = fail\n" +" pool = failpool\n" +"build out1: fail\n" +"build out2: fail\n" +"build out3: fail\n" +"build final: cat out1 out2 out3\n")); + + // Swallow ten failures; we should stop before building final. + config_.failures_allowed = 11; + + string err; + EXPECT_TRUE(builder_.AddTarget("final", &err)); + ASSERT_EQ("", err); + + EXPECT_FALSE(builder_.Build(&err)); + ASSERT_EQ(3u, command_runner_.commands_ran_.size()); + ASSERT_EQ("cannot make progress due to previous errors", err); +} + TEST_F(BuildTest, PoolEdgesReadyButNotWanted) { fs_.Create("x", ""); -- cgit v0.12