From e679202a14d9ca08ccd0f471f2bcbf6388ddb3de Mon Sep 17 00:00:00 2001 From: Brad King Date: Thu, 7 Sep 2017 13:55:14 -0400 Subject: Factor ManifestParser options into a structure This will allow more options to be added without updating everywhere that constructs a ManifestParser. Also extend the AssertParse function to take the options so tests can control them. --- src/build_log_perftest.cc | 2 +- src/manifest_parser.cc | 8 ++-- src/manifest_parser.h | 9 +++- src/manifest_parser_perftest.cc | 2 +- src/manifest_parser_test.cc | 98 +++++++++++++++++++++-------------------- src/ninja.cc | 9 ++-- src/test.cc | 5 ++- src/test.h | 4 +- 8 files changed, 75 insertions(+), 62 deletions(-) diff --git a/src/build_log_perftest.cc b/src/build_log_perftest.cc index b4efb1d..e471d13 100644 --- a/src/build_log_perftest.cc +++ b/src/build_log_perftest.cc @@ -71,7 +71,7 @@ bool WriteTestData(string* err) { long_rule_command += "$in -o $out\n"; State state; - ManifestParser parser(&state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&state, NULL); if (!parser.ParseTest("rule cxx\n command = " + long_rule_command, err)) return false; diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc index 2164921..b4d7fe7 100644 --- a/src/manifest_parser.cc +++ b/src/manifest_parser.cc @@ -26,9 +26,9 @@ #include "version.h" ManifestParser::ManifestParser(State* state, FileReader* file_reader, - DupeEdgeAction dupe_edge_action) + ManifestParserOptions options) : state_(state), file_reader_(file_reader), - dupe_edge_action_(dupe_edge_action), quiet_(false) { + options_(options), quiet_(false) { env_ = &state->bindings_; } @@ -346,7 +346,7 @@ bool ManifestParser::ParseEdge(string* err) { if (!CanonicalizePath(&path, &slash_bits, &path_err)) return lexer_.Error(path_err, err); if (!state_->AddOut(edge, path, slash_bits)) { - if (dupe_edge_action_ == kDupeEdgeActionError) { + if (options_.dupe_edge_action_ == kDupeEdgeActionError) { lexer_.Error("multiple rules generate " + path + " [-w dupbuild=err]", err); return false; @@ -400,7 +400,7 @@ bool ManifestParser::ParseFileInclude(bool new_scope, string* err) { return false; string path = eval.Evaluate(env_); - ManifestParser subparser(state_, file_reader_, dupe_edge_action_); + ManifestParser subparser(state_, file_reader_, options_); if (new_scope) { subparser.env_ = new BindingEnv(env_); } else { diff --git a/src/manifest_parser.h b/src/manifest_parser.h index 043e4b2..4ae21c4 100644 --- a/src/manifest_parser.h +++ b/src/manifest_parser.h @@ -31,10 +31,15 @@ enum DupeEdgeAction { kDupeEdgeActionError, }; +struct ManifestParserOptions { + ManifestParserOptions(): dupe_edge_action_(kDupeEdgeActionWarn) {} + DupeEdgeAction dupe_edge_action_; +}; + /// Parses .ninja files. struct ManifestParser { ManifestParser(State* state, FileReader* file_reader, - DupeEdgeAction dupe_edge_action); + ManifestParserOptions options = ManifestParserOptions()); /// Load and parse a file. bool Load(const string& filename, string* err, Lexer* parent = NULL); @@ -67,7 +72,7 @@ private: BindingEnv* env_; FileReader* file_reader_; Lexer lexer_; - DupeEdgeAction dupe_edge_action_; + ManifestParserOptions options_; bool quiet_; }; diff --git a/src/manifest_parser_perftest.cc b/src/manifest_parser_perftest.cc index 60c2054..67d11f9 100644 --- a/src/manifest_parser_perftest.cc +++ b/src/manifest_parser_perftest.cc @@ -56,7 +56,7 @@ int LoadManifests(bool measure_command_evaluation) { string err; RealDiskInterface disk_interface; State state; - ManifestParser parser(&state, &disk_interface, kDupeEdgeActionWarn); + ManifestParser parser(&state, &disk_interface); if (!parser.Load("build.ninja", &err)) { fprintf(stderr, "Failed to read test data: %s\n", err.c_str()); exit(1); diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc index 3c82dc5..756efdf 100644 --- a/src/manifest_parser_test.cc +++ b/src/manifest_parser_test.cc @@ -23,7 +23,7 @@ struct ParserTest : public testing::Test { void AssertParse(const char* input) { - ManifestParser parser(&state, &fs_, kDupeEdgeActionWarn); + ManifestParser parser(&state, &fs_); string err; EXPECT_TRUE(parser.ParseTest(input, &err)); ASSERT_EQ("", err); @@ -358,7 +358,9 @@ TEST_F(ParserTest, DuplicateEdgeWithMultipleOutputsError) { "build out1 out2: cat in1\n" "build out1: cat in2\n" "build final: cat out1\n"; - ManifestParser parser(&state, &fs_, kDupeEdgeActionError); + ManifestParserOptions parser_opts; + parser_opts.dupe_edge_action_ = kDupeEdgeActionError; + ManifestParser parser(&state, &fs_, parser_opts); string err; EXPECT_FALSE(parser.ParseTest(kInput, &err)); EXPECT_EQ("input:5: multiple rules generate out1 [-w dupbuild=err]\n", err); @@ -373,7 +375,9 @@ TEST_F(ParserTest, DuplicateEdgeInIncludedFile) { "build final: cat out1\n"); const char kInput[] = "subninja sub.ninja\n"; - ManifestParser parser(&state, &fs_, kDupeEdgeActionError); + ManifestParserOptions parser_opts; + parser_opts.dupe_edge_action_ = kDupeEdgeActionError; + ManifestParser parser(&state, &fs_, parser_opts); string err; EXPECT_FALSE(parser.ParseTest(kInput, &err)); EXPECT_EQ("sub.ninja:5: multiple rules generate out1 [-w dupbuild=err]\n", @@ -391,7 +395,7 @@ TEST_F(ParserTest, ReservedWords) { TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest(string("subn", 4), &err)); EXPECT_EQ("input:1: expected '=', got eof\n" @@ -402,7 +406,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("foobar", &err)); EXPECT_EQ("input:1: expected '=', got eof\n" @@ -413,7 +417,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("x 3", &err)); EXPECT_EQ("input:1: expected '=', got identifier\n" @@ -424,7 +428,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("x = 3", &err)); EXPECT_EQ("input:1: unexpected EOF\n" @@ -435,7 +439,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("x = 3\ny 2", &err)); EXPECT_EQ("input:2: expected '=', got identifier\n" @@ -446,7 +450,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("x = $", &err)); EXPECT_EQ("input:1: bad $-escape (literal $ must be written as $$)\n" @@ -457,7 +461,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("x = $\n $[\n", &err)); EXPECT_EQ("input:2: bad $-escape (literal $ must be written as $$)\n" @@ -468,7 +472,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("x = a$\n b$\n $\n", &err)); EXPECT_EQ("input:4: unexpected EOF\n" @@ -477,7 +481,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("build\n", &err)); EXPECT_EQ("input:1: expected path\n" @@ -488,7 +492,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("build x: y z\n", &err)); EXPECT_EQ("input:1: unknown build rule 'y'\n" @@ -499,7 +503,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("build x:: y z\n", &err)); EXPECT_EQ("input:1: expected build command name\n" @@ -510,7 +514,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule cat\n command = cat ok\n" "build x: cat $\n :\n", @@ -523,7 +527,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule cat\n", &err)); @@ -532,7 +536,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule cat\n" " command = echo\n" @@ -546,7 +550,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule cat\n" " command = echo\n" @@ -558,7 +562,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule cat\n" " command = ${fafsd\n" @@ -573,7 +577,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule cat\n" " command = cat\n" @@ -588,7 +592,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule cat\n" " command = cat\n" @@ -602,7 +606,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule %foo\n", &err)); @@ -611,7 +615,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule cc\n" " command = foo\n" @@ -625,7 +629,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule cc\n command = foo\n" "build $.: cc bar.cc\n", @@ -638,7 +642,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule cc\n command = foo\n && bar", &err)); @@ -647,7 +651,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule cc\n command = foo\n" "build $: cc bar.cc\n", @@ -660,7 +664,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("default\n", &err)); @@ -672,7 +676,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("default nonexistent\n", &err)); @@ -684,7 +688,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule r\n command = r\n" "build b: r\n" @@ -698,7 +702,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("default $a\n", &err)); EXPECT_EQ("input:1: empty path\n" @@ -709,7 +713,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule r\n" " command = r\n" @@ -721,7 +725,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; // the indented blank line must terminate the rule // this also verifies that "unexpected (token)" errors are correct @@ -734,7 +738,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("pool\n", &err)); EXPECT_EQ("input:1: expected pool name\n", err); @@ -742,7 +746,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("pool foo\n", &err)); EXPECT_EQ("input:2: expected 'depth =' line\n", err); @@ -750,7 +754,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("pool foo\n" " depth = 4\n" @@ -763,7 +767,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("pool foo\n" " depth = -1\n", &err)); @@ -775,7 +779,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("pool foo\n" " bar = 1\n", &err)); @@ -787,7 +791,7 @@ TEST_F(ParserTest, Errors) { { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; // Pool names are dereferenced at edge parsing time. EXPECT_FALSE(parser.ParseTest("rule run\n" @@ -800,7 +804,7 @@ TEST_F(ParserTest, Errors) { TEST_F(ParserTest, MissingInput) { State local_state; - ManifestParser parser(&local_state, &fs_, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, &fs_); string err; EXPECT_FALSE(parser.Load("build.ninja", &err)); EXPECT_EQ("loading 'build.ninja': No such file or directory", err); @@ -808,7 +812,7 @@ TEST_F(ParserTest, MissingInput) { TEST_F(ParserTest, MultipleOutputs) { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_TRUE(parser.ParseTest("rule cc\n command = foo\n depfile = bar\n" "build a.o b.o: cc c.cc\n", @@ -818,7 +822,7 @@ TEST_F(ParserTest, MultipleOutputs) { TEST_F(ParserTest, MultipleOutputsWithDeps) { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule cc\n command = foo\n deps = gcc\n" "build a.o b.o: cc c.cc\n", @@ -853,7 +857,7 @@ TEST_F(ParserTest, SubNinja) { } TEST_F(ParserTest, MissingSubNinja) { - ManifestParser parser(&state, &fs_, kDupeEdgeActionWarn); + ManifestParser parser(&state, &fs_); string err; EXPECT_FALSE(parser.ParseTest("subninja foo.ninja\n", &err)); EXPECT_EQ("input:1: loading 'foo.ninja': No such file or directory\n" @@ -866,7 +870,7 @@ TEST_F(ParserTest, DuplicateRuleInDifferentSubninjas) { // Test that rules are scoped to subninjas. fs_.Create("test.ninja", "rule cat\n" " command = cat\n"); - ManifestParser parser(&state, &fs_, kDupeEdgeActionWarn); + ManifestParser parser(&state, &fs_); string err; EXPECT_TRUE(parser.ParseTest("rule cat\n" " command = cat\n" @@ -879,7 +883,7 @@ TEST_F(ParserTest, DuplicateRuleInDifferentSubninjasWithInclude) { " command = cat\n"); fs_.Create("test.ninja", "include rules.ninja\n" "build x : cat\n"); - ManifestParser parser(&state, &fs_, kDupeEdgeActionWarn); + ManifestParser parser(&state, &fs_); string err; EXPECT_TRUE(parser.ParseTest("include rules.ninja\n" "subninja test.ninja\n" @@ -899,7 +903,7 @@ TEST_F(ParserTest, Include) { TEST_F(ParserTest, BrokenInclude) { fs_.Create("include.ninja", "build\n"); - ManifestParser parser(&state, &fs_, kDupeEdgeActionWarn); + ManifestParser parser(&state, &fs_); string err; EXPECT_FALSE(parser.ParseTest("include include.ninja\n", &err)); EXPECT_EQ("include.ninja:1: expected path\n" @@ -974,7 +978,7 @@ TEST_F(ParserTest, ImplicitOutputDupes) { } TEST_F(ParserTest, NoExplicitOutput) { - ManifestParser parser(&state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&state, NULL); string err; EXPECT_TRUE(parser.ParseTest( "rule cat\n" @@ -1034,7 +1038,7 @@ TEST_F(ParserTest, UTF8) { TEST_F(ParserTest, CRLF) { State local_state; - ManifestParser parser(&local_state, NULL, kDupeEdgeActionWarn); + ManifestParser parser(&local_state, NULL); string err; EXPECT_TRUE(parser.ParseTest("# comment with crlf\r\n", &err)); diff --git a/src/ninja.cc b/src/ninja.cc index 54de7b9..586e8dc 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -1144,10 +1144,11 @@ int real_main(int argc, char** argv) { for (int cycle = 1; cycle <= kCycleLimit; ++cycle) { NinjaMain ninja(ninja_command, config); - ManifestParser parser(&ninja.state_, &ninja.disk_interface_, - options.dupe_edges_should_err - ? kDupeEdgeActionError - : kDupeEdgeActionWarn); + ManifestParserOptions parser_opts; + if (options.dupe_edges_should_err) { + parser_opts.dupe_edge_action_ = kDupeEdgeActionError; + } + ManifestParser parser(&ninja.state_, &ninja.disk_interface_, parser_opts); string err; if (!parser.Load(options.input_file, &err)) { Error("%s", err.c_str()); diff --git a/src/test.cc b/src/test.cc index 51882f0..a9816bc 100644 --- a/src/test.cc +++ b/src/test.cc @@ -95,8 +95,9 @@ Node* StateTestWithBuiltinRules::GetNode(const string& path) { return state_.GetNode(path, 0); } -void AssertParse(State* state, const char* input) { - ManifestParser parser(state, NULL, kDupeEdgeActionWarn); +void AssertParse(State* state, const char* input, + ManifestParserOptions opts) { + ManifestParser parser(state, NULL, opts); string err; EXPECT_TRUE(parser.ParseTest(input, &err)); ASSERT_EQ("", err); diff --git a/src/test.h b/src/test.h index 02ed929..3bce8f7 100644 --- a/src/test.h +++ b/src/test.h @@ -16,6 +16,7 @@ #define NINJA_TEST_H_ #include "disk_interface.h" +#include "manifest_parser.h" #include "state.h" #include "util.h" @@ -122,7 +123,8 @@ struct StateTestWithBuiltinRules : public testing::Test { State state_; }; -void AssertParse(State* state, const char* input); +void AssertParse(State* state, const char* input, + ManifestParserOptions = ManifestParserOptions()); void AssertHash(const char* expected, uint64_t actual); void VerifyGraph(const State& state); -- cgit v0.12 From c0dde0ff7911780152dbe86d5782ebdecfeb37f2 Mon Sep 17 00:00:00 2001 From: Brad King Date: Thu, 7 Sep 2017 10:56:25 -0400 Subject: 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 --- src/build_test.cc | 13 +++++++++++++ src/graph.cc | 15 +++++++++++++++ src/graph.h | 1 + src/graph_test.cc | 12 ++++++++++++ src/manifest_parser.cc | 19 +++++++++++++++++++ src/manifest_parser.h | 10 +++++++++- src/manifest_parser_test.cc | 26 ++++++++++++++++++++++++++ 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* 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::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)) { -- cgit v0.12 From 87111bff382655075f2577c591745a335f0103c7 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Sun, 10 Sep 2017 21:19:22 -0400 Subject: mark this 1.8.2.git --- src/version.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/version.cc b/src/version.cc index 3ee95b2..1b6cfac 100644 --- a/src/version.cc +++ b/src/version.cc @@ -18,7 +18,7 @@ #include "util.h" -const char* kNinjaVersion = "1.8.1.git"; +const char* kNinjaVersion = "1.8.2.git"; void ParseVersion(const string& version, int* major, int* minor) { size_t end = version.find('.'); -- cgit v0.12