add a straightforward deps log test, fix the other one

The first test I wrote was wrong; write a simpler test that exercises
the "no failures" code paths, then fix the second test and the bugs it
exposed.

3 files changed, 100 insertions, 17 deletions

diff --git a/src/build.cc b/src/build.cc
index 5e874df..7de4b16 100644
--- a/src/build.cc
+++ b/src/build.cc
@@ -801,10 +801,12 @@ void Builder::FinishCommand(CommandRunner::Result* result) {
   // can fail, which makes the command fail from a build perspective.
 
   vector<Node*> deps_nodes;
+  TimeStamp deps_mtime = 0;
   string deps_type = edge->GetBinding("deps");
   if (result->success() && !deps_type.empty()) {
     string extract_err;
-    if (!ExtractDeps(result, deps_type, &deps_nodes, &extract_err)) {
+    if (!ExtractDeps(result, deps_type, &deps_nodes, &deps_mtime,
+                     &extract_err)) {
       if (!result->output.empty())
         result->output.append("\n");
       result->output.append(extract_err);
@@ -875,10 +877,12 @@ void Builder::FinishCommand(CommandRunner::Result* result) {
   if (!deps_type.empty()) {
     assert(edge->outputs_.size() == 1 && "should have been rejected by parser");
     Node* out = edge->outputs_[0];
-    TimeStamp mtime = disk_interface_->Stat(out->path());
-    // XXX we could reuse the restat logic to avoid a second stat,
-    // but in practice we only care about the single output.
-    scan_.deps_log()->RecordDeps(out, mtime, deps_nodes);
+    if (!deps_mtime) {
+      // On Windows there's no separate file to compare against; just reuse
+      // the output's mtime.
+      deps_mtime = disk_interface_->Stat(out->path());
+    }
+    scan_.deps_log()->RecordDeps(out, deps_mtime, deps_nodes);
   }
 
 }
@@ -886,6 +890,7 @@ void Builder::FinishCommand(CommandRunner::Result* result) {
 bool Builder::ExtractDeps(CommandRunner::Result* result,
                           const string& deps_type,
                           vector<Node*>* deps_nodes,
+                          TimeStamp* deps_mtime,
                           string* err) {
 #ifdef _WIN32
   if (deps_type == "msvc") {
@@ -900,7 +905,13 @@ bool Builder::ExtractDeps(CommandRunner::Result* result,
   if (deps_type == "gcc") {
     string depfile = result->edge->GetBinding("depfile");
     if (depfile.empty()) {
-      *err = string("edge with deps=gcc but no depfile makes no sense\n");
+      *err = string("edge with deps=gcc but no depfile makes no sense");
+      return false;
+    }
+
+    *deps_mtime = disk_interface_->Stat(depfile);
+    if (*deps_mtime <= 0) {
+      *err = string("unable to read depfile");
       return false;
     }
 
diff --git a/src/build.h b/src/build.h
index fb5fd10..d5f01cd 100644
--- a/src/build.h
+++ b/src/build.h
@@ -177,7 +177,8 @@ struct Builder {
 
  private:
   bool ExtractDeps(CommandRunner::Result* result, const string& deps_type,
-                   vector<Node*>* deps_nodes, string* err);
+                   vector<Node*>* deps_nodes, TimeStamp* deps_mtime,
+                   string* err);
 
   DiskInterface* disk_interface_;
   DependencyScan scan_;
diff --git a/src/build_test.cc b/src/build_test.cc
index 7df742f..2a0fa0f 100644
--- a/src/build_test.cc
+++ b/src/build_test.cc
@@ -1388,7 +1388,7 @@ TEST_F(BuildTest, FailedDepsParse) {
 
   // These deps will fail to parse, as they should only have one
   // path to the left of the colon.
-  fs_.Create("in1.d", "XXX YYY");
+  fs_.Create("in1.d", "AAA BBB");
 
   EXPECT_FALSE(builder_.Build(&err));
   EXPECT_EQ("subcommand failed", err);
@@ -1417,10 +1417,8 @@ struct BuildWithDepsLogTest : public BuildTest {
   void* builder_;
 };
 
-TEST_F(BuildWithDepsLogTest, ObsoleteDeps) {
-  // Don't make use of the class's built-in Builder etc. so that we
-  // can construct objects with the DepsLog in place.
-
+/// Run a straightforwad build where the deps log is used.
+TEST_F(BuildWithDepsLogTest, Straightforward) {
   string err;
   // Note: in1 was created by the superclass SetUp().
   const char* manifest =
@@ -1447,6 +1445,8 @@ TEST_F(BuildWithDepsLogTest, ObsoleteDeps) {
 
     // The deps file should have been removed.
     EXPECT_EQ(0, fs_.Stat("in1.d"));
+    // Recreate it for the next step.
+    fs_.Create("in1.d", "out: in2");
     deps_log.Close();
     builder.command_runner_.release();
   }
@@ -1456,12 +1456,9 @@ TEST_F(BuildWithDepsLogTest, ObsoleteDeps) {
     ASSERT_NO_FATAL_FAILURE(AddCatRule(&state));
     ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
 
-    // Pretend that the build aborted before the deps were
-    // removed, leaving behind an obsolete .d file, but after
-    // the output was written.
-    fs_.Create("in1.d", "XXX");
+    // Touch the file only mentioned in the deps.
     fs_.Tick();
-    fs_.Create("out", "");
+    fs_.Create("in2", "");
 
     // Run the build again.
     DepsLog deps_log;
@@ -1476,6 +1473,80 @@ TEST_F(BuildWithDepsLogTest, ObsoleteDeps) {
     EXPECT_TRUE(builder.Build(&err));
     EXPECT_EQ("", err);
 
+    // We should have rebuilt the output due to in2 being
+    // out of date.
+    EXPECT_EQ(1u, command_runner_.commands_ran_.size());
+
+    builder.command_runner_.release();
+  }
+}
+
+/// Verify that obsolete deps still cause a rebuild.
+TEST_F(BuildWithDepsLogTest, ObsoleteDeps) {
+  string err;
+  // Note: in1 was created by the superclass SetUp().
+  const char* manifest =
+      "build out: cat in1\n"
+      "  deps = gcc\n"
+      "  depfile = in1.d\n";
+  {
+    // Create the obsolete deps, then run a build to incorporate them.
+    // The idea is that the inputs/outputs are newer than the logged
+    // deps.
+    fs_.Create("in1.d", "out: ");
+    fs_.Tick();
+
+    fs_.Create("in1", "");
+
+    State state;
+    ASSERT_NO_FATAL_FAILURE(AddCatRule(&state));
+    ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
+
+    // Run the build once, everything should be ok.
+    DepsLog deps_log;
+    ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
+    ASSERT_EQ("", err);
+
+    Builder builder(&state, config_, NULL, &deps_log, &fs_);
+    builder.command_runner_.reset(&command_runner_);
+    EXPECT_TRUE(builder.AddTarget("out", &err));
+    ASSERT_EQ("", err);
+    EXPECT_TRUE(builder.Build(&err));
+    EXPECT_EQ("", err);
+
+    fs_.Create("out", "");
+    // The deps file should have been removed.
+    EXPECT_EQ(0, fs_.Stat("in1.d"));
+    deps_log.Close();
+    builder.command_runner_.release();
+  }
+
+  // Now we should be in a situation where in1/out2 both have recent
+  // timestamps but the deps are old.  Verify we rebuild.
+  fs_.Tick();
+
+  {
+    State state;
+    ASSERT_NO_FATAL_FAILURE(AddCatRule(&state));
+    ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
+
+    DepsLog deps_log;
+    ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
+    ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
+
+    Builder builder(&state, config_, NULL, &deps_log, &fs_);
+    builder.command_runner_.reset(&command_runner_);
+    command_runner_.commands_ran_.clear();
+    EXPECT_TRUE(builder.AddTarget("out", &err));
+    ASSERT_EQ("", err);
+
+    // Recreate the deps here just to prove the old recorded deps are
+    // the problem.
+    fs_.Create("in1.d", "out: ");
+
+    EXPECT_TRUE(builder.Build(&err));
+    EXPECT_EQ("", err);
+
     // We should have rebuilt the output due to the deps being
     // out of date.
     EXPECT_EQ(1u, command_runner_.commands_ran_.size());