From 6ef944ea6fdacb9a51afdb9e1ebb42f55fa44bfe Mon Sep 17 00:00:00 2001 From: Marc Delorme Date: Mon, 29 Mar 2021 21:00:31 +0900 Subject: [PATCH 1/3] Make outputs modified outside of the build system considered dirty In the case an output mtime is more recent than its log entry, the output should be considered dirty since its state is not equal to the state it would have if the rule command was executed again. --- src/build_test.cc | 27 ++++++++++++++++++++++++++- src/graph.cc | 14 ++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/build_test.cc b/src/build_test.cc index e0c43b11af..fecfdb5169 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -1446,6 +1446,32 @@ TEST_F(BuildWithLogTest, RebuildWithNoInputs) { EXPECT_EQ(1u, command_runner_.commands_ran_.size()); } +TEST_F(BuildWithLogTest, RebuildIfOutputIsModified) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, +"build out: cat in\n")); + + string err; + + fs_.Create("in", ""); + + EXPECT_TRUE(builder_.AddTarget("out", &err)); + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); + EXPECT_EQ(1u, command_runner_.commands_ran_.size()); + + command_runner_.commands_ran_.clear(); + state_.Reset(); + + fs_.Tick(); + + fs_.WriteFile("out", ""); + + EXPECT_TRUE(builder_.AddTarget("out", &err)); + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); + EXPECT_EQ(1u, command_runner_.commands_ran_.size()); +} + TEST_F(BuildWithLogTest, RestatTest) { ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, "rule true\n" @@ -1541,7 +1567,6 @@ TEST_F(BuildWithLogTest, RestatMissingFile) { fs_.Tick(); fs_.Create("in", ""); - fs_.Create("out2", ""); // Run a build, expect only the first command to run. // It doesn't touch its output (due to being the "true" command), so diff --git a/src/graph.cc b/src/graph.cc index 822b7c5bcf..5b29ff14f8 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -276,6 +276,20 @@ bool DependencyScan::RecomputeOutputDirty(const Edge* edge, if (build_log()) { bool generator = edge->GetBindingBool("generator"); if (entry || (entry = build_log()->LookupByOutput(output->path()))) { + if (output->mtime() > entry->mtime) { + // May also be dirty due to the mtime in the log being older from the + // actual mtime of the output. This can occur if the output has been + // modified from a process unrelated to the build system. In that case the + // output state does not match anymore the state it would have if rebuilt + // by ninja, so it is dirty. + // + // Note: The mtime in the log can be for recent than the actual mtime. This can + // occur if the edge is mark with 'restat = 1'. If the rule command did not + // change the output, the output log mtime will be inherited from the most + // recent inputs or depfile. + EXPLAIN("recored mtime of %s is different from its actual mtime", output->path().c_str()); + return true; + } if (!generator && BuildLog::LogEntry::HashCommand(command) != entry->command_hash) { // May also be dirty due to the command changing since the last build. From f3c6ed72348d578e311e8c46db7ffd4243a41ebc Mon Sep 17 00:00:00 2001 From: Marc Delorme Date: Thu, 1 Apr 2021 11:01:14 +0900 Subject: [PATCH 2/3] Add an '-m' option to enable 'modified output is considered dirty' --- src/build.cc | 3 ++- src/build.h | 5 +++-- src/build_test.cc | 25 ++++++++++++++++++++++++- src/disk_interface_test.cc | 2 +- src/graph.cc | 2 +- src/graph.h | 7 +++++-- src/graph_test.cc | 2 +- src/ninja.cc | 7 ++++++- 8 files changed, 43 insertions(+), 10 deletions(-) diff --git a/src/build.cc b/src/build.cc index fb5890a76d..87ff50a0d4 100644 --- a/src/build.cc +++ b/src/build.cc @@ -504,7 +504,8 @@ Builder::Builder(State* state, const BuildConfig& config, : state_(state), config_(config), plan_(this), status_(status), start_time_millis_(start_time_millis), disk_interface_(disk_interface), scan_(state, build_log, deps_log, disk_interface, - &config_.depfile_parser_options) { + &config_.depfile_parser_options, + config.modified_output_is_dirty) { } Builder::~Builder() { diff --git a/src/build.h b/src/build.h index 06823c2e50..70741181e2 100644 --- a/src/build.h +++ b/src/build.h @@ -155,8 +155,8 @@ struct CommandRunner { /// Options (e.g. verbosity, parallelism) passed to a build. struct BuildConfig { - BuildConfig() : verbosity(NORMAL), dry_run(false), parallelism(1), - failures_allowed(1), max_load_average(-0.0f) {} + BuildConfig() : verbosity(NORMAL), dry_run(false), modified_output_is_dirty(false), + parallelism(1), failures_allowed(1), max_load_average(-0.0f) {} enum Verbosity { NORMAL, @@ -165,6 +165,7 @@ struct BuildConfig { }; Verbosity verbosity; bool dry_run; + bool modified_output_is_dirty; int parallelism; int failures_allowed; /// The maximum load average we must not exceed. A negative value diff --git a/src/build_test.cc b/src/build_test.cc index fecfdb5169..094cbda8f9 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -494,6 +494,10 @@ struct BuildTest : public StateTestWithBuiltinRules, public BuildLogUser { : config_(MakeConfig()), command_runner_(&fs_), status_(config_), builder_(&state_, config_, NULL, log, &fs_, &status_, 0) {} + explicit BuildTest(const BuildConfig& config) + : config_(config), command_runner_(&fs_), status_(config_), + builder_(&state_, config_, NULL, NULL, &fs_, &status_, 0) {} + virtual void SetUp() { StateTestWithBuiltinRules::SetUp(); @@ -1292,6 +1296,12 @@ struct BuildWithLogTest : public BuildTest { BuildWithLogTest() { builder_.SetBuildLog(&build_log_); } + + explicit BuildWithLogTest(const BuildConfig& config) + : BuildTest(config) + { + builder_.SetBuildLog(&build_log_); + } BuildLog build_log_; }; @@ -1446,7 +1456,19 @@ TEST_F(BuildWithLogTest, RebuildWithNoInputs) { EXPECT_EQ(1u, command_runner_.commands_ran_.size()); } -TEST_F(BuildWithLogTest, RebuildIfOutputIsModified) { +struct BuildWithModifiedOutputDirtyAndLogTest : public BuildWithLogTest { + BuildWithModifiedOutputDirtyAndLogTest() : BuildWithLogTest(MakeConfig()) { + } + + BuildConfig MakeConfig() { + BuildConfig config; + config.verbosity = BuildConfig::QUIET; + config.modified_output_is_dirty = true; + return config; + } +}; + +TEST_F(BuildWithModifiedOutputDirtyAndLogTest, RebuildIfOutputIsModified) { ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, "build out: cat in\n")); @@ -1567,6 +1589,7 @@ TEST_F(BuildWithLogTest, RestatMissingFile) { fs_.Tick(); fs_.Create("in", ""); + fs_.Create("out2", ""); // Run a build, expect only the first command to run. // It doesn't touch its output (due to being the "true" command), so diff --git a/src/disk_interface_test.cc b/src/disk_interface_test.cc index b424243a94..c8e6edede0 100644 --- a/src/disk_interface_test.cc +++ b/src/disk_interface_test.cc @@ -221,7 +221,7 @@ TEST_F(DiskInterfaceTest, RemoveFile) { struct StatTest : public StateTestWithBuiltinRules, public DiskInterface { - StatTest() : scan_(&state_, NULL, NULL, this, NULL) {} + StatTest() : scan_(&state_, NULL, NULL, this, NULL, false) {} // DiskInterface implementation. virtual TimeStamp Stat(const string& path, string* err) const; diff --git a/src/graph.cc b/src/graph.cc index 5b29ff14f8..1af204bc49 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -276,7 +276,7 @@ bool DependencyScan::RecomputeOutputDirty(const Edge* edge, if (build_log()) { bool generator = edge->GetBindingBool("generator"); if (entry || (entry = build_log()->LookupByOutput(output->path()))) { - if (output->mtime() > entry->mtime) { + if (modified_output_is_dirty_ && output->mtime() > entry->mtime) { // May also be dirty due to the mtime in the log being older from the // actual mtime of the output. This can occur if the output has been // modified from a process unrelated to the build system. In that case the diff --git a/src/graph.h b/src/graph.h index bb4f10c479..84cd021e02 100644 --- a/src/graph.h +++ b/src/graph.h @@ -284,11 +284,13 @@ struct ImplicitDepLoader { struct DependencyScan { DependencyScan(State* state, BuildLog* build_log, DepsLog* deps_log, DiskInterface* disk_interface, - DepfileParserOptions const* depfile_parser_options) + DepfileParserOptions const* depfile_parser_options, + bool modified_output_is_dirty) : build_log_(build_log), disk_interface_(disk_interface), dep_loader_(state, deps_log, disk_interface, depfile_parser_options), - dyndep_loader_(state, disk_interface) {} + dyndep_loader_(state, disk_interface), + modified_output_is_dirty_(modified_output_is_dirty) {} /// Update the |dirty_| state of the given node by inspecting its input edge. /// Examine inputs, outputs, and command lines to judge whether an edge @@ -333,6 +335,7 @@ struct DependencyScan { DiskInterface* disk_interface_; ImplicitDepLoader dep_loader_; DyndepLoader dyndep_loader_; + bool modified_output_is_dirty_; }; #endif // NINJA_GRAPH_H_ diff --git a/src/graph_test.cc b/src/graph_test.cc index 6b4bb517e8..12a57513b7 100644 --- a/src/graph_test.cc +++ b/src/graph_test.cc @@ -20,7 +20,7 @@ using namespace std; struct GraphTest : public StateTestWithBuiltinRules { - GraphTest() : scan_(&state_, NULL, NULL, &fs_, NULL) {} + GraphTest() : scan_(&state_, NULL, NULL, &fs_, NULL, false) {} VirtualFileSystem fs_; DependencyScan scan_; diff --git a/src/ninja.cc b/src/ninja.cc index 1cff6e8ca8..3db96d0f02 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -224,6 +224,7 @@ void Usage(const BuildConfig& config) { " -j N run N jobs in parallel (0 means infinity) [default=%d on this system]\n" " -k N keep going until N jobs fail (0 means infinity) [default=1]\n" " -l N do not start new jobs if the load average is greater than N\n" +" -m considered modified output as dirty\n" " -n dry run (don't run commands but act like they succeeded)\n" "\n" " -d MODE enable debugging (use '-d list' to list modes)\n" @@ -1317,7 +1318,7 @@ int ReadFlags(int* argc, char*** argv, int opt; while (!options->tool && - (opt = getopt_long(*argc, *argv, "d:f:j:k:l:nt:vw:C:h", kLongOptions, + (opt = getopt_long(*argc, *argv, "d:f:j:k:l:mnt:vw:C:h", kLongOptions, NULL)) != -1) { switch (opt) { case 'd': @@ -1358,6 +1359,10 @@ int ReadFlags(int* argc, char*** argv, config->max_load_average = value; break; } + case 'm': { + config->modified_output_is_dirty = true; + break; + } case 'n': config->dry_run = true; break; From 4b10b3317da8ea7eda31a93b9034e69026059ec0 Mon Sep 17 00:00:00 2001 From: Marc Delorme Date: Mon, 19 Apr 2021 22:02:36 +0900 Subject: [PATCH 3/3] Fix restat situation when not all outputs are restat --- src/build.cc | 3 ++- src/build_test.cc | 49 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/src/build.cc b/src/build.cc index 87ff50a0d4..db65527bb2 100644 --- a/src/build.cc +++ b/src/build.cc @@ -794,7 +794,8 @@ bool Builder::FinishCommand(CommandRunner::Result* result, string* err) { // of a restat. status_->PlanHasTotalEdges(plan_.command_edge_count()); - output_mtime = restat_mtime; + if (restat_mtime > output_mtime) + output_mtime = restat_mtime; } } diff --git a/src/build_test.cc b/src/build_test.cc index 094cbda8f9..7346a85048 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -1494,6 +1494,55 @@ TEST_F(BuildWithModifiedOutputDirtyAndLogTest, RebuildIfOutputIsModified) { EXPECT_EQ(1u, command_runner_.commands_ran_.size()); } + +TEST_F(BuildWithModifiedOutputDirtyAndLogTest, DoNotRebuildMultipleOutputEdgeWithRestat) { + string err; + + ASSERT_NO_FATAL_FAILURE( + AssertParse(&state_, + "rule true\n" + " command = true $in\n" + " restat = 1\n" + "build out | out.bis: true in\n")); + fs_.Tick(); + fs_.Create("in", ""); + fs_.Tick(); + fs_.Create("out", ""); + fs_.Create("out.bis", ""); + fs_.Tick(); + + EXPECT_TRUE(builder_.AddTarget("out", &err)); + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); + EXPECT_EQ(1u, command_runner_.commands_ran_.size()); + + state_.Reset(); + command_runner_.commands_ran_.clear(); + + fs_.Tick(); + fs_.Create("in", ""); + fs_.Tick(); + fs_.Create("out", ""); + + + EXPECT_TRUE(builder_.AddTarget("out", &err)); + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); + EXPECT_EQ(1u, command_runner_.commands_ran_.size()); + + state_.Reset(); + command_runner_.commands_ran_.clear(); + + // In this situation 'out.bis' should have been restat, its mtime + // will be older than 'in'. 'out' will have its mtime more + // recent than 'in'. We wanna check than ninja will not try to + // rebuild anything it this tricky situation. + EXPECT_TRUE(builder_.AddTarget("out", &err)); + EXPECT_EQ("", err); + EXPECT_TRUE(builder_.AlreadyUpToDate()); + EXPECT_EQ(0u, command_runner_.commands_ran_.size()); +} + TEST_F(BuildWithLogTest, RestatTest) { ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, "rule true\n"