Skip to content

Commit

Permalink
Remove -w dupbuild completely, always error on duplicate edges
Browse files Browse the repository at this point in the history
Step 5, fixes ninja-build#931.
  • Loading branch information
jhasse committed Nov 29, 2023
1 parent 82a7cb3 commit 8f47d5a
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 79 deletions.
15 changes: 2 additions & 13 deletions src/manifest_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<size_t>(implicit_outs))
--implicit_outs;
}
lexer_.Error("multiple rules generate " + path, err);
return false;
}
}

Expand Down
6 changes: 1 addition & 5 deletions src/manifest_parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
61 changes: 16 additions & 45 deletions src/manifest_parser_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -330,39 +330,14 @@ 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"
" command = cat $in > $out\n"
"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);
Expand All @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down
18 changes: 2 additions & 16 deletions src/ninja.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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;
}
Expand Down

0 comments on commit 8f47d5a

Please sign in to comment.