From 0397155218f3d311200ec4e25786028f14c53c6a Mon Sep 17 00:00:00 2001 From: Evan Martin Date: Mon, 8 Apr 2013 10:20:58 -0700 Subject: add a test for the "deps out of date" case It touched the various remaining XXXes in the code, hooray. --- src/build.cc | 8 +++-- src/build_test.cc | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/deps_log.cc | 6 +++- src/deps_log.h | 3 +- src/graph.cc | 2 +- src/test.cc | 6 +++- src/test.h | 6 ++++ 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::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_; -- cgit v0.12