Merge pull request #2352 from digit-google/remove-phony-in-edges
Remove phony edges for nodes created by a dependency loader.
diff --git a/src/build.cc b/src/build.cc
index 76ff93a..6903e45 100644
--- a/src/build.cc
+++ b/src/build.cc
@@ -95,15 +95,20 @@
bool Plan::AddSubTarget(const Node* node, const Node* dependent, string* err,
set<Edge*>* dyndep_walk) {
Edge* edge = node->in_edge();
- if (!edge) { // Leaf node.
- if (node->dirty()) {
- string referenced;
- if (dependent)
- referenced = ", needed by '" + dependent->path() + "',";
- *err = "'" + node->path() + "'" + referenced + " missing "
- "and no known rule to make it";
- }
- return false;
+ if (!edge) {
+ // Leaf node, this can be either a regular input from the manifest
+ // (e.g. a source file), or an implicit input from a depfile or dyndep
+ // file. In the first case, a dirty flag means the file is missing,
+ // and the build should stop. In the second, do not do anything here
+ // since there is no producing edge to add to the plan.
+ if (node->dirty() && !node->generated_by_dep_loader()) {
+ string referenced;
+ if (dependent)
+ referenced = ", needed by '" + dependent->path() + "',";
+ *err = "'" + node->path() + "'" + referenced +
+ " missing and no known rule to make it";
+ }
+ return false;
}
if (edge->outputs_ready())
diff --git a/src/build_test.cc b/src/build_test.cc
index d32ad3e..5ed8245 100644
--- a/src/build_test.cc
+++ b/src/build_test.cc
@@ -986,9 +986,19 @@
ASSERT_EQ(1u, fs_.files_read_.size());
EXPECT_EQ("foo.o.d", fs_.files_read_[0]);
- // Expect three new edges: one generating foo.o, and two more from
- // loading the depfile.
- ASSERT_EQ(orig_edges + 3, (int)state_.edges_.size());
+ // Expect one new edge generating foo.o. Loading the depfile should have
+ // added nodes, but not phony edges to the graph.
+ ASSERT_EQ(orig_edges + 1, (int)state_.edges_.size());
+
+ // Verify that nodes for blah.h and bar.h were added and that they
+ // are marked as generated by a dep loader.
+ ASSERT_FALSE(state_.LookupNode("foo.o")->generated_by_dep_loader());
+ ASSERT_FALSE(state_.LookupNode("foo.c")->generated_by_dep_loader());
+ ASSERT_TRUE(state_.LookupNode("blah.h"));
+ ASSERT_TRUE(state_.LookupNode("blah.h")->generated_by_dep_loader());
+ ASSERT_TRUE(state_.LookupNode("bar.h"));
+ ASSERT_TRUE(state_.LookupNode("bar.h")->generated_by_dep_loader());
+
// Expect our edge to now have three inputs: foo.c and two headers.
ASSERT_EQ(3u, edge->inputs_.size());
@@ -1154,7 +1164,6 @@
ASSERT_NO_FATAL_FAILURE(AssertParse(&state_,
"rule cc\n command = cc $in\n depfile = $out.d\n"
"build gen/stuff\\things/foo.o: cc x\\y/z\\foo.c\n"));
- Edge* edge = state_.edges_.back();
fs_.Create("x/y/z/foo.c", "");
GetNode("bar.h")->MarkDirty(); // Mark bar.h as missing.
@@ -1167,10 +1176,10 @@
// The depfile path does not get Canonicalize as it seems unnecessary.
EXPECT_EQ("gen/stuff\\things/foo.o.d", fs_.files_read_[0]);
- // Expect three new edges: one generating foo.o, and two more from
- // loading the depfile.
- ASSERT_EQ(orig_edges + 3, (int)state_.edges_.size());
+ // Expect one new edge enerating foo.o.
+ ASSERT_EQ(orig_edges + 1, (int)state_.edges_.size());
// Expect our edge to now have three inputs: foo.c and two headers.
+ Edge* edge = state_.edges_.back();
ASSERT_EQ(3u, edge->inputs_.size());
// Expect the command line we generate to only use the original input, and
@@ -2968,9 +2977,9 @@
EXPECT_TRUE(builder.AddTarget("fo o.o", &err));
ASSERT_EQ("", err);
- // Expect three new edges: one generating fo o.o, and two more from
- // loading the depfile.
- ASSERT_EQ(3u, state.edges_.size());
+ // Expect one new edge generating fo o.o, loading the depfile should
+ // note generate new edges.
+ ASSERT_EQ(1u, state.edges_.size());
// Expect our edge to now have three inputs: foo.c and two headers.
ASSERT_EQ(3u, edge->inputs_.size());
@@ -3110,16 +3119,14 @@
Builder builder(&state, config_, NULL, &deps_log, &fs_, &status_, 0);
builder.command_runner_.reset(&command_runner_);
- Edge* edge = state.edges_.back();
-
state.GetNode("bar.h", 0)->MarkDirty(); // Mark bar.h as missing.
EXPECT_TRUE(builder.AddTarget("a/b/c/d/e/fo o.o", &err));
ASSERT_EQ("", err);
- // Expect three new edges: one generating fo o.o, and two more from
- // loading the depfile.
- ASSERT_EQ(3u, state.edges_.size());
+ // Expect one new edge generating fo o.o.
+ ASSERT_EQ(1u, state.edges_.size());
// Expect our edge to now have three inputs: foo.c and two headers.
+ Edge* edge = state.edges_.back();
ASSERT_EQ(3u, edge->inputs_.size());
// Expect the command line we generate to only use the original input.
@@ -3675,8 +3682,8 @@
EXPECT_TRUE(builder_.AddTarget("out", &err));
ASSERT_EQ("", err);
- // Loading the depfile gave tmp.imp a phony input edge.
- ASSERT_TRUE(GetNode("tmp.imp")->in_edge()->is_phony());
+ // Loading the depfile did not give tmp.imp a phony input edge.
+ ASSERT_FALSE(GetNode("tmp.imp")->in_edge());
EXPECT_TRUE(builder_.Build(&err));
EXPECT_EQ("", err);
diff --git a/src/dyndep.cc b/src/dyndep.cc
index dd4ed09..a0d699d 100644
--- a/src/dyndep.cc
+++ b/src/dyndep.cc
@@ -97,15 +97,10 @@
for (std::vector<Node*>::const_iterator i =
dyndeps->implicit_outputs_.begin();
i != dyndeps->implicit_outputs_.end(); ++i) {
- if (Edge* old_in_edge = (*i)->in_edge()) {
- // This node already has an edge producing it. Fail with an error
- // unless the edge was generated by ImplicitDepLoader, in which
- // case we can replace it with the now-known real producer.
- if (!old_in_edge->generated_by_dep_loader_) {
- *err = "multiple rules generate " + (*i)->path();
- return false;
- }
- old_in_edge->outputs_.clear();
+ if ((*i)->in_edge()) {
+ // This node already has an edge producing it.
+ *err = "multiple rules generate " + (*i)->path();
+ return false;
}
(*i)->set_in_edge(edge);
}
diff --git a/src/graph.cc b/src/graph.cc
index 199294d..62f13ec 100644
--- a/src/graph.cc
+++ b/src/graph.cc
@@ -728,7 +728,6 @@
Node* node = state_->GetNode(*i, slash_bits);
*implicit_dep = node;
node->AddOutEdge(edge);
- CreatePhonyInEdge(node);
}
return true;
@@ -756,7 +755,6 @@
Node* node = deps->nodes[i];
*implicit_dep = node;
node->AddOutEdge(edge);
- CreatePhonyInEdge(node);
}
return true;
}
@@ -768,21 +766,3 @@
edge->implicit_deps_ += count;
return edge->inputs_.end() - edge->order_only_deps_ - count;
}
-
-void ImplicitDepLoader::CreatePhonyInEdge(Node* node) {
- if (node->in_edge())
- return;
-
- Edge* phony_edge = state_->AddEdge(&State::kPhonyRule);
- phony_edge->generated_by_dep_loader_ = true;
- node->set_in_edge(phony_edge);
- phony_edge->outputs_.push_back(node);
-
- // RecomputeDirty might not be called for phony_edge if a previous call
- // to RecomputeDirty had caused the file to be stat'ed. Because previous
- // invocations of RecomputeDirty would have seen this node without an
- // input edge (and therefore ready), we have to set outputs_ready_ to true
- // to avoid a potential stuck build. If we do call RecomputeDirty for
- // this node, it will simply set outputs_ready_ to the correct value.
- phony_edge->outputs_ready_ = true;
-}
diff --git a/src/graph.h b/src/graph.h
index d07a9b7..5c8ca2c 100644
--- a/src/graph.h
+++ b/src/graph.h
@@ -104,6 +104,14 @@
Edge* in_edge() const { return in_edge_; }
void set_in_edge(Edge* edge) { in_edge_ = edge; }
+ /// Indicates whether this node was generated from a depfile or dyndep file,
+ /// instead of being a regular input or output from the Ninja manifest.
+ bool generated_by_dep_loader() const { return generated_by_dep_loader_; }
+
+ void set_generated_by_dep_loader(bool value) {
+ generated_by_dep_loader_ = value;
+ }
+
int id() const { return id_; }
void set_id(int id) { id_ = id; }
@@ -146,6 +154,13 @@
/// has not yet been loaded.
bool dyndep_pending_;
+ /// Set to true when this node comes from a depfile, a dyndep file or the
+ /// deps log. If it does not have a producing edge, the build should not
+ /// abort if it is missing (as for regular source inputs). By default
+ /// all nodes have this flag set to true, since the deps and build logs
+ /// can be loaded before the manifest.
+ bool generated_by_dep_loader_ = true;
+
/// The Edge that produces this Node, or NULL when there is no
/// known edge to produce it.
Edge* in_edge_;
@@ -297,11 +312,6 @@
/// an iterator pointing at the first new space.
std::vector<Node*>::iterator PreallocateSpace(Edge* edge, int count);
- /// If we don't have a edge that generates this input already,
- /// create one; this makes us not abort if the input is missing,
- /// but instead will rebuild in that circumstance.
- void CreatePhonyInEdge(Node* node);
-
State* state_;
DiskInterface* disk_interface_;
DepsLog* deps_log_;
diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc
index 8db6eb3..103c365 100644
--- a/src/manifest_parser.cc
+++ b/src/manifest_parser.cc
@@ -14,8 +14,10 @@
#include "manifest_parser.h"
+#include <assert.h>
#include <stdio.h>
#include <stdlib.h>
+
#include <vector>
#include "graph.h"
@@ -416,6 +418,7 @@
if (dgi == edge->inputs_.end()) {
return lexer_.Error("dyndep '" + dyndep + "' is not an input", err);
}
+ assert(!edge->dyndep_->generated_by_dep_loader());
}
return true;
diff --git a/src/state.cc b/src/state.cc
index 556b0d8..0a68f21 100644
--- a/src/state.cc
+++ b/src/state.cc
@@ -128,6 +128,7 @@
void State::AddIn(Edge* edge, StringPiece path, uint64_t slash_bits) {
Node* node = GetNode(path, slash_bits);
+ node->set_generated_by_dep_loader(false);
edge->inputs_.push_back(node);
node->AddOutEdge(edge);
}
@@ -138,6 +139,7 @@
return false;
edge->outputs_.push_back(node);
node->set_in_edge(edge);
+ node->set_generated_by_dep_loader(false);
return true;
}
@@ -145,6 +147,7 @@
Node* node = GetNode(path, slash_bits);
edge->validations_.push_back(node);
node->AddValidationOutEdge(edge);
+ node->set_generated_by_dep_loader(false);
}
bool State::AddDefault(StringPiece path, string* err) {
diff --git a/src/state.h b/src/state.h
index 878ac6d..886b78f 100644
--- a/src/state.h
+++ b/src/state.h
@@ -105,6 +105,9 @@
Node* LookupNode(StringPiece path) const;
Node* SpellcheckNode(const std::string& path);
+ /// Add input / output / validation nodes to a given edge. This also
+ /// ensures that the generated_by_dep_loader() flag for all these nodes
+ /// is set to false, to indicate that they come from the input manifest.
void AddIn(Edge* edge, StringPiece path, uint64_t slash_bits);
bool AddOut(Edge* edge, StringPiece path, uint64_t slash_bits);
void AddValidation(Edge* edge, StringPiece path, uint64_t slash_bits);
