From 1daa7470ab7ed147726b560d0bc55327fff3482f Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 8 Mar 2019 18:39:55 -0500 Subject: depfile_parser: remove restriction on multiple outputs --- src/build.cc | 2 +- src/build_test.cc | 223 ++++++++++++++++++++++++++++++++++++++++++++ src/depfile_parser.cc | 43 +++------ src/depfile_parser.h | 13 +-- src/depfile_parser.in.cc | 43 +++------ src/depfile_parser_test.cc | 91 +++++++++++------- src/graph.cc | 36 ++++++- src/manifest_parser.cc | 8 -- src/manifest_parser_test.cc | 5 +- src/ninja.cc | 18 +--- 10 files changed, 344 insertions(+), 138 deletions(-) diff --git a/src/build.cc b/src/build.cc index fe8daca..ced7110 100644 --- a/src/build.cc +++ b/src/build.cc @@ -1033,7 +1033,7 @@ bool Builder::FinishCommand(CommandRunner::Result* result, string* err) { } if (!deps_type.empty() && !config_.dry_run) { - assert(edge->outputs_.size() == 1 && "should have been rejected by parser"); + assert(edge->outputs_.size() >= 1 && "should have been rejected by parser"); for (std::vector::const_iterator o = edge->outputs_.begin(); o != edge->outputs_.end(); ++o) { TimeStamp deps_mtime = disk_interface_->Stat((*o)->path(), err); diff --git a/src/build_test.cc b/src/build_test.cc index ddf8574..426e825 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -488,6 +488,11 @@ struct BuildTest : public StateTestWithBuiltinRules, public BuildLogUser { status_(config_) { } + BuildTest(DepsLog* log) : config_(MakeConfig()), command_runner_(&fs_), + builder_(&state_, config_, NULL, log, &fs_), + status_(config_) { + } + virtual void SetUp() { StateTestWithBuiltinRules::SetUp(); @@ -582,6 +587,8 @@ bool FakeCommandRunner::StartCommand(Edge* edge) { edge->rule().name() == "cat_rsp" || edge->rule().name() == "cat_rsp_out" || edge->rule().name() == "cc" || + edge->rule().name() == "cp_multi_msvc" || + edge->rule().name() == "cp_multi_gcc" || edge->rule().name() == "touch" || edge->rule().name() == "touch-interrupt" || edge->rule().name() == "touch-fail-tick2") { @@ -643,6 +650,14 @@ bool FakeCommandRunner::WaitForCommand(Result* result) { return true; } + if (edge->rule().name() == "cp_multi_msvc") { + const std::string prefix = edge->GetBinding("msvc_deps_prefix"); + for (std::vector::iterator in = edge->inputs_.begin(); + in != edge->inputs_.end(); ++in) { + result->output += prefix + (*in)->path() + '\n'; + } + } + if (edge->rule().name() == "fail" || (edge->rule().name() == "touch-fail-tick2" && fs_->now_ == 2)) result->status = ExitFailure; @@ -1855,6 +1870,214 @@ TEST_F(BuildTest, FailedDepsParse) { EXPECT_EQ("subcommand failed", err); } +struct BuildWithQueryDepsLogTest : public BuildTest { + BuildWithQueryDepsLogTest() : BuildTest(&log_) { + } + + ~BuildWithQueryDepsLogTest() { + log_.Close(); + } + + virtual void SetUp() { + BuildTest::SetUp(); + + temp_dir_.CreateAndEnter("BuildWithQueryDepsLogTest"); + + std::string err; + ASSERT_TRUE(log_.OpenForWrite("ninja_deps", &err)); + ASSERT_EQ("", err); + } + + ScopedTempDir temp_dir_; + + DepsLog log_; +}; + +/// Test a MSVC-style deps log with multiple outputs. +TEST_F(BuildWithQueryDepsLogTest, TwoOutputsDepFileMSVC) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, +"rule cp_multi_msvc\n" +" command = echo 'using $in' && for file in $out; do cp $in $$file; done\n" +" deps = msvc\n" +" msvc_deps_prefix = using \n" +"build out1 out2: cp_multi_msvc in1\n")); + + std::string err; + EXPECT_TRUE(builder_.AddTarget("out1", &err)); + ASSERT_EQ("", err); + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); + ASSERT_EQ(1u, command_runner_.commands_ran_.size()); + EXPECT_EQ("echo 'using in1' && for file in out1 out2; do cp in1 $file; done", command_runner_.commands_ran_[0]); + + Node* out1_node = state_.LookupNode("out1"); + DepsLog::Deps* out1_deps = log_.GetDeps(out1_node); + EXPECT_EQ(1, out1_deps->node_count); + EXPECT_EQ("in1", out1_deps->nodes[0]->path()); + + Node* out2_node = state_.LookupNode("out2"); + DepsLog::Deps* out2_deps = log_.GetDeps(out2_node); + EXPECT_EQ(1, out2_deps->node_count); + EXPECT_EQ("in1", out2_deps->nodes[0]->path()); +} + +/// Test a GCC-style deps log with multiple outputs. +TEST_F(BuildWithQueryDepsLogTest, TwoOutputsDepFileGCCOneLine) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, +"rule cp_multi_gcc\n" +" command = echo '$out: $in' > in.d && for file in $out; do cp in1 $$file; done\n" +" deps = gcc\n" +" depfile = in.d\n" +"build out1 out2: cp_multi_gcc in1 in2\n")); + + std::string err; + EXPECT_TRUE(builder_.AddTarget("out1", &err)); + ASSERT_EQ("", err); + fs_.Create("in.d", "out1 out2: in1 in2"); + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); + ASSERT_EQ(1u, command_runner_.commands_ran_.size()); + EXPECT_EQ("echo 'out1 out2: in1 in2' > in.d && for file in out1 out2; do cp in1 $file; done", command_runner_.commands_ran_[0]); + + Node* out1_node = state_.LookupNode("out1"); + DepsLog::Deps* out1_deps = log_.GetDeps(out1_node); + EXPECT_EQ(2, out1_deps->node_count); + EXPECT_EQ("in1", out1_deps->nodes[0]->path()); + EXPECT_EQ("in2", out1_deps->nodes[1]->path()); + + Node* out2_node = state_.LookupNode("out2"); + DepsLog::Deps* out2_deps = log_.GetDeps(out2_node); + EXPECT_EQ(2, out2_deps->node_count); + EXPECT_EQ("in1", out2_deps->nodes[0]->path()); + EXPECT_EQ("in2", out2_deps->nodes[1]->path()); +} + +/// Test a GCC-style deps log with multiple outputs using a line per input. +TEST_F(BuildWithQueryDepsLogTest, TwoOutputsDepFileGCCMultiLineInput) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, +"rule cp_multi_gcc\n" +" command = echo '$out: in1\\n$out: in2' > in.d && for file in $out; do cp in1 $$file; done\n" +" deps = gcc\n" +" depfile = in.d\n" +"build out1 out2: cp_multi_gcc in1 in2\n")); + + std::string err; + EXPECT_TRUE(builder_.AddTarget("out1", &err)); + ASSERT_EQ("", err); + fs_.Create("in.d", "out1 out2: in1\nout1 out2: in2"); + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); + ASSERT_EQ(1u, command_runner_.commands_ran_.size()); + EXPECT_EQ("echo 'out1 out2: in1\\nout1 out2: in2' > in.d && for file in out1 out2; do cp in1 $file; done", command_runner_.commands_ran_[0]); + + Node* out1_node = state_.LookupNode("out1"); + DepsLog::Deps* out1_deps = log_.GetDeps(out1_node); + EXPECT_EQ(2, out1_deps->node_count); + EXPECT_EQ("in1", out1_deps->nodes[0]->path()); + EXPECT_EQ("in2", out1_deps->nodes[1]->path()); + + Node* out2_node = state_.LookupNode("out2"); + DepsLog::Deps* out2_deps = log_.GetDeps(out2_node); + EXPECT_EQ(2, out2_deps->node_count); + EXPECT_EQ("in1", out2_deps->nodes[0]->path()); + EXPECT_EQ("in2", out2_deps->nodes[1]->path()); +} + +/// Test a GCC-style deps log with multiple outputs using a line per output. +TEST_F(BuildWithQueryDepsLogTest, TwoOutputsDepFileGCCMultiLineOutput) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, +"rule cp_multi_gcc\n" +" command = echo 'out1: $in\\nout2: $in' > in.d && for file in $out; do cp in1 $$file; done\n" +" deps = gcc\n" +" depfile = in.d\n" +"build out1 out2: cp_multi_gcc in1 in2\n")); + + std::string err; + EXPECT_TRUE(builder_.AddTarget("out1", &err)); + ASSERT_EQ("", err); + fs_.Create("in.d", "out1: in1 in2\nout2: in1 in2"); + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); + ASSERT_EQ(1u, command_runner_.commands_ran_.size()); + EXPECT_EQ("echo 'out1: in1 in2\\nout2: in1 in2' > in.d && for file in out1 out2; do cp in1 $file; done", command_runner_.commands_ran_[0]); + + Node* out1_node = state_.LookupNode("out1"); + DepsLog::Deps* out1_deps = log_.GetDeps(out1_node); + EXPECT_EQ(2, out1_deps->node_count); + EXPECT_EQ("in1", out1_deps->nodes[0]->path()); + EXPECT_EQ("in2", out1_deps->nodes[1]->path()); + + Node* out2_node = state_.LookupNode("out2"); + DepsLog::Deps* out2_deps = log_.GetDeps(out2_node); + EXPECT_EQ(2, out2_deps->node_count); + EXPECT_EQ("in1", out2_deps->nodes[0]->path()); + EXPECT_EQ("in2", out2_deps->nodes[1]->path()); +} + +/// Test a GCC-style deps log with multiple outputs mentioning only the main output. +TEST_F(BuildWithQueryDepsLogTest, TwoOutputsDepFileGCCOnlyMainOutput) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, +"rule cp_multi_gcc\n" +" command = echo 'out1: $in' > in.d && for file in $out; do cp in1 $$file; done\n" +" deps = gcc\n" +" depfile = in.d\n" +"build out1 out2: cp_multi_gcc in1 in2\n")); + + std::string err; + EXPECT_TRUE(builder_.AddTarget("out1", &err)); + ASSERT_EQ("", err); + fs_.Create("in.d", "out1: in1 in2"); + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); + ASSERT_EQ(1u, command_runner_.commands_ran_.size()); + EXPECT_EQ("echo 'out1: in1 in2' > in.d && for file in out1 out2; do cp in1 $file; done", command_runner_.commands_ran_[0]); + + Node* out1_node = state_.LookupNode("out1"); + DepsLog::Deps* out1_deps = log_.GetDeps(out1_node); + EXPECT_EQ(2, out1_deps->node_count); + EXPECT_EQ("in1", out1_deps->nodes[0]->path()); + EXPECT_EQ("in2", out1_deps->nodes[1]->path()); + + Node* out2_node = state_.LookupNode("out2"); + DepsLog::Deps* out2_deps = log_.GetDeps(out2_node); + EXPECT_EQ(2, out2_deps->node_count); + EXPECT_EQ("in1", out2_deps->nodes[0]->path()); + EXPECT_EQ("in2", out2_deps->nodes[1]->path()); +} + +/// Test a GCC-style deps log with multiple outputs mentioning only the secondary output. +TEST_F(BuildWithQueryDepsLogTest, TwoOutputsDepFileGCCOnlySecondaryOutput) { + // Note: This ends up short-circuiting the node creation due to the primary + // output not being present, but it should still work. + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, +"rule cp_multi_gcc\n" +" command = echo 'out2: $in' > in.d && for file in $out; do cp in1 $$file; done\n" +" deps = gcc\n" +" depfile = in.d\n" +"build out1 out2: cp_multi_gcc in1 in2\n")); + + std::string err; + EXPECT_TRUE(builder_.AddTarget("out1", &err)); + ASSERT_EQ("", err); + fs_.Create("in.d", "out2: in1 in2"); + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); + ASSERT_EQ(1u, command_runner_.commands_ran_.size()); + EXPECT_EQ("echo 'out2: in1 in2' > in.d && for file in out1 out2; do cp in1 $file; done", command_runner_.commands_ran_[0]); + + Node* out1_node = state_.LookupNode("out1"); + DepsLog::Deps* out1_deps = log_.GetDeps(out1_node); + EXPECT_EQ(2, out1_deps->node_count); + EXPECT_EQ("in1", out1_deps->nodes[0]->path()); + EXPECT_EQ("in2", out1_deps->nodes[1]->path()); + + Node* out2_node = state_.LookupNode("out2"); + DepsLog::Deps* out2_deps = log_.GetDeps(out2_node); + EXPECT_EQ(2, out2_deps->node_count); + EXPECT_EQ("in1", out2_deps->nodes[0]->path()); + EXPECT_EQ("in2", out2_deps->nodes[1]->path()); +} + /// Tests of builds involving deps logs necessarily must span /// multiple builds. We reuse methods on BuildTest but not the /// builder_ it sets up, because we want pristine objects for diff --git a/src/depfile_parser.cc b/src/depfile_parser.cc index 6faeac6..e92584e 100644 --- a/src/depfile_parser.cc +++ b/src/depfile_parser.cc @@ -16,6 +16,8 @@ #include "depfile_parser.h" #include "util.h" +#include + DepfileParser::DepfileParser(DepfileParserOptions options) : options_(options) { @@ -48,9 +50,6 @@ bool DepfileParser::Parse(string* content, string* err) { char* in = &(*content)[0]; char* end = in + content->size(); bool have_target = false; - bool have_secondary_target_on_this_rule = false; - bool have_newline_since_primary_target = false; - bool warned_distinct_target_lines = false; bool parsing_targets = true; while (in < end) { bool have_newline = false; @@ -294,41 +293,23 @@ yy28: } if (len > 0) { - if (is_dependency) { - if (have_secondary_target_on_this_rule) { - if (!have_newline_since_primary_target) { - *err = "depfile has multiple output paths"; - return false; - } else if (options_.depfile_distinct_target_lines_action_ == - kDepfileDistinctTargetLinesActionError) { - *err = - "depfile has multiple output paths (on separate lines)" - " [-w depfilemulti=err]"; - return false; - } else { - if (!warned_distinct_target_lines) { - warned_distinct_target_lines = true; - Warning("depfile has multiple output paths (on separate lines); " - "continuing anyway [-w depfilemulti=warn]"); - } - continue; - } + StringPiece piece = StringPiece(filename, len); + // If we've seen this as an input before, skip it. + if (std::find(ins_.begin(), ins_.end(), piece) == ins_.end()) { + if (is_dependency) { + // New input. + ins_.push_back(piece); + } else { + // Check for a new output. + if (std::find(outs_.begin(), outs_.end(), piece) == outs_.end()) + outs_.push_back(piece); } - ins_.push_back(StringPiece(filename, len)); - } else if (!out_.str_) { - out_ = StringPiece(filename, len); - } else if (out_ != StringPiece(filename, len)) { - have_secondary_target_on_this_rule = true; } } if (have_newline) { // A newline ends a rule so the next filename will be a new target. parsing_targets = true; - have_secondary_target_on_this_rule = false; - if (have_target) { - have_newline_since_primary_target = true; - } } } if (!have_target) { diff --git a/src/depfile_parser.h b/src/depfile_parser.h index be20374..11b1228 100644 --- a/src/depfile_parser.h +++ b/src/depfile_parser.h @@ -21,17 +21,8 @@ using namespace std; #include "string_piece.h" -enum DepfileDistinctTargetLinesAction { - kDepfileDistinctTargetLinesActionWarn, - kDepfileDistinctTargetLinesActionError, -}; - struct DepfileParserOptions { - DepfileParserOptions() - : depfile_distinct_target_lines_action_( - kDepfileDistinctTargetLinesActionWarn) {} - DepfileDistinctTargetLinesAction - depfile_distinct_target_lines_action_; + DepfileParserOptions() {} }; /// Parser for the dependency information emitted by gcc's -M flags. @@ -44,7 +35,7 @@ struct DepfileParser { /// pointers within it. bool Parse(string* content, string* err); - StringPiece out_; + std::vector outs_; vector ins_; DepfileParserOptions options_; }; diff --git a/src/depfile_parser.in.cc b/src/depfile_parser.in.cc index 735a0c3..eba892f 100644 --- a/src/depfile_parser.in.cc +++ b/src/depfile_parser.in.cc @@ -15,6 +15,8 @@ #include "depfile_parser.h" #include "util.h" +#include + DepfileParser::DepfileParser(DepfileParserOptions options) : options_(options) { @@ -47,9 +49,6 @@ bool DepfileParser::Parse(string* content, string* err) { char* in = &(*content)[0]; char* end = in + content->size(); bool have_target = false; - bool have_secondary_target_on_this_rule = false; - bool have_newline_since_primary_target = false; - bool warned_distinct_target_lines = false; bool parsing_targets = true; while (in < end) { bool have_newline = false; @@ -146,41 +145,23 @@ bool DepfileParser::Parse(string* content, string* err) { } if (len > 0) { - if (is_dependency) { - if (have_secondary_target_on_this_rule) { - if (!have_newline_since_primary_target) { - *err = "depfile has multiple output paths"; - return false; - } else if (options_.depfile_distinct_target_lines_action_ == - kDepfileDistinctTargetLinesActionError) { - *err = - "depfile has multiple output paths (on separate lines)" - " [-w depfilemulti=err]"; - return false; - } else { - if (!warned_distinct_target_lines) { - warned_distinct_target_lines = true; - Warning("depfile has multiple output paths (on separate lines); " - "continuing anyway [-w depfilemulti=warn]"); - } - continue; - } + StringPiece piece = StringPiece(filename, len); + // If we've seen this as an input before, skip it. + if (std::find(ins_.begin(), ins_.end(), piece) == ins_.end()) { + if (is_dependency) { + // New input. + ins_.push_back(piece); + } else { + // Check for a new output. + if (std::find(outs_.begin(), outs_.end(), piece) == outs_.end()) + outs_.push_back(piece); } - ins_.push_back(StringPiece(filename, len)); - } else if (!out_.str_) { - out_ = StringPiece(filename, len); - } else if (out_ != StringPiece(filename, len)) { - have_secondary_target_on_this_rule = true; } } if (have_newline) { // A newline ends a rule so the next filename will be a new target. parsing_targets = true; - have_secondary_target_on_this_rule = false; - if (have_target) { - have_newline_since_primary_target = true; - } } } if (!have_target) { diff --git a/src/depfile_parser_test.cc b/src/depfile_parser_test.cc index 19224f3..e5e3038 100644 --- a/src/depfile_parser_test.cc +++ b/src/depfile_parser_test.cc @@ -34,7 +34,8 @@ TEST_F(DepfileParserTest, Basic) { "build/ninja.o: ninja.cc ninja.h eval_env.h manifest_parser.h\n", &err)); ASSERT_EQ("", err); - EXPECT_EQ("build/ninja.o", parser_.out_.AsString()); + ASSERT_EQ(1u, parser_.outs_.size()); + EXPECT_EQ("build/ninja.o", parser_.outs_[0].AsString()); EXPECT_EQ(4u, parser_.ins_.size()); } @@ -54,7 +55,8 @@ TEST_F(DepfileParserTest, Continuation) { " bar.h baz.h\n", &err)); ASSERT_EQ("", err); - EXPECT_EQ("foo.o", parser_.out_.AsString()); + ASSERT_EQ(1u, parser_.outs_.size()); + EXPECT_EQ("foo.o", parser_.outs_[0].AsString()); EXPECT_EQ(2u, parser_.ins_.size()); } @@ -65,7 +67,8 @@ TEST_F(DepfileParserTest, CarriageReturnContinuation) { " bar.h baz.h\r\n", &err)); ASSERT_EQ("", err); - EXPECT_EQ("foo.o", parser_.out_.AsString()); + ASSERT_EQ(1u, parser_.outs_.size()); + EXPECT_EQ("foo.o", parser_.outs_[0].AsString()); EXPECT_EQ(2u, parser_.ins_.size()); } @@ -79,8 +82,9 @@ TEST_F(DepfileParserTest, BackSlashes) { " Project\\Thing\\Bar.tlb \\\n", &err)); ASSERT_EQ("", err); + ASSERT_EQ(1u, parser_.outs_.size()); EXPECT_EQ("Project\\Dir\\Build\\Release8\\Foo\\Foo.res", - parser_.out_.AsString()); + parser_.outs_[0].AsString()); EXPECT_EQ(4u, parser_.ins_.size()); } @@ -90,8 +94,9 @@ TEST_F(DepfileParserTest, Spaces) { "a\\ bc\\ def: a\\ b c d", &err)); ASSERT_EQ("", err); + ASSERT_EQ(1u, parser_.outs_.size()); EXPECT_EQ("a bc def", - parser_.out_.AsString()); + parser_.outs_[0].AsString()); ASSERT_EQ(3u, parser_.ins_.size()); EXPECT_EQ("a b", parser_.ins_[0].AsString()); @@ -111,8 +116,9 @@ TEST_F(DepfileParserTest, MultipleBackslashes) { "a\\ b\\#c.h: \\\\\\\\\\ \\\\\\\\ \\\\share\\info\\\\#1", &err)); ASSERT_EQ("", err); + ASSERT_EQ(1u, parser_.outs_.size()); EXPECT_EQ("a b#c.h", - parser_.out_.AsString()); + parser_.outs_[0].AsString()); ASSERT_EQ(3u, parser_.ins_.size()); EXPECT_EQ("\\\\ ", parser_.ins_[0].AsString()); @@ -130,8 +136,9 @@ TEST_F(DepfileParserTest, Escapes) { "\\!\\@\\#$$\\%\\^\\&\\[\\]\\\\:", &err)); ASSERT_EQ("", err); + ASSERT_EQ(1u, parser_.outs_.size()); EXPECT_EQ("\\!\\@#$\\%\\^\\&\\[\\]\\\\", - parser_.out_.AsString()); + parser_.outs_[0].AsString()); ASSERT_EQ(0u, parser_.ins_.size()); } @@ -147,8 +154,9 @@ TEST_F(DepfileParserTest, SpecialChars) { " a[1]b@2%c", &err)); ASSERT_EQ("", err); + ASSERT_EQ(1u, parser_.outs_.size()); EXPECT_EQ("C:/Program Files (x86)/Microsoft crtdefs.h", - parser_.out_.AsString()); + parser_.outs_[0].AsString()); ASSERT_EQ(5u, parser_.ins_.size()); EXPECT_EQ("en@quot.header~", parser_.ins_[0].AsString()); @@ -166,18 +174,25 @@ TEST_F(DepfileParserTest, UnifyMultipleOutputs) { // check that multiple duplicate targets are properly unified string err; EXPECT_TRUE(Parse("foo foo: x y z", &err)); - ASSERT_EQ("foo", parser_.out_.AsString()); + ASSERT_EQ(1u, parser_.outs_.size()); + ASSERT_EQ("foo", parser_.outs_[0].AsString()); ASSERT_EQ(3u, parser_.ins_.size()); EXPECT_EQ("x", parser_.ins_[0].AsString()); EXPECT_EQ("y", parser_.ins_[1].AsString()); EXPECT_EQ("z", parser_.ins_[2].AsString()); } -TEST_F(DepfileParserTest, RejectMultipleDifferentOutputs) { - // check that multiple different outputs are rejected by the parser +TEST_F(DepfileParserTest, MultipleDifferentOutputs) { + // check that multiple different outputs are accepted by the parser string err; - EXPECT_FALSE(Parse("foo bar: x y z", &err)); - ASSERT_EQ("depfile has multiple output paths", err); + EXPECT_TRUE(Parse("foo bar: x y z", &err)); + ASSERT_EQ(2u, parser_.outs_.size()); + ASSERT_EQ("foo", parser_.outs_[0].AsString()); + ASSERT_EQ("bar", parser_.outs_[1].AsString()); + ASSERT_EQ(3u, parser_.ins_.size()); + EXPECT_EQ("x", parser_.ins_[0].AsString()); + EXPECT_EQ("y", parser_.ins_[1].AsString()); + EXPECT_EQ("z", parser_.ins_[2].AsString()); } TEST_F(DepfileParserTest, MultipleEmptyRules) { @@ -185,7 +200,8 @@ TEST_F(DepfileParserTest, MultipleEmptyRules) { EXPECT_TRUE(Parse("foo: x\n" "foo: \n" "foo:\n", &err)); - ASSERT_EQ("foo", parser_.out_.AsString()); + ASSERT_EQ(1u, parser_.outs_.size()); + ASSERT_EQ("foo", parser_.outs_[0].AsString()); ASSERT_EQ(1u, parser_.ins_.size()); EXPECT_EQ("x", parser_.ins_[0].AsString()); } @@ -196,7 +212,8 @@ TEST_F(DepfileParserTest, UnifyMultipleRulesLF) { "foo: y\n" "foo \\\n" "foo: z\n", &err)); - ASSERT_EQ("foo", parser_.out_.AsString()); + ASSERT_EQ(1u, parser_.outs_.size()); + ASSERT_EQ("foo", parser_.outs_[0].AsString()); ASSERT_EQ(3u, parser_.ins_.size()); EXPECT_EQ("x", parser_.ins_[0].AsString()); EXPECT_EQ("y", parser_.ins_[1].AsString()); @@ -209,7 +226,8 @@ TEST_F(DepfileParserTest, UnifyMultipleRulesCRLF) { "foo: y\r\n" "foo \\\r\n" "foo: z\r\n", &err)); - ASSERT_EQ("foo", parser_.out_.AsString()); + ASSERT_EQ(1u, parser_.outs_.size()); + ASSERT_EQ("foo", parser_.outs_[0].AsString()); ASSERT_EQ(3u, parser_.ins_.size()); EXPECT_EQ("x", parser_.ins_[0].AsString()); EXPECT_EQ("y", parser_.ins_[1].AsString()); @@ -222,7 +240,8 @@ TEST_F(DepfileParserTest, UnifyMixedRulesLF) { " y\n" "foo \\\n" "foo: z\n", &err)); - ASSERT_EQ("foo", parser_.out_.AsString()); + ASSERT_EQ(1u, parser_.outs_.size()); + ASSERT_EQ("foo", parser_.outs_[0].AsString()); ASSERT_EQ(3u, parser_.ins_.size()); EXPECT_EQ("x", parser_.ins_[0].AsString()); EXPECT_EQ("y", parser_.ins_[1].AsString()); @@ -235,7 +254,8 @@ TEST_F(DepfileParserTest, UnifyMixedRulesCRLF) { " y\r\n" "foo \\\r\n" "foo: z\r\n", &err)); - ASSERT_EQ("foo", parser_.out_.AsString()); + ASSERT_EQ(1u, parser_.outs_.size()); + ASSERT_EQ("foo", parser_.outs_[0].AsString()); ASSERT_EQ(3u, parser_.ins_.size()); EXPECT_EQ("x", parser_.ins_[0].AsString()); EXPECT_EQ("y", parser_.ins_[1].AsString()); @@ -247,7 +267,8 @@ TEST_F(DepfileParserTest, IndentedRulesLF) { EXPECT_TRUE(Parse(" foo: x\n" " foo: y\n" " foo: z\n", &err)); - ASSERT_EQ("foo", parser_.out_.AsString()); + ASSERT_EQ(1u, parser_.outs_.size()); + ASSERT_EQ("foo", parser_.outs_[0].AsString()); ASSERT_EQ(3u, parser_.ins_.size()); EXPECT_EQ("x", parser_.ins_[0].AsString()); EXPECT_EQ("y", parser_.ins_[1].AsString()); @@ -259,7 +280,8 @@ TEST_F(DepfileParserTest, IndentedRulesCRLF) { EXPECT_TRUE(Parse(" foo: x\r\n" " foo: y\r\n" " foo: z\r\n", &err)); - ASSERT_EQ("foo", parser_.out_.AsString()); + ASSERT_EQ(1u, parser_.outs_.size()); + ASSERT_EQ("foo", parser_.outs_[0].AsString()); ASSERT_EQ(3u, parser_.ins_.size()); EXPECT_EQ("x", parser_.ins_[0].AsString()); EXPECT_EQ("y", parser_.ins_[1].AsString()); @@ -272,7 +294,8 @@ TEST_F(DepfileParserTest, TolerateMP) { "x:\n" "y:\n" "z:\n", &err)); - ASSERT_EQ("foo", parser_.out_.AsString()); + ASSERT_EQ(1u, parser_.outs_.size()); + ASSERT_EQ("foo", parser_.outs_[0].AsString()); ASSERT_EQ(3u, parser_.ins_.size()); EXPECT_EQ("x", parser_.ins_[0].AsString()); EXPECT_EQ("y", parser_.ins_[1].AsString()); @@ -287,25 +310,25 @@ TEST_F(DepfileParserTest, MultipleRulesTolerateMP) { "y:\n" "foo: z\n" "z:\n", &err)); - ASSERT_EQ("foo", parser_.out_.AsString()); + ASSERT_EQ(1u, parser_.outs_.size()); + ASSERT_EQ("foo", parser_.outs_[0].AsString()); ASSERT_EQ(3u, parser_.ins_.size()); EXPECT_EQ("x", parser_.ins_[0].AsString()); EXPECT_EQ("y", parser_.ins_[1].AsString()); EXPECT_EQ("z", parser_.ins_[2].AsString()); } -TEST_F(DepfileParserTest, MultipleRulesRejectDifferentOutputs) { - // check that multiple different outputs are rejected by the parser +TEST_F(DepfileParserTest, MultipleRulesDifferentOutputs) { + // check that multiple different outputs are accepted by the parser // when spread across multiple rules - DepfileParserOptions parser_opts; - parser_opts.depfile_distinct_target_lines_action_ = - kDepfileDistinctTargetLinesActionError; - DepfileParser parser(parser_opts); string err; - string input = - "foo: x y\n" - "bar: y z\n"; - EXPECT_FALSE(parser.Parse(&input, &err)); - ASSERT_EQ("depfile has multiple output paths (on separate lines)" - " [-w depfilemulti=err]", err); + EXPECT_TRUE(Parse("foo: x y\n" + "bar: y z\n", &err)); + ASSERT_EQ(2u, parser_.outs_.size()); + ASSERT_EQ("foo", parser_.outs_[0].AsString()); + ASSERT_EQ("bar", parser_.outs_[1].AsString()); + ASSERT_EQ(3u, parser_.ins_.size()); + EXPECT_EQ("x", parser_.ins_[0].AsString()); + EXPECT_EQ("y", parser_.ins_[1].AsString()); + EXPECT_EQ("z", parser_.ins_[2].AsString()); } diff --git a/src/graph.cc b/src/graph.cc index 3214513..58a4630 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -14,6 +14,7 @@ #include "graph.h" +#include #include #include @@ -511,6 +512,17 @@ bool ImplicitDepLoader::LoadDeps(Edge* edge, string* err) { return true; } +struct matches { + matches(std::vector::iterator i) : i_(i) {} + + bool operator()(const Node* node) const { + StringPiece opath = StringPiece(node->path()); + return *i_ == opath; + } + + std::vector::iterator i_; +}; + bool ImplicitDepLoader::LoadDepFile(Edge* edge, const string& path, string* err) { METRIC_RECORD("depfile load"); @@ -541,9 +553,15 @@ bool ImplicitDepLoader::LoadDepFile(Edge* edge, const string& path, return false; } + if (depfile.outs_.empty()) { + *err = path + ": no outputs declared"; + return false; + } + uint64_t unused; - if (!CanonicalizePath(const_cast(depfile.out_.str_), - &depfile.out_.len_, &unused, err)) { + std::vector::iterator primary_out = depfile.outs_.begin(); + if (!CanonicalizePath(const_cast(primary_out->str_), + &primary_out->len_, &unused, err)) { *err = path + ": " + *err; return false; } @@ -552,12 +570,22 @@ bool ImplicitDepLoader::LoadDepFile(Edge* edge, const string& path, // mark the edge as dirty. Node* first_output = edge->outputs_[0]; StringPiece opath = StringPiece(first_output->path()); - if (opath != depfile.out_) { + if (opath != *primary_out) { EXPLAIN("expected depfile '%s' to mention '%s', got '%s'", path.c_str(), - first_output->path().c_str(), depfile.out_.AsString().c_str()); + first_output->path().c_str(), primary_out->AsString().c_str()); return false; } + // Ensure that all mentioned outputs are outputs of the edge. + for (std::vector::iterator o = depfile.outs_.begin(); + o != depfile.outs_.end(); ++o) { + matches m(o); + if (std::find_if(edge->outputs_.begin(), edge->outputs_.end(), m) == edge->outputs_.end()) { + *err = path + ": depfile mentions '" + o->AsString() + "' as an output, but no such output was declared"; + return false; + } + } + // Preallocate space in edge->inputs_ to be filled in below. vector::iterator implicit_dep = PreallocateSpace(edge, depfile.ins_.size()); diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc index e28be2f..bb53dc2 100644 --- a/src/manifest_parser.cc +++ b/src/manifest_parser.cc @@ -379,14 +379,6 @@ bool ManifestParser::ParseEdge(string* err) { } } - // Multiple outputs aren't (yet?) supported with depslog. - string deps_type = edge->GetBinding("deps"); - if (!deps_type.empty() && edge->outputs_.size() > 1) { - return lexer_.Error("multiple outputs aren't (yet?) supported by depslog; " - "bring this up on the mailing list if it affects you", - err); - } - // Lookup, validate, and save any dyndep binding. It will be used later // to load generated dependency information dynamically, but it must // be one of our manifest-specified inputs. diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc index f2b7467..f4aee2d 100644 --- a/src/manifest_parser_test.cc +++ b/src/manifest_parser_test.cc @@ -858,11 +858,10 @@ TEST_F(ParserTest, MultipleOutputsWithDeps) { State local_state; ManifestParser parser(&local_state, NULL); string err; - EXPECT_FALSE(parser.ParseTest("rule cc\n command = foo\n deps = gcc\n" + EXPECT_TRUE(parser.ParseTest("rule cc\n command = foo\n deps = gcc\n" "build a.o b.o: cc c.cc\n", &err)); - EXPECT_EQ("input:5: multiple outputs aren't (yet?) supported by depslog; " - "bring this up on the mailing list if it affects you\n", err); + EXPECT_EQ("", err); } TEST_F(ParserTest, SubNinja) { diff --git a/src/ninja.cc b/src/ninja.cc index c24f09d..6dadb44 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -73,10 +73,6 @@ struct Options { /// Whether phony cycles should warn or print an error. bool phony_cycle_should_err; - - /// Whether a depfile with multiple targets on separate lines should - /// warn or print an error. - bool depfile_distinct_target_lines_should_err; }; /// The Ninja main() loads up a series of data structures; various tools need @@ -989,7 +985,6 @@ bool WarningEnable(const string& name, Options* options) { printf("warning flags:\n" " dupbuild={err,warn} multiple build lines for one target\n" " phonycycle={err,warn} phony build statement references itself\n" -" depfilemulti={err,warn} depfile has multiple output paths on separate lines\n" ); return false; } else if (name == "dupbuild=err") { @@ -1004,11 +999,9 @@ bool WarningEnable(const string& name, Options* options) { } else if (name == "phonycycle=warn") { options->phony_cycle_should_err = false; return true; - } else if (name == "depfilemulti=err") { - options->depfile_distinct_target_lines_should_err = true; - return true; - } else if (name == "depfilemulti=warn") { - options->depfile_distinct_target_lines_should_err = false; + } else if (name == "depfilemulti=err" || + name == "depfilemulti=warn") { + Warning("deprecated warning 'depfilemulti'"); return true; } else { const char* suggestion = @@ -1284,11 +1277,6 @@ NORETURN void real_main(int argc, char** argv) { if (exit_code >= 0) exit(exit_code); - if (options.depfile_distinct_target_lines_should_err) { - config.depfile_parser_options.depfile_distinct_target_lines_action_ = - kDepfileDistinctTargetLinesActionError; - } - if (options.working_dir) { // The formatting of this string, complete with funny quotes, is // so Emacs can properly identify that the cwd has changed for -- cgit v1.2.1