From 25a3c97a8203dff5589bc13e835b9f362a51f3ab Mon Sep 17 00:00:00 2001 From: Maxim Kalaev Date: Mon, 2 Sep 2013 15:44:32 -0700 Subject: Fix restat rebuild if deps are missing. Fixes issue #603. Apparently, the problem was caused by my fix r "consider target dirty if depfile is missing" (bcc8ad1), which was not working correctly with restat rules cleaning nodes. Switching to deps only triggered an easily observable issue. Fix by setting a flag in edges with invalid deps, and not cleaning edges with that flag set. --- src/build.cc | 4 ++ src/build_test.cc | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++- src/graph.cc | 3 +- src/graph.h | 5 ++- 4 files changed, 126 insertions(+), 4 deletions(-) diff --git a/src/build.cc b/src/build.cc index 76d317f..8a93632 100644 --- a/src/build.cc +++ b/src/build.cc @@ -408,6 +408,10 @@ 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::iterator 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 eeb3db1..3616199 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -60,12 +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; 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. diff --git a/src/graph.h b/src/graph.h index d5d0f4f..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 outputs_; BindingEnv* env_; bool outputs_ready_; + bool deps_missing_; const Rule& rule() const { return *rule_; } Pool* pool() const { return pool_; } -- cgit v0.12