diff options
-rw-r--r-- | src/build_test.cc | 13 | ||||
-rw-r--r-- | src/graph.cc | 15 | ||||
-rw-r--r-- | src/graph.h | 1 | ||||
-rw-r--r-- | src/graph_test.cc | 12 | ||||
-rw-r--r-- | src/manifest_parser.cc | 19 | ||||
-rw-r--r-- | src/manifest_parser.h | 10 | ||||
-rw-r--r-- | src/manifest_parser_test.cc | 26 | ||||
-rw-r--r-- | src/ninja.cc | 18 |
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)) { |