Restore tolerance of self-referencing phony build statements

Since commit v1.8.0^2~3^2~1 (Teach RecomputeDirty to detect cycles in
the build graph, 2015-11-13) we correctly reject self-referencing phony
build statements like

    build a: phony a

as cycles.  Unfortunately this breaks support for CMake 2.8.12.x and
3.0.x because those versions incorrectly produce edges of this form
(that we used to tolerate).  In order to preserve compatibility with
those CMake versions we need to restore tolerance of these edges.

Add a special case to the manifest parser to filter out self-referencing
inputs of phony edges of the form produced by those CMake versions.
Warn by default, but add a `-w phonycycle={err,warn}` option to make it
an error.

Fixes: #1322

8 files changed, 111 insertions, 3 deletions

diff --git a/src/build_test.cc b/src/build_test.cc
index a0f898f..46ab33e 100644
--- a/src/build_test.cc
+++ b/src/build_test.cc
@@ -1068,6 +1068,19 @@ TEST_F(BuildTest, PhonyNoWork) {
   EXPECT_TRUE(builder_.AlreadyUpToDate());
 }
 
+// Test a self-referencing phony.  Ideally this should not work, but
+// ninja 1.7 and below tolerated and CMake 2.8.12.x and 3.0.x both
+// incorrectly produce it.  We tolerate it for compatibility.
+TEST_F(BuildTest, PhonySelfReference) {
+  string err;
+  ASSERT_NO_FATAL_FAILURE(AssertParse(&state_,
+"build a: phony a\n"));
+
+  EXPECT_TRUE(builder_.AddTarget("a", &err));
+  ASSERT_EQ("", err);
+  EXPECT_TRUE(builder_.AlreadyUpToDate());
+}
+
 TEST_F(BuildTest, Fail) {
   ASSERT_NO_FATAL_FAILURE(AssertParse(&state_,
 "rule fail\n"
diff --git a/src/graph.cc b/src/graph.cc
index 7dd9491..ce4ea77 100644
--- a/src/graph.cc
+++ b/src/graph.cc
@@ -170,6 +170,13 @@ bool DependencyScan::VerifyDAG(Node* node, vector<Node*>* stack, string* err) {
     err->append(" -> ");
   }
   err->append((*start)->path());
+
+  if ((start + 1) == stack->end() && edge->maybe_phonycycle_diagnostic()) {
+    // The manifest parser would have filtered out the self-referencing
+    // input if it were not configured to allow the error.
+    err->append(" [-w phonycycle=err]");
+  }
+
   return false;
 }
 
@@ -410,6 +417,14 @@ bool Edge::use_console() const {
   return pool() == &State::kConsolePool;
 }
 
+bool Edge::maybe_phonycycle_diagnostic() const {
+  // CMake 2.8.12.x and 3.0.x produced self-referencing phony rules
+  // of the form "build a: phony ... a ...".   Restrict our
+  // "phonycycle" diagnostic option to the form it used.
+  return is_phony() && outputs_.size() == 1 && implicit_outs_ == 0 &&
+      implicit_deps_ == 0;
+}
+
 // static
 string Node::PathDecanonicalized(const string& path, uint64_t slash_bits) {
   string result = path;
diff --git a/src/graph.h b/src/graph.h
index 586c588..a8f0641 100644
--- a/src/graph.h
+++ b/src/graph.h
@@ -201,6 +201,7 @@ struct Edge {
 
   bool is_phony() const;
   bool use_console() const;
+  bool maybe_phonycycle_diagnostic() const;
 };
 
 
diff --git a/src/graph_test.cc b/src/graph_test.cc
index d4d2824..422bc9a 100644
--- a/src/graph_test.cc
+++ b/src/graph_test.cc
@@ -323,6 +323,18 @@ TEST_F(GraphTest, NestedPhonyPrintsDone) {
   ASSERT_FALSE(plan_.more_to_do());
 }
 
+TEST_F(GraphTest, PhonySelfReferenceError) {
+  ManifestParserOptions parser_opts;
+  parser_opts.phony_cycle_action_ = kPhonyCycleActionError;
+  AssertParse(&state_,
+"build a: phony a\n",
+  parser_opts);
+
+  string err;
+  EXPECT_FALSE(scan_.RecomputeDirty(GetNode("a"), &err));
+  ASSERT_EQ("dependency cycle: a -> a [-w phonycycle=err]", err);
+}
+
 TEST_F(GraphTest, DependencyCycle) {
   AssertParse(&state_,
 "build out: cat mid\n"
diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc
index b4d7fe7..27c423b 100644
--- a/src/manifest_parser.cc
+++ b/src/manifest_parser.cc
@@ -383,6 +383,25 @@ bool ManifestParser::ParseEdge(string* err) {
   edge->implicit_deps_ = implicit;
   edge->order_only_deps_ = order_only;
 
+  if (options_.phony_cycle_action_ == kPhonyCycleActionWarn &&
+      edge->maybe_phonycycle_diagnostic()) {
+    // CMake 2.8.12.x and 3.0.x incorrectly write phony build statements
+    // that reference themselves.  Ninja used to tolerate these in the
+    // build graph but that has since been fixed.  Filter them out to
+    // support users of those old CMake versions.
+    Node* out = edge->outputs_[0];
+    vector<Node*>::iterator new_end =
+        remove(edge->inputs_.begin(), edge->inputs_.end(), out);
+    if (new_end != edge->inputs_.end()) {
+      edge->inputs_.erase(new_end, edge->inputs_.end());
+      if (!quiet_) {
+        Warning("phony target '%s' names itself as an input; "
+                "ignoring [-w phonycycle=warn]",
+                out->path().c_str());
+      }
+    }
+  }
+
   // Multiple outputs aren't (yet?) supported with depslog.
   string deps_type = edge->GetBinding("deps");
   if (!deps_type.empty() && edge->outputs_.size() > 1) {
diff --git a/src/manifest_parser.h b/src/manifest_parser.h
index 4ae21c4..2136018 100644
--- a/src/manifest_parser.h
+++ b/src/manifest_parser.h
@@ -31,9 +31,17 @@ enum DupeEdgeAction {
   kDupeEdgeActionError,
 };
 
+enum PhonyCycleAction {
+  kPhonyCycleActionWarn,
+  kPhonyCycleActionError,
+};
+
 struct ManifestParserOptions {
-  ManifestParserOptions(): dupe_edge_action_(kDupeEdgeActionWarn) {}
+  ManifestParserOptions()
+      : dupe_edge_action_(kDupeEdgeActionWarn),
+        phony_cycle_action_(kPhonyCycleActionWarn) {}
   DupeEdgeAction dupe_edge_action_;
+  PhonyCycleAction phony_cycle_action_;
 };
 
 /// Parses .ninja files.
diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc
index 756efdf..39ed810 100644
--- a/src/manifest_parser_test.cc
+++ b/src/manifest_parser_test.cc
@@ -384,6 +384,32 @@ TEST_F(ParserTest, DuplicateEdgeInIncludedFile) {
             err);
 }
 
+TEST_F(ParserTest, PhonySelfReferenceIgnored) {
+  ASSERT_NO_FATAL_FAILURE(AssertParse(
+"build a: phony a\n"
+));
+
+  Node* node = state.LookupNode("a");
+  Edge* edge = node->in_edge();
+  ASSERT_TRUE(edge->inputs_.empty());
+}
+
+TEST_F(ParserTest, PhonySelfReferenceKept) {
+  const char kInput[] =
+"build a: phony a\n";
+  ManifestParserOptions parser_opts;
+  parser_opts.phony_cycle_action_ = kPhonyCycleActionError;
+  ManifestParser parser(&state, &fs_, parser_opts);
+  string err;
+  EXPECT_TRUE(parser.ParseTest(kInput, &err));
+  EXPECT_EQ("", err);
+
+  Node* node = state.LookupNode("a");
+  Edge* edge = node->in_edge();
+  ASSERT_EQ(edge->inputs_.size(), 1);
+  ASSERT_EQ(edge->inputs_[0], node);
+}
+
 TEST_F(ParserTest, ReservedWords) {
   ASSERT_NO_FATAL_FAILURE(AssertParse(
 "rule build\n"
diff --git a/src/ninja.cc b/src/ninja.cc
index 586e8dc..ed004ac 100644
--- a/src/ninja.cc
+++ b/src/ninja.cc
@@ -70,6 +70,9 @@ struct Options {
 
   /// Whether duplicate rules for one target should warn or print an error.
   bool dupe_edges_should_err;
+
+  /// Whether phony cycles should warn or print an error.
+  bool phony_cycle_should_err;
 };
 
 /// The Ninja main() loads up a series of data structures; various tools need
@@ -845,7 +848,8 @@ bool DebugEnable(const string& name) {
 bool WarningEnable(const string& name, Options* options) {
   if (name == "list") {
     printf("warning flags:\n"
-"  dupbuild={err,warn}  multiple build lines for one target\n");
+"  dupbuild={err,warn}  multiple build lines for one target\n"
+"  phonycycle={err,warn}  phony build statement references itself\n");
     return false;
   } else if (name == "dupbuild=err") {
     options->dupe_edges_should_err = true;
@@ -853,9 +857,16 @@ bool WarningEnable(const string& name, Options* options) {
   } else if (name == "dupbuild=warn") {
     options->dupe_edges_should_err = false;
     return true;
+  } else if (name == "phonycycle=err") {
+    options->phony_cycle_should_err = true;
+    return true;
+  } else if (name == "phonycycle=warn") {
+    options->phony_cycle_should_err = false;
+    return true;
   } else {
     const char* suggestion =
-        SpellcheckString(name.c_str(), "dupbuild=err", "dupbuild=warn", NULL);
+        SpellcheckString(name.c_str(), "dupbuild=err", "dupbuild=warn",
+                         "phonycycle=err", "phonycycle=warn", NULL);
     if (suggestion) {
       Error("unknown warning flag '%s', did you mean '%s'?",
             name.c_str(), suggestion);
@@ -1148,6 +1159,9 @@ int real_main(int argc, char** argv) {
     if (options.dupe_edges_should_err) {
       parser_opts.dupe_edge_action_ = kDupeEdgeActionError;
     }
+    if (options.phony_cycle_should_err) {
+      parser_opts.phony_cycle_action_ = kPhonyCycleActionError;
+    }
     ManifestParser parser(&ninja.state_, &ninja.disk_interface_, parser_opts);
     string err;
     if (!parser.Load(options.input_file, &err)) {