From 8f47d5aa33c6c303a71093be2eac02672dfb2966 Mon Sep 17 00:00:00 2001 From: Jan Niklas Hasse Date: Wed, 29 Nov 2023 21:16:06 +0100 Subject: Remove `-w dupbuild` completely, always error on duplicate edges Step 5, fixes #931. --- src/manifest_parser.cc | 15 ++--------- src/manifest_parser.h | 6 +---- src/manifest_parser_test.cc | 61 ++++++++++++--------------------------------- src/ninja.cc | 18 ++----------- 4 files changed, 21 insertions(+), 79 deletions(-) diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc index 103c365..aa52989 100644 --- a/src/manifest_parser.cc +++ b/src/manifest_parser.cc @@ -337,19 +337,8 @@ bool ManifestParser::ParseEdge(string* err) { uint64_t slash_bits; CanonicalizePath(&path, &slash_bits); if (!state_->AddOut(edge, path, slash_bits)) { - if (options_.dupe_edge_action_ == kDupeEdgeActionError) { - lexer_.Error("multiple rules generate " + path, err); - return false; - } else { - if (!quiet_) { - Warning( - "multiple rules generate %s. builds involving this target will " - "not be correct; continuing anyway", - path.c_str()); - } - if (e - i <= static_cast(implicit_outs)) - --implicit_outs; - } + lexer_.Error("multiple rules generate " + path, err); + return false; } } diff --git a/src/manifest_parser.h b/src/manifest_parser.h index 954cf46..db6812d 100644 --- a/src/manifest_parser.h +++ b/src/manifest_parser.h @@ -31,11 +31,7 @@ enum PhonyCycleAction { }; struct ManifestParserOptions { - ManifestParserOptions() - : dupe_edge_action_(kDupeEdgeActionWarn), - phony_cycle_action_(kPhonyCycleActionWarn) {} - DupeEdgeAction dupe_edge_action_; - PhonyCycleAction phony_cycle_action_; + PhonyCycleAction phony_cycle_action_ = kPhonyCycleActionWarn; }; /// Parses .ninja files. diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc index 66b72e2..8a7b135 100644 --- a/src/manifest_parser_test.cc +++ b/src/manifest_parser_test.cc @@ -330,29 +330,6 @@ TEST_F(ParserTest, CanonicalizePathsBackslashes) { } #endif -TEST_F(ParserTest, DuplicateEdgeWithMultipleOutputs) { - ASSERT_NO_FATAL_FAILURE(AssertParse( -"rule cat\n" -" command = cat $in > $out\n" -"build out1 out2: cat in1\n" -"build out1: cat in2\n" -"build final: cat out1\n" -)); - // AssertParse() checks that the generated build graph is self-consistent. - // That's all the checking that this test needs. -} - -TEST_F(ParserTest, NoDeadPointerFromDuplicateEdge) { - ASSERT_NO_FATAL_FAILURE(AssertParse( -"rule cat\n" -" command = cat $in > $out\n" -"build out: cat in\n" -"build out: cat in\n" -)); - // AssertParse() checks that the generated build graph is self-consistent. - // That's all the checking that this test needs. -} - TEST_F(ParserTest, DuplicateEdgeWithMultipleOutputsError) { const char kInput[] = "rule cat\n" @@ -360,9 +337,7 @@ TEST_F(ParserTest, DuplicateEdgeWithMultipleOutputsError) { "build out1 out2: cat in1\n" "build out1: cat in2\n" "build final: cat out1\n"; - ManifestParserOptions parser_opts; - parser_opts.dupe_edge_action_ = kDupeEdgeActionError; - ManifestParser parser(&state, &fs_, parser_opts); + ManifestParser parser(&state, &fs_); string err; EXPECT_FALSE(parser.ParseTest(kInput, &err)); EXPECT_EQ("input:5: multiple rules generate out1\n", err); @@ -377,9 +352,7 @@ TEST_F(ParserTest, DuplicateEdgeInIncludedFile) { "build final: cat out1\n"); const char kInput[] = "subninja sub.ninja\n"; - ManifestParserOptions parser_opts; - parser_opts.dupe_edge_action_ = kDupeEdgeActionError; - ManifestParser parser(&state, &fs_, parser_opts); + ManifestParser parser(&state, &fs_); string err; EXPECT_FALSE(parser.ParseTest(kInput, &err)); EXPECT_EQ("sub.ninja:5: multiple rules generate out1\n", err); @@ -997,28 +970,26 @@ TEST_F(ParserTest, ImplicitOutputEmpty) { EXPECT_FALSE(edge->is_implicit_out(0)); } -TEST_F(ParserTest, ImplicitOutputDupe) { - ASSERT_NO_FATAL_FAILURE(AssertParse( +TEST_F(ParserTest, ImplicitOutputDupeError) { + const char kInput[] = "rule cat\n" " command = cat $in > $out\n" -"build foo baz | foo baq foo: cat bar\n")); - - Edge* edge = state.LookupNode("foo")->in_edge(); - ASSERT_EQ(edge->outputs_.size(), 3); - EXPECT_FALSE(edge->is_implicit_out(0)); - EXPECT_FALSE(edge->is_implicit_out(1)); - EXPECT_TRUE(edge->is_implicit_out(2)); +"build foo baz | foo baq foo: cat bar\n"; + ManifestParser parser(&state, &fs_); + string err; + EXPECT_FALSE(parser.ParseTest(kInput, &err)); + EXPECT_EQ("input:4: multiple rules generate foo\n", err); } -TEST_F(ParserTest, ImplicitOutputDupes) { - ASSERT_NO_FATAL_FAILURE(AssertParse( +TEST_F(ParserTest, ImplicitOutputDupesError) { + const char kInput[] = "rule cat\n" " command = cat $in > $out\n" -"build foo foo foo | foo foo foo foo: cat bar\n")); - - Edge* edge = state.LookupNode("foo")->in_edge(); - ASSERT_EQ(edge->outputs_.size(), 1); - EXPECT_FALSE(edge->is_implicit_out(0)); +"build foo foo foo | foo foo foo foo: cat bar\n"; + ManifestParser parser(&state, &fs_); + string err; + EXPECT_FALSE(parser.ParseTest(kInput, &err)); + EXPECT_EQ("input:4: multiple rules generate foo\n", err); } TEST_F(ParserTest, NoExplicitOutput) { diff --git a/src/ninja.cc b/src/ninja.cc index c011be1..839e9e6 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -77,9 +77,6 @@ struct Options { /// Tool to run rather than building. const Tool* tool; - /// 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; }; @@ -1210,12 +1207,6 @@ bool WarningEnable(const string& name, Options* options) { " phonycycle={err,warn} phony build statement references itself\n" ); return false; - } else if (name == "dupbuild=err") { - options->dupe_edges_should_err = true; - return true; - } 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; @@ -1227,9 +1218,8 @@ bool WarningEnable(const string& name, Options* options) { Warning("deprecated warning 'depfilemulti'"); return true; } else { - const char* suggestion = - SpellcheckString(name.c_str(), "dupbuild=err", "dupbuild=warn", - "phonycycle=err", "phonycycle=warn", NULL); + const char* suggestion = SpellcheckString(name.c_str(), "phonycycle=err", + "phonycycle=warn", nullptr); if (suggestion) { Error("unknown warning flag '%s', did you mean '%s'?", name.c_str(), suggestion); @@ -1525,7 +1515,6 @@ NORETURN void real_main(int argc, char** argv) { BuildConfig config; Options options = {}; options.input_file = "build.ninja"; - options.dupe_edges_should_err = true; setvbuf(stdout, NULL, _IOLBF, BUFSIZ); const char* ninja_command = argv[0]; @@ -1562,9 +1551,6 @@ NORETURN void real_main(int argc, char** argv) { NinjaMain ninja(ninja_command, config); ManifestParserOptions parser_opts; - if (options.dupe_edges_should_err) { - parser_opts.dupe_edge_action_ = kDupeEdgeActionError; - } if (options.phony_cycle_should_err) { parser_opts.phony_cycle_action_ = kPhonyCycleActionError; } -- cgit v0.12 From 4d98903d4c986f720ddb3f18d32c1125ef3e680e Mon Sep 17 00:00:00 2001 From: Jan Niklas Hasse Date: Wed, 6 Dec 2023 20:49:28 +0100 Subject: Improve misleading error message when an output is defined multiple times --- src/manifest_parser.cc | 4 ++-- src/manifest_parser_test.cc | 4 ++-- src/missing_deps_test.cc | 6 +++--- src/state.cc | 11 +++++++++-- src/state.h | 2 +- src/state_test.cc | 2 +- 6 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc index aa52989..c4b2980 100644 --- a/src/manifest_parser.cc +++ b/src/manifest_parser.cc @@ -336,8 +336,8 @@ bool ManifestParser::ParseEdge(string* err) { return lexer_.Error("empty path", err); uint64_t slash_bits; CanonicalizePath(&path, &slash_bits); - if (!state_->AddOut(edge, path, slash_bits)) { - lexer_.Error("multiple rules generate " + path, err); + if (!state_->AddOut(edge, path, slash_bits, err)) { + lexer_.Error(std::string(*err), err); return false; } } diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc index 8a7b135..c5a1fe8 100644 --- a/src/manifest_parser_test.cc +++ b/src/manifest_parser_test.cc @@ -978,7 +978,7 @@ TEST_F(ParserTest, ImplicitOutputDupeError) { ManifestParser parser(&state, &fs_); string err; EXPECT_FALSE(parser.ParseTest(kInput, &err)); - EXPECT_EQ("input:4: multiple rules generate foo\n", err); + EXPECT_EQ("input:4: foo is defined as an output multiple times\n", err); } TEST_F(ParserTest, ImplicitOutputDupesError) { @@ -989,7 +989,7 @@ TEST_F(ParserTest, ImplicitOutputDupesError) { ManifestParser parser(&state, &fs_); string err; EXPECT_FALSE(parser.ParseTest(kInput, &err)); - EXPECT_EQ("input:4: multiple rules generate foo\n", err); + EXPECT_EQ("input:4: foo is defined as an output multiple times\n", err); } TEST_F(ParserTest, NoExplicitOutput) { diff --git a/src/missing_deps_test.cc b/src/missing_deps_test.cc index 12ae8ed..dae377b 100644 --- a/src/missing_deps_test.cc +++ b/src/missing_deps_test.cc @@ -64,9 +64,9 @@ struct MissingDependencyScannerTest : public testing::Test { compile_rule_.AddBinding("deps", deps_type); generator_rule_.AddBinding("deps", deps_type); Edge* header_edge = state_.AddEdge(&generator_rule_); - state_.AddOut(header_edge, "generated_header", 0); + state_.AddOut(header_edge, "generated_header", 0, nullptr); Edge* compile_edge = state_.AddEdge(&compile_rule_); - state_.AddOut(compile_edge, "compiled_object", 0); + state_.AddOut(compile_edge, "compiled_object", 0, nullptr); } void CreateGraphDependencyBetween(const char* from, const char* to) { @@ -130,7 +130,7 @@ TEST_F(MissingDependencyScannerTest, MissingDepFixedIndirect) { CreateInitialState(); // Adding an indirect dependency also fixes the issue Edge* intermediate_edge = state_.AddEdge(&generator_rule_); - state_.AddOut(intermediate_edge, "intermediate", 0); + state_.AddOut(intermediate_edge, "intermediate", 0, nullptr); CreateGraphDependencyBetween("compiled_object", "intermediate"); CreateGraphDependencyBetween("intermediate", "generated_header"); RecordDepsLogDep("compiled_object", "generated_header"); diff --git a/src/state.cc b/src/state.cc index 0a68f21..d4b9a71 100644 --- a/src/state.cc +++ b/src/state.cc @@ -133,10 +133,17 @@ void State::AddIn(Edge* edge, StringPiece path, uint64_t slash_bits) { node->AddOutEdge(edge); } -bool State::AddOut(Edge* edge, StringPiece path, uint64_t slash_bits) { +bool State::AddOut(Edge* edge, StringPiece path, uint64_t slash_bits, + std::string* err) { Node* node = GetNode(path, slash_bits); - if (node->in_edge()) + if (Edge* other = node->in_edge()) { + if (other == edge) { + *err = path.AsString() + " is defined as an output multiple times"; + } else { + *err = "multiple rules generate " + path.AsString(); + } return false; + } edge->outputs_.push_back(node); node->set_in_edge(edge); node->set_generated_by_dep_loader(false); diff --git a/src/state.h b/src/state.h index 886b78f..29bed56 100644 --- a/src/state.h +++ b/src/state.h @@ -109,7 +109,7 @@ struct State { /// 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); + bool AddOut(Edge* edge, StringPiece path, uint64_t slash_bits, std::string* err); void AddValidation(Edge* edge, StringPiece path, uint64_t slash_bits); bool AddDefault(StringPiece path, std::string* error); diff --git a/src/state_test.cc b/src/state_test.cc index 96469f9..e0e3060 100644 --- a/src/state_test.cc +++ b/src/state_test.cc @@ -36,7 +36,7 @@ TEST(State, Basic) { Edge* edge = state.AddEdge(rule); state.AddIn(edge, "in1", 0); state.AddIn(edge, "in2", 0); - state.AddOut(edge, "out", 0); + state.AddOut(edge, "out", 0, nullptr); EXPECT_EQ("cat in1 in2 > out", edge->EvaluateCommand()); -- cgit v0.12