Merge pull request #648 from nico/landmaxim

Land the majority of pull request #608.

4 files changed, 170 insertions, 53 deletions

diff --git a/src/build.cc b/src/build.cc
index 45f6849..8a93632 100644
--- a/src/build.cc
+++ b/src/build.cc
@@ -408,31 +408,27 @@ void Plan::CleanNode(DependencyScan* scan, Node* node) {
     if (want_i == want_.end() || !want_i->second)
       continue;
 
+    // Don't attempt to clean an edge if it failed to load deps.
+    if ((*ei)->deps_missing_)
+      continue;
+
     // If all non-order-only inputs for this edge are now clean,
     // we might have changed the dirty state of the outputs.
     vector<Node*>::iterator
         begin = (*ei)->inputs_.begin(),
         end = (*ei)->inputs_.end() - (*ei)->order_only_deps_;
     if (find_if(begin, end, mem_fun(&Node::dirty)) == end) {
-      // Recompute most_recent_input and command.
+      // Recompute most_recent_input.
       Node* most_recent_input = NULL;
       for (vector<Node*>::iterator ni = begin; ni != end; ++ni) {
         if (!most_recent_input || (*ni)->mtime() > most_recent_input->mtime())
           most_recent_input = *ni;
       }
-      string command = (*ei)->EvaluateCommand(true);
 
       // Now, this edge is dirty if any of the outputs are dirty.
-      bool dirty = false;
-      for (vector<Node*>::iterator ni = (*ei)->outputs_.begin();
-           !dirty && ni != (*ei)->outputs_.end(); ++ni) {
-        dirty = scan->RecomputeOutputDirty(*ei, most_recent_input, 0,
-                                           command, *ni);
-      }
-
       // If the edge isn't dirty, clean the outputs and mark the edge as not
       // wanted.
-      if (!dirty) {
+      if (!scan->RecomputeOutputsDirty(*ei, most_recent_input)) {
         for (vector<Node*>::iterator ni = (*ei)->outputs_.begin();
              ni != (*ei)->outputs_.end(); ++ni) {
           CleanNode(scan, *ni);
diff --git a/src/build_test.cc b/src/build_test.cc
index 66d0954..4b1c829 100644
--- a/src/build_test.cc
+++ b/src/build_test.cc
@@ -435,6 +435,13 @@ struct BuildTest : public StateTestWithBuiltinRules {
     builder_.command_runner_.release();
   }
 
+  /// Rebuild target in the 'working tree' (fs_).
+  /// State of command_runner_ and logs contents (if specified) ARE MODIFIED.
+  /// Handy to check for NOOP builds, and higher-level rebuild tests.
+  void RebuildTarget(const string& target, const char* manifest,
+                     const char* log_path = NULL,
+                     const char* deps_path = NULL);
+
   // Mark a path dirty.
   void Dirty(const string& path);
 
@@ -452,6 +459,39 @@ struct BuildTest : public StateTestWithBuiltinRules {
   BuildStatus status_;
 };
 
+void BuildTest::RebuildTarget(const string& target, const char* manifest,
+                              const char* log_path, const char* deps_path) {
+  State state;
+  ASSERT_NO_FATAL_FAILURE(AddCatRule(&state));
+  AssertParse(&state, manifest);
+
+  string err;
+  BuildLog build_log, *pbuild_log = NULL;
+  if (log_path) {
+    ASSERT_TRUE(build_log.Load(log_path, &err));
+    ASSERT_TRUE(build_log.OpenForWrite(log_path, &err));
+    ASSERT_EQ("", err);
+    pbuild_log = &build_log;
+  }
+
+  DepsLog deps_log, *pdeps_log = NULL;
+  if (deps_path) {
+    ASSERT_TRUE(deps_log.Load(deps_path, &state, &err));
+    ASSERT_TRUE(deps_log.OpenForWrite(deps_path, &err));
+    ASSERT_EQ("", err);
+    pdeps_log = &deps_log;
+  }
+
+  Builder builder(&state, config_, pbuild_log, pdeps_log, &fs_);
+  EXPECT_TRUE(builder.AddTarget(target, &err));
+
+  command_runner_.commands_ran_.clear();
+  builder.command_runner_.reset(&command_runner_);
+  bool build_res = builder.Build(&err);
+  builder.command_runner_.release();
+  EXPECT_TRUE(build_res) << "builder.Build(&err)";
+}
+
 bool FakeCommandRunner::CanRunMore() {
   // Only run one at a time.
   return last_command_ == NULL;
@@ -1780,7 +1820,7 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) {
 
     // Expect three new edges: one generating foo.o, and two more from
     // loading the depfile.
-    ASSERT_EQ(3, (int)state.edges_.size());
+    ASSERT_EQ(3u, state.edges_.size());
     // Expect our edge to now have three inputs: foo.c and two headers.
     ASSERT_EQ(3u, edge->inputs_.size());
 
@@ -1791,3 +1831,79 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) {
     builder.command_runner_.release();
   }
 }
+
+/// Check that a restat rule doesn't clear an edge if the depfile is missing.
+/// Follows from: https://github.com/martine/ninja/issues/603
+TEST_F(BuildTest, RestatMissingDepfile) {
+const char* manifest =
+"rule true\n"
+"  command = true\n"  // Would be "write if out-of-date" in reality.
+"  restat = 1\n"
+"build header.h: true header.in\n"
+"build out: cat header.h\n"
+"  depfile = out.d\n";
+
+  fs_.Create("header.h", "");
+  fs_.Tick();
+  fs_.Create("out", "");
+  fs_.Create("header.in", "");
+
+  // Normally, only 'header.h' would be rebuilt, as
+  // its rule doesn't touch the output and has 'restat=1' set.
+  // But we are also missing the depfile for 'out',
+  // which should force its command to run anyway!
+  RebuildTarget("out", manifest);
+  ASSERT_EQ(2u, command_runner_.commands_ran_.size());
+}
+
+/// Check that a restat rule doesn't clear an edge if the deps are missing.
+/// https://github.com/martine/ninja/issues/603
+TEST_F(BuildWithDepsLogTest, RestatMissingDepfileDepslog) {
+  string err;
+  const char* manifest =
+"rule true\n"
+"  command = true\n"  // Would be "write if out-of-date" in reality.
+"  restat = 1\n"
+"build header.h: true header.in\n"
+"build out: cat header.h\n"
+"  deps = gcc\n"
+"  depfile = out.d\n";
+
+  // Build once to populate ninja deps logs from out.d
+  fs_.Create("header.in", "");
+  fs_.Create("out.d", "out: header.h");
+  fs_.Create("header.h", "");
+
+  RebuildTarget("out", manifest, "build_log", "ninja_deps");
+  ASSERT_EQ(2u, command_runner_.commands_ran_.size());
+
+  // Sanity: this rebuild should be NOOP
+  RebuildTarget("out", manifest, "build_log", "ninja_deps");
+  ASSERT_EQ(0u, command_runner_.commands_ran_.size());
+
+  // Touch 'header.in', blank dependencies log (create a different one).
+  // Building header.h triggers 'restat' outputs cleanup.
+  // Validate that out is rebuilt netherless, as deps are missing.
+  fs_.Tick();
+  fs_.Create("header.in", "");
+
+  // (switch to a new blank deps_log "ninja_deps2")
+  RebuildTarget("out", manifest, "build_log", "ninja_deps2");
+  ASSERT_EQ(2u, command_runner_.commands_ran_.size());
+
+  // Sanity: this build should be NOOP
+  RebuildTarget("out", manifest, "build_log", "ninja_deps2");
+  ASSERT_EQ(0u, command_runner_.commands_ran_.size());
+
+  // Check that invalidating deps by target timestamp also works here
+  // Repeat the test but touch target instead of blanking the log.
+  fs_.Tick();
+  fs_.Create("header.in", "");
+  fs_.Create("out", "");
+  RebuildTarget("out", manifest, "build_log", "ninja_deps2");
+  ASSERT_EQ(2u, command_runner_.commands_ran_.size());
+
+  // And this build should be NOOP again
+  RebuildTarget("out", manifest, "build_log", "ninja_deps2");
+  ASSERT_EQ(0u, command_runner_.commands_ran_.size());
+}
diff --git a/src/graph.cc b/src/graph.cc
index fdd93de..3616199 100644
--- a/src/graph.cc
+++ b/src/graph.cc
@@ -60,13 +60,13 @@ bool Rule::IsReservedBinding(const string& var) {
 bool DependencyScan::RecomputeDirty(Edge* edge, string* err) {
   bool dirty = false;
   edge->outputs_ready_ = true;
+  edge->deps_missing_ = false;
 
-  TimeStamp deps_mtime = 0;
-  if (!dep_loader_.LoadDeps(edge, &deps_mtime, err)) {
+  if (!dep_loader_.LoadDeps(edge, err)) {
     if (!err->empty())
       return false;
     // Failed to load dependency info: rebuild to regenerate it.
-    dirty = true;
+    dirty = edge->deps_missing_ = true;
   }
 
   // Visit all inputs; we're dirty if any of the inputs are dirty.
@@ -107,19 +107,8 @@ bool DependencyScan::RecomputeDirty(Edge* edge, string* err) {
 
   // We may also be dirty due to output state: missing outputs, out of
   // date outputs, etc.  Visit all outputs and determine whether they're dirty.
-  if (!dirty) {
-    string command = edge->EvaluateCommand(true);
-
-    for (vector<Node*>::iterator i = edge->outputs_.begin();
-         i != edge->outputs_.end(); ++i) {
-      (*i)->StatIfNecessary(disk_interface_);
-      if (RecomputeOutputDirty(edge, most_recent_input, deps_mtime,
-                               command, *i)) {
-        dirty = true;
-        break;
-      }
-    }
-  }
+  if (!dirty)
+    dirty = RecomputeOutputsDirty(edge, most_recent_input);
 
   // Finally, visit each output to mark off that we've visited it, and update
   // their dirty state if necessary.
@@ -141,9 +130,20 @@ bool DependencyScan::RecomputeDirty(Edge* edge, string* err) {
   return true;
 }
 
+bool DependencyScan::RecomputeOutputsDirty(Edge* edge,
+                                           Node* most_recent_input) {   
+  string command = edge->EvaluateCommand(true);
+  for (vector<Node*>::iterator i = edge->outputs_.begin();
+       i != edge->outputs_.end(); ++i) {
+    (*i)->StatIfNecessary(disk_interface_);
+    if (RecomputeOutputDirty(edge, most_recent_input, command, *i))
+      return true;
+  }
+  return false;
+}
+
 bool DependencyScan::RecomputeOutputDirty(Edge* edge,
                                           Node* most_recent_input,
-                                          TimeStamp deps_mtime,
                                           const string& command,
                                           Node* output) {
   if (edge->is_phony()) {
@@ -185,13 +185,6 @@ bool DependencyScan::RecomputeOutputDirty(Edge* edge,
     }
   }
 
-  // Dirty if the output is newer than the deps.
-  if (deps_mtime && output->mtime() > deps_mtime) {
-    EXPLAIN("stored deps info out of date for for %s (%d vs %d)",
-            output->path().c_str(), deps_mtime, output->mtime());
-    return true;
-  }
-
   // May also be dirty due to the command changing since the last build.
   // But if this is a generator rule, the command changing does not make us
   // dirty.
@@ -332,10 +325,10 @@ void Node::Dump(const char* prefix) const {
   }
 }
 
-bool ImplicitDepLoader::LoadDeps(Edge* edge, TimeStamp* mtime, string* err) {
+bool ImplicitDepLoader::LoadDeps(Edge* edge, string* err) {
   string deps_type = edge->GetBinding("deps");
   if (!deps_type.empty()) {
-    if (!LoadDepsFromLog(edge, mtime, err)) {
+    if (!LoadDepsFromLog(edge, err)) {
       if (!err->empty())
         return false;
       EXPLAIN("deps for %s are missing", edge->outputs_[0]->path().c_str());
@@ -406,13 +399,21 @@ bool ImplicitDepLoader::LoadDepFile(Edge* edge, const string& path,
   return true;
 }
 
-bool ImplicitDepLoader::LoadDepsFromLog(Edge* edge, TimeStamp* deps_mtime,
-                                        string* err) {
-  DepsLog::Deps* deps = deps_log_->GetDeps(edge->outputs_[0]);
-  if (!deps)
+bool ImplicitDepLoader::LoadDepsFromLog(Edge* edge, string* err) {
+  // NOTE: deps are only supported for single-target edges.
+  Node* output = edge->outputs_[0];
+  DepsLog::Deps* deps = deps_log_->GetDeps(output);
+  if (!deps) {
+    EXPLAIN("deps for '%s' are missing", output->path().c_str());
     return false;
+  }
 
-  *deps_mtime = deps->mtime;
+  // Deps are invalid if the output is newer than the deps.
+  if (output->mtime() > deps->mtime) {
+    EXPLAIN("stored deps info out of date for for '%s' (%d vs %d)",
+            output->path().c_str(), deps->mtime, output->mtime());
+    return false;
+  }
 
   vector<Node*>::iterator implicit_dep =
       PreallocateSpace(edge, deps->node_count);
diff --git a/src/graph.h b/src/graph.h
index 428ba01..868413c 100644
--- a/src/graph.h
+++ b/src/graph.h
@@ -135,8 +135,8 @@ struct Rule {
 
 /// An edge in the dependency graph; links between Nodes using Rules.
 struct Edge {
-  Edge() : rule_(NULL), env_(NULL), outputs_ready_(false), implicit_deps_(0),
-           order_only_deps_(0) {}
+  Edge() : rule_(NULL), env_(NULL), outputs_ready_(false), deps_missing_(false),
+           implicit_deps_(0), order_only_deps_(0) {}
 
   /// Return true if all inputs' in-edges are ready.
   bool AllInputsReady() const;
@@ -157,6 +157,7 @@ struct Edge {
   vector<Node*> outputs_;
   BindingEnv* env_;
   bool outputs_ready_;
+  bool deps_missing_;
 
   const Rule& rule() const { return *rule_; }
   Pool* pool() const { return pool_; }
@@ -192,10 +193,10 @@ struct ImplicitDepLoader {
                     DiskInterface* disk_interface)
       : state_(state), disk_interface_(disk_interface), deps_log_(deps_log) {}
 
-  /// Load implicit dependencies for \a edge.  May fill in \a mtime with
-  /// the timestamp of the loaded information.
-  /// @return false on error (without filling \a err if info is just missing).
-  bool LoadDeps(Edge* edge, TimeStamp* mtime, string* err);
+  /// Load implicit dependencies for \a edge.
+  /// @return false on error (without filling \a err if info is just missing
+  //                          or out of date).
+  bool LoadDeps(Edge* edge, string* err);
 
   DepsLog* deps_log() const {
     return deps_log_;
@@ -208,7 +209,7 @@ struct ImplicitDepLoader {
 
   /// Load implicit dependencies for \a edge from the DepsLog.
   /// @return false on error (without filling \a err if info is just missing).
-  bool LoadDepsFromLog(Edge* edge, TimeStamp* mtime, string* err);
+  bool LoadDepsFromLog(Edge* edge, string* err);
 
   /// Preallocate \a count spaces in the input array on \a edge, returning
   /// an iterator pointing at the first new space.
@@ -240,11 +241,9 @@ struct DependencyScan {
   /// Returns false on failure.
   bool RecomputeDirty(Edge* edge, string* err);
 
-  /// Recompute whether a given single output should be marked dirty.
+  /// Recompute whether any output of the edge is dirty.
   /// Returns true if so.
-  bool RecomputeOutputDirty(Edge* edge, Node* most_recent_input,
-                            TimeStamp deps_mtime,
-                            const string& command, Node* output);
+  bool RecomputeOutputsDirty(Edge* edge, Node* most_recent_input);
 
   BuildLog* build_log() const {
     return build_log_;
@@ -258,6 +257,11 @@ struct DependencyScan {
   }
 
  private:
+  /// Recompute whether a given single output should be marked dirty.
+  /// Returns true if so.
+  bool RecomputeOutputDirty(Edge* edge, Node* most_recent_input,
+                            const string& command, Node* output);
+
   BuildLog* build_log_;
   DiskInterface* disk_interface_;
   ImplicitDepLoader dep_loader_;