Improve incremental build: Make outputs modified outside of the build system considered dirty

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() {
@@ -793,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;
     }
   }
 

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

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,6 +1456,93 @@ TEST_F(BuildWithLogTest, RebuildWithNoInputs) {
   EXPECT_EQ(1u, command_runner_.commands_ran_.size());
 }
 
+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"));
+
+  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(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"

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;

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 (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
+        // 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.

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_

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_;

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;