add a test for the "deps out of date" case

It touched the various remaining XXXes in the code, hooray.

7 files changed, 120 insertions, 7 deletions

diff --git a/src/build.cc b/src/build.cc
index 0caf6e7..2d3d9b4 100644
--- a/src/build.cc
+++ b/src/build.cc
@@ -872,11 +872,13 @@ void Builder::FinishCommand(CommandRunner::Result* result) {
                                      restat_mtime);
   }
 
-  if (!deps_type.empty() && scan_.deps_log()) {
+  if (!deps_type.empty()) {
     assert(edge->outputs_.size() == 1);
     Node* out = edge->outputs_[0];
-    // XXX need to restat for restat_mtime.
-    scan_.deps_log()->RecordDeps(out, restat_mtime, deps_nodes);
+    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);
   }
 
 }
diff --git a/src/build_test.cc b/src/build_test.cc
index 1907197..a227854 100644
--- a/src/build_test.cc
+++ b/src/build_test.cc
@@ -15,6 +15,7 @@
 #include "build.h"
 
 #include "build_log.h"
+#include "deps_log.h"
 #include "graph.h"
 #include "test.h"
 
@@ -396,6 +397,11 @@ struct BuildTest : public StateTestWithBuiltinRules {
   BuildTest() : config_(MakeConfig()), command_runner_(&fs_),
                 builder_(&state_, config_, NULL, NULL, &fs_),
                 status_(config_) {
+  }
+
+  virtual void SetUp() {
+    StateTestWithBuiltinRules::SetUp();
+
     builder_.command_runner_.reset(&command_runner_);
     AssertParse(&state_,
 "build cat1: cat in1\n"
@@ -1369,3 +1375,93 @@ TEST_F(BuildTest, FailedDepsParse) {
   EXPECT_FALSE(builder_.Build(&err));
   EXPECT_EQ("subcommand failed", err);
 }
+
+/// 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
+/// each build.
+struct BuildWithDepsLogTest : public BuildTest {
+  BuildWithDepsLogTest() {}
+
+  virtual void SetUp() {
+    BuildTest::SetUp();
+
+    temp_dir_.CreateAndEnter("BuildWithDepsLogTest");
+  }
+
+  virtual void TearDown() {
+    temp_dir_.Cleanup();
+  }
+
+  ScopedTempDir temp_dir_;
+
+  /// Shadow parent class builder_ so we don't accidentally use it.
+  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.
+
+  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";
+  {
+    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);
+    fs_.Create("in1.d", "out: in2");
+    EXPECT_TRUE(builder.Build(&err));
+    EXPECT_EQ("", err);
+
+    // The deps file should have been removed.
+    EXPECT_EQ(0, fs_.Stat("in1.d"));
+    deps_log.Close();
+    builder.command_runner_.release();
+  }
+
+  {
+    State state;
+    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");
+    fs_.Tick();
+    fs_.Create("out", "");
+
+    // Run the build again.
+    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);
+    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());
+
+    builder.command_runner_.release();
+  }
+}
diff --git a/src/deps_log.cc b/src/deps_log.cc
index 454d2e5..5590a32 100644
--- a/src/deps_log.cc
+++ b/src/deps_log.cc
@@ -30,6 +30,9 @@ const char kFileSignature[] = "# ninja deps v%d\n";
 const int kCurrentVersion = 1;
 }  // anonymous namespace
 
+DepsLog::~DepsLog() {
+  Close();
+}
 
 bool DepsLog::OpenForWrite(const string& path, string* err) {
   file_ = fopen(path.c_str(), "ab");
@@ -113,7 +116,8 @@ bool DepsLog::RecordDeps(Node* node, TimeStamp mtime,
 }
 
 void DepsLog::Close() {
-  fclose(file_);
+  if (file_)
+    fclose(file_);
   file_ = NULL;
 }
 
diff --git a/src/deps_log.h b/src/deps_log.h
index e32a6fe..99b006e 100644
--- a/src/deps_log.h
+++ b/src/deps_log.h
@@ -61,7 +61,8 @@ struct State;
 /// wins, allowing updates to just be appended to the file.  A separate
 /// repacking step can run occasionally to remove dead records.
 struct DepsLog {
-  DepsLog() : dead_record_count_(0) {}
+  DepsLog() : dead_record_count_(0), file_(NULL) {}
+  ~DepsLog();
 
   // Writing (build-time) interface.
   bool OpenForWrite(const string& path, string* err);
diff --git a/src/graph.cc b/src/graph.cc
index 5cc491b..2614882 100644
--- a/src/graph.cc
+++ b/src/graph.cc
@@ -411,7 +411,7 @@ bool ImplicitDepLoader::LoadDepsFromLog(Edge* edge, TimeStamp* deps_mtime,
   if (!deps)
     return false;
 
-  // XXX mtime
+  *deps_mtime = deps->mtime;
 
   vector<Node*>::iterator implicit_dep =
       PreallocateSpace(edge, deps->node_count);
diff --git a/src/test.cc b/src/test.cc
index c48ca82..45a9226 100644
--- a/src/test.cc
+++ b/src/test.cc
@@ -74,7 +74,11 @@ string GetSystemTempDir() {
 }  // anonymous namespace
 
 StateTestWithBuiltinRules::StateTestWithBuiltinRules() {
-  AssertParse(&state_,
+  AddCatRule(&state_);
+}
+
+void StateTestWithBuiltinRules::AddCatRule(State* state) {
+  AssertParse(state,
 "rule cat\n"
 "  command = cat $in > $out\n");
 }
diff --git a/src/test.h b/src/test.h
index bff0e5a..9f29e07 100644
--- a/src/test.h
+++ b/src/test.h
@@ -29,6 +29,12 @@ struct Node;
 /// builtin "cat" rule.
 struct StateTestWithBuiltinRules : public testing::Test {
   StateTestWithBuiltinRules();
+
+  /// Add a "cat" rule to \a state.  Used by some tests; it's
+  /// otherwise done by the ctor to state_.
+  void AddCatRule(State* state);
+
+  /// Short way to get a Node by its path from state_.
   Node* GetNode(const string& path);
 
   State state_;