Restore depfile toleration of multiple output paths on distinct lines
Prior to introduction of depfile parser handling of multiple rules,
ninja silently accepted a depfile of the form:
out: in1 in2 in3
other: otherIn1 otherIn2 otherIn3
and incorrectly treated `other` and `otherIn*` as additional inputs to
`out`. Now we prefer to reject this just as we already do for a depfile
specifying multiple outputs on one line. However, this can break
existing cases where such a depfile was silently tolerated.
Add a `-w depfilemulti={err,warn}` option to control this behavior,
and make it just a warning by default.
diff --git a/src/build.cc b/src/build.cc
index ed219fd..b392803 100644
--- a/src/build.cc
+++ b/src/build.cc
@@ -557,7 +557,8 @@
BuildLog* build_log, DepsLog* deps_log,
DiskInterface* disk_interface)
: state_(state), config_(config), disk_interface_(disk_interface),
- scan_(state, build_log, deps_log, disk_interface) {
+ scan_(state, build_log, deps_log, disk_interface,
+ &config_.depfile_parser_options) {
status_ = new BuildStatus(config);
}
@@ -906,7 +907,7 @@
if (content.empty())
return true;
- DepfileParser deps;
+ DepfileParser deps(config_.depfile_parser_options);
if (!deps.Parse(&content, err))
return false;
diff --git a/src/build.h b/src/build.h
index 9b90e8a..a42b8d4 100644
--- a/src/build.h
+++ b/src/build.h
@@ -23,6 +23,7 @@
#include <string>
#include <vector>
+#include "depfile_parser.h"
#include "graph.h" // XXX needed for DependencyScan; should rearrange.
#include "exit_status.h"
#include "line_printer.h"
@@ -151,6 +152,7 @@
/// The maximum load average we must not exceed. A negative value
/// means that we do not have any limit.
double max_load_average;
+ DepfileParserOptions depfile_parser_options;
};
/// Builder wraps the build process: starting commands, updating status.
diff --git a/src/depfile_parser.cc b/src/depfile_parser.cc
index 7724eed..405289f 100644
--- a/src/depfile_parser.cc
+++ b/src/depfile_parser.cc
@@ -14,6 +14,12 @@
// limitations under the License.
#include "depfile_parser.h"
+#include "util.h"
+
+DepfileParser::DepfileParser(DepfileParserOptions options)
+ : options_(options)
+{
+}
// A note on backslashes in Makefiles, from reading the docs:
// Backslash-newline is the line continuation character.
@@ -37,6 +43,8 @@
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;
@@ -219,8 +227,23 @@
if (len > 0) {
if (is_dependency) {
if (have_secondary_target_on_this_rule) {
- *err = "depfile has multiple output paths";
- return false;
+ 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;
+ }
}
ins_.push_back(StringPiece(filename, len));
} else if (!out_.str_) {
@@ -234,6 +257,9 @@
// 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 1e6ebb5..be20374 100644
--- a/src/depfile_parser.h
+++ b/src/depfile_parser.h
@@ -21,8 +21,24 @@
#include "string_piece.h"
+enum DepfileDistinctTargetLinesAction {
+ kDepfileDistinctTargetLinesActionWarn,
+ kDepfileDistinctTargetLinesActionError,
+};
+
+struct DepfileParserOptions {
+ DepfileParserOptions()
+ : depfile_distinct_target_lines_action_(
+ kDepfileDistinctTargetLinesActionWarn) {}
+ DepfileDistinctTargetLinesAction
+ depfile_distinct_target_lines_action_;
+};
+
/// Parser for the dependency information emitted by gcc's -M flags.
struct DepfileParser {
+ explicit DepfileParser(DepfileParserOptions options =
+ DepfileParserOptions());
+
/// Parse an input file. Input must be NUL-terminated.
/// Warning: may mutate the content in-place and parsed StringPieces are
/// pointers within it.
@@ -30,6 +46,7 @@
StringPiece out_;
vector<StringPiece> ins_;
+ DepfileParserOptions options_;
};
#endif // NINJA_DEPFILE_PARSER_H_
diff --git a/src/depfile_parser.in.cc b/src/depfile_parser.in.cc
index d299ee2..f8c94b3 100644
--- a/src/depfile_parser.in.cc
+++ b/src/depfile_parser.in.cc
@@ -13,6 +13,12 @@
// limitations under the License.
#include "depfile_parser.h"
+#include "util.h"
+
+DepfileParser::DepfileParser(DepfileParserOptions options)
+ : options_(options)
+{
+}
// A note on backslashes in Makefiles, from reading the docs:
// Backslash-newline is the line continuation character.
@@ -36,6 +42,8 @@
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;
@@ -119,8 +127,23 @@
if (len > 0) {
if (is_dependency) {
if (have_secondary_target_on_this_rule) {
- *err = "depfile has multiple output paths";
- return false;
+ 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;
+ }
}
ins_.push_back(StringPiece(filename, len));
} else if (!out_.str_) {
@@ -134,6 +157,9 @@
// 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 70e4029..52fe7cd 100644
--- a/src/depfile_parser_test.cc
+++ b/src/depfile_parser_test.cc
@@ -276,8 +276,15 @@
TEST_F(DepfileParserTest, MultipleRulesRejectDifferentOutputs) {
// check that multiple different outputs are rejected 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;
- EXPECT_FALSE(Parse("foo: x y\n"
- "bar: y z\n", &err));
- ASSERT_EQ("depfile has multiple output paths", 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);
}
diff --git a/src/disk_interface_test.cc b/src/disk_interface_test.cc
index 2de4f28..bac515d 100644
--- a/src/disk_interface_test.cc
+++ b/src/disk_interface_test.cc
@@ -213,7 +213,7 @@
struct StatTest : public StateTestWithBuiltinRules,
public DiskInterface {
- StatTest() : scan_(&state_, NULL, NULL, this) {}
+ StatTest() : scan_(&state_, NULL, NULL, this, NULL) {}
// DiskInterface implementation.
virtual TimeStamp Stat(const string& path, string* err) const;
diff --git a/src/graph.cc b/src/graph.cc
index b41c247..9c2f784 100644
--- a/src/graph.cc
+++ b/src/graph.cc
@@ -491,7 +491,9 @@
return false;
}
- DepfileParser depfile;
+ DepfileParser depfile(depfile_parser_options_
+ ? *depfile_parser_options_
+ : DepfileParserOptions());
string depfile_err;
if (!depfile.Parse(&content, &depfile_err)) {
*err = path + ": " + depfile_err;
diff --git a/src/graph.h b/src/graph.h
index a8f0641..d58fecd 100644
--- a/src/graph.h
+++ b/src/graph.h
@@ -24,6 +24,7 @@
#include "util.h"
struct BuildLog;
+struct DepfileParserOptions;
struct DiskInterface;
struct DepsLog;
struct Edge;
@@ -209,8 +210,10 @@
/// "depfile" attribute in build files.
struct ImplicitDepLoader {
ImplicitDepLoader(State* state, DepsLog* deps_log,
- DiskInterface* disk_interface)
- : state_(state), disk_interface_(disk_interface), deps_log_(deps_log) {}
+ DiskInterface* disk_interface,
+ DepfileParserOptions const* depfile_parser_options)
+ : state_(state), disk_interface_(disk_interface), deps_log_(deps_log),
+ depfile_parser_options_(depfile_parser_options) {}
/// Load implicit dependencies for \a edge.
/// @return false on error (without filling \a err if info is just missing
@@ -242,6 +245,7 @@
State* state_;
DiskInterface* disk_interface_;
DepsLog* deps_log_;
+ DepfileParserOptions const* depfile_parser_options_;
};
@@ -249,10 +253,11 @@
/// and updating the dirty/outputs_ready state of all the nodes and edges.
struct DependencyScan {
DependencyScan(State* state, BuildLog* build_log, DepsLog* deps_log,
- DiskInterface* disk_interface)
+ DiskInterface* disk_interface,
+ DepfileParserOptions const* depfile_parser_options)
: build_log_(build_log),
disk_interface_(disk_interface),
- dep_loader_(state, deps_log, disk_interface) {}
+ dep_loader_(state, deps_log, disk_interface, depfile_parser_options) {}
/// Update the |dirty_| state of the given node by inspecting its input edge.
/// Examine inputs, outputs, and command lines to judge whether an edge
diff --git a/src/graph_test.cc b/src/graph_test.cc
index 422bc9a..4a66831 100644
--- a/src/graph_test.cc
+++ b/src/graph_test.cc
@@ -18,7 +18,7 @@
#include "test.h"
struct GraphTest : public StateTestWithBuiltinRules {
- GraphTest() : scan_(&state_, NULL, NULL, &fs_) {}
+ GraphTest() : scan_(&state_, NULL, NULL, &fs_, NULL) {}
VirtualFileSystem fs_;
DependencyScan scan_;
diff --git a/src/ninja.cc b/src/ninja.cc
index a3d73ad..b608426 100644
--- a/src/ninja.cc
+++ b/src/ninja.cc
@@ -73,6 +73,10 @@
/// 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
@@ -912,7 +916,9 @@
if (name == "list") {
printf("warning flags:\n"
" dupbuild={err,warn} multiple build lines for one target\n"
-" phonycycle={err,warn} phony build statement references itself\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") {
options->dupe_edges_should_err = true;
@@ -926,6 +932,12 @@
} 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;
+ return true;
} else {
const char* suggestion =
SpellcheckString(name.c_str(), "dupbuild=err", "dupbuild=warn",
@@ -1200,6 +1212,11 @@
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
