From 3beebde51a2089ecb01820f1428efe0263deaeea Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Tue, 31 Mar 2015 08:12:12 -0700 Subject: Let Stat() have an err outparam instead of writing to stderr. Also check for Stat() failure in a few more places. This way, ninja doesn't print two "ninja: error: " lines if stat() fails during a build. It also makes it easier to keep the stat tests quiet. Every caller of Stat() needs to explicitly log the error string if that's desired. --- src/build.cc | 27 +++++++----- src/build_test.cc | 9 ++-- src/clean.cc | 6 ++- src/clean_test.cc | 85 ++++++++++++++++++++----------------- src/disk_interface.cc | 50 ++++++++++------------ src/disk_interface.h | 11 ++--- src/disk_interface_test.cc | 93 +++++++++++++++++++++++++++-------------- src/graph.cc | 8 +--- src/manifest_parser_perftest.cc | 19 +++++---- src/ninja.cc | 13 +++++- src/test.cc | 6 ++- src/test.h | 3 +- 12 files changed, 192 insertions(+), 138 deletions(-) diff --git a/src/build.cc b/src/build.cc index 1e10c7c..d898929 100644 --- a/src/build.cc +++ b/src/build.cc @@ -560,10 +560,12 @@ void Builder::Cleanup() { // need to rebuild an output because of a modified header file // mentioned in a depfile, and the command touches its depfile // but is interrupted before it touches its output file.) - if (!depfile.empty() || - (*o)->mtime() != disk_interface_->Stat((*o)->path())) { + string err; + TimeStamp new_mtime = disk_interface_->Stat((*o)->path(), &err); + if (new_mtime == -1) // Log and ignore Stat() errors. + Error("%s", err.c_str()); + if (!depfile.empty() || (*o)->mtime() != new_mtime) disk_interface_->RemoveFile((*o)->path()); - } } if (!depfile.empty()) disk_interface_->RemoveFile(depfile); @@ -760,7 +762,9 @@ bool Builder::FinishCommand(CommandRunner::Result* result, string* err) { for (vector::iterator o = edge->outputs_.begin(); o != edge->outputs_.end(); ++o) { - TimeStamp new_mtime = disk_interface_->Stat((*o)->path()); + TimeStamp new_mtime = disk_interface_->Stat((*o)->path(), err); + if (new_mtime == -1) + return false; if ((*o)->mtime() == new_mtime) { // The rule command did not change the output. Propagate the clean // state through the build graph. @@ -776,14 +780,18 @@ bool Builder::FinishCommand(CommandRunner::Result* result, string* err) { // (existing) non-order-only input or the depfile. for (vector::iterator i = edge->inputs_.begin(); i != edge->inputs_.end() - edge->order_only_deps_; ++i) { - TimeStamp input_mtime = disk_interface_->Stat((*i)->path()); + TimeStamp input_mtime = disk_interface_->Stat((*i)->path(), err); + if (input_mtime == -1) + return false; if (input_mtime > restat_mtime) restat_mtime = input_mtime; } string depfile = edge->GetUnescapedDepfile(); if (restat_mtime != 0 && deps_type.empty() && !depfile.empty()) { - TimeStamp depfile_mtime = disk_interface_->Stat(depfile); + TimeStamp depfile_mtime = disk_interface_->Stat(depfile, err); + if (depfile_mtime == -1) + return false; if (depfile_mtime > restat_mtime) restat_mtime = depfile_mtime; } @@ -812,12 +820,9 @@ bool Builder::FinishCommand(CommandRunner::Result* result, string* err) { if (!deps_type.empty() && !config_.dry_run) { assert(edge->outputs_.size() == 1 && "should have been rejected by parser"); Node* out = edge->outputs_[0]; - TimeStamp deps_mtime = disk_interface_->Stat(out->path()); - if (deps_mtime == -1) { - // TODO: Let DiskInterface::Stat() take err instead of it calling Error(). - *err = "stat failed"; + TimeStamp deps_mtime = disk_interface_->Stat(out->path(), err); + if (deps_mtime == -1) return false; - } if (!scan_.deps_log()->RecordDeps(out, deps_mtime, deps_nodes)) { *err = string("Error writing to deps log: ") + strerror(errno); return false; diff --git a/src/build_test.cc b/src/build_test.cc index 0cdcd87..13d1e7e 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -1483,7 +1483,7 @@ TEST_F(BuildTest, InterruptCleanup) { EXPECT_FALSE(builder_.Build(&err)); EXPECT_EQ("interrupted by user", err); builder_.Cleanup(); - EXPECT_GT(fs_.Stat("out1"), 0); + EXPECT_GT(fs_.Stat("out1", &err), 0); err = ""; // A touched output of an interrupted command should be deleted. @@ -1492,7 +1492,7 @@ TEST_F(BuildTest, InterruptCleanup) { EXPECT_FALSE(builder_.Build(&err)); EXPECT_EQ("interrupted by user", err); builder_.Cleanup(); - EXPECT_EQ(0, fs_.Stat("out2")); + EXPECT_EQ(0, fs_.Stat("out2", &err)); } TEST_F(BuildTest, StatFailureAbortsBuild) { @@ -1503,6 +1503,7 @@ TEST_F(BuildTest, StatFailureAbortsBuild) { // This simulates a stat failure: fs_.files_[kTooLongToStat].mtime = -1; + fs_.files_[kTooLongToStat].stat_error = "stat failed"; string err; EXPECT_FALSE(builder_.AddTarget(kTooLongToStat, &err)); @@ -1626,7 +1627,7 @@ TEST_F(BuildWithDepsLogTest, Straightforward) { EXPECT_EQ("", err); // The deps file should have been removed. - EXPECT_EQ(0, fs_.Stat("in1.d")); + EXPECT_EQ(0, fs_.Stat("in1.d", &err)); // Recreate it for the next step. fs_.Create("in1.d", "out: in2"); deps_log.Close(); @@ -1706,7 +1707,7 @@ TEST_F(BuildWithDepsLogTest, ObsoleteDeps) { fs_.Create("out", ""); // The deps file should have been removed, so no need to timestamp it. - EXPECT_EQ(0, fs_.Stat("in1.d")); + EXPECT_EQ(0, fs_.Stat("in1.d", &err)); { State state; diff --git a/src/clean.cc b/src/clean.cc index 7b044c5..1d6ba9e 100644 --- a/src/clean.cc +++ b/src/clean.cc @@ -49,7 +49,11 @@ int Cleaner::RemoveFile(const string& path) { } bool Cleaner::FileExists(const string& path) { - return disk_interface_->Stat(path) > 0; + string err; + TimeStamp mtime = disk_interface_->Stat(path, &err); + if (mtime == -1) + Error("%s", err.c_str()); + return mtime > 0; // Treat Stat() errors as "file does not exist". } void Cleaner::Report(const string& path) { diff --git a/src/clean_test.cc b/src/clean_test.cc index 5869bbb..395343b 100644 --- a/src/clean_test.cc +++ b/src/clean_test.cc @@ -44,10 +44,11 @@ TEST_F(CleanTest, CleanAll) { EXPECT_EQ(4u, fs_.files_removed_.size()); // Check they are removed. - EXPECT_EQ(0, fs_.Stat("in1")); - EXPECT_EQ(0, fs_.Stat("out1")); - EXPECT_EQ(0, fs_.Stat("in2")); - EXPECT_EQ(0, fs_.Stat("out2")); + string err; + EXPECT_EQ(0, fs_.Stat("in1", &err)); + EXPECT_EQ(0, fs_.Stat("out1", &err)); + EXPECT_EQ(0, fs_.Stat("in2", &err)); + EXPECT_EQ(0, fs_.Stat("out2", &err)); fs_.files_removed_.clear(); EXPECT_EQ(0, cleaner.CleanAll()); @@ -75,10 +76,11 @@ TEST_F(CleanTest, CleanAllDryRun) { EXPECT_EQ(0u, fs_.files_removed_.size()); // Check they are not removed. - EXPECT_NE(0, fs_.Stat("in1")); - EXPECT_NE(0, fs_.Stat("out1")); - EXPECT_NE(0, fs_.Stat("in2")); - EXPECT_NE(0, fs_.Stat("out2")); + string err; + EXPECT_LT(0, fs_.Stat("in1", &err)); + EXPECT_LT(0, fs_.Stat("out1", &err)); + EXPECT_LT(0, fs_.Stat("in2", &err)); + EXPECT_LT(0, fs_.Stat("out2", &err)); fs_.files_removed_.clear(); EXPECT_EQ(0, cleaner.CleanAll()); @@ -105,10 +107,11 @@ TEST_F(CleanTest, CleanTarget) { EXPECT_EQ(2u, fs_.files_removed_.size()); // Check they are removed. - EXPECT_EQ(0, fs_.Stat("in1")); - EXPECT_EQ(0, fs_.Stat("out1")); - EXPECT_NE(0, fs_.Stat("in2")); - EXPECT_NE(0, fs_.Stat("out2")); + string err; + EXPECT_EQ(0, fs_.Stat("in1", &err)); + EXPECT_EQ(0, fs_.Stat("out1", &err)); + EXPECT_LT(0, fs_.Stat("in2", &err)); + EXPECT_LT(0, fs_.Stat("out2", &err)); fs_.files_removed_.clear(); ASSERT_EQ(0, cleaner.CleanTarget("out1")); @@ -135,11 +138,12 @@ TEST_F(CleanTest, CleanTargetDryRun) { EXPECT_EQ(2, cleaner.cleaned_files_count()); EXPECT_EQ(0u, fs_.files_removed_.size()); - // Check they are removed. - EXPECT_NE(0, fs_.Stat("in1")); - EXPECT_NE(0, fs_.Stat("out1")); - EXPECT_NE(0, fs_.Stat("in2")); - EXPECT_NE(0, fs_.Stat("out2")); + // Check they are not removed. + string err; + EXPECT_LT(0, fs_.Stat("in1", &err)); + EXPECT_LT(0, fs_.Stat("out1", &err)); + EXPECT_LT(0, fs_.Stat("in2", &err)); + EXPECT_LT(0, fs_.Stat("out2", &err)); fs_.files_removed_.clear(); ASSERT_EQ(0, cleaner.CleanTarget("out1")); @@ -168,10 +172,11 @@ TEST_F(CleanTest, CleanRule) { EXPECT_EQ(2u, fs_.files_removed_.size()); // Check they are removed. - EXPECT_EQ(0, fs_.Stat("in1")); - EXPECT_NE(0, fs_.Stat("out1")); - EXPECT_EQ(0, fs_.Stat("in2")); - EXPECT_NE(0, fs_.Stat("out2")); + string err; + EXPECT_EQ(0, fs_.Stat("in1", &err)); + EXPECT_LT(0, fs_.Stat("out1", &err)); + EXPECT_EQ(0, fs_.Stat("in2", &err)); + EXPECT_LT(0, fs_.Stat("out2", &err)); fs_.files_removed_.clear(); ASSERT_EQ(0, cleaner.CleanRule("cat_e")); @@ -200,11 +205,12 @@ TEST_F(CleanTest, CleanRuleDryRun) { EXPECT_EQ(2, cleaner.cleaned_files_count()); EXPECT_EQ(0u, fs_.files_removed_.size()); - // Check they are removed. - EXPECT_NE(0, fs_.Stat("in1")); - EXPECT_NE(0, fs_.Stat("out1")); - EXPECT_NE(0, fs_.Stat("in2")); - EXPECT_NE(0, fs_.Stat("out2")); + // Check they are not removed. + string err; + EXPECT_LT(0, fs_.Stat("in1", &err)); + EXPECT_LT(0, fs_.Stat("out1", &err)); + EXPECT_LT(0, fs_.Stat("in2", &err)); + EXPECT_LT(0, fs_.Stat("out2", &err)); fs_.files_removed_.clear(); ASSERT_EQ(0, cleaner.CleanRule("cat_e")); @@ -328,12 +334,13 @@ TEST_F(CleanTest, CleanRsp) { EXPECT_EQ(6u, fs_.files_removed_.size()); // Check they are removed. - EXPECT_EQ(0, fs_.Stat("in1")); - EXPECT_EQ(0, fs_.Stat("out1")); - EXPECT_EQ(0, fs_.Stat("in2")); - EXPECT_EQ(0, fs_.Stat("out2")); - EXPECT_EQ(0, fs_.Stat("in2.rsp")); - EXPECT_EQ(0, fs_.Stat("out2.rsp")); + string err; + EXPECT_EQ(0, fs_.Stat("in1", &err)); + EXPECT_EQ(0, fs_.Stat("out1", &err)); + EXPECT_EQ(0, fs_.Stat("in2", &err)); + EXPECT_EQ(0, fs_.Stat("out2", &err)); + EXPECT_EQ(0, fs_.Stat("in2.rsp", &err)); + EXPECT_EQ(0, fs_.Stat("out2.rsp", &err)); } TEST_F(CleanTest, CleanFailure) { @@ -345,6 +352,7 @@ TEST_F(CleanTest, CleanFailure) { } TEST_F(CleanTest, CleanPhony) { + string err; ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, "build phony: phony t1 t2\n" "build t1: cat\n" @@ -358,7 +366,7 @@ TEST_F(CleanTest, CleanPhony) { Cleaner cleaner(&state_, config_, &fs_); EXPECT_EQ(0, cleaner.CleanAll()); EXPECT_EQ(2, cleaner.cleaned_files_count()); - EXPECT_NE(0, fs_.Stat("phony")); + EXPECT_LT(0, fs_.Stat("phony", &err)); fs_.Create("t1", ""); fs_.Create("t2", ""); @@ -366,7 +374,7 @@ TEST_F(CleanTest, CleanPhony) { // Check that CleanTarget does not remove "phony". EXPECT_EQ(0, cleaner.CleanTarget("phony")); EXPECT_EQ(2, cleaner.cleaned_files_count()); - EXPECT_NE(0, fs_.Stat("phony")); + EXPECT_LT(0, fs_.Stat("phony", &err)); } TEST_F(CleanTest, CleanDepFileAndRspFileWithSpaces) { @@ -391,8 +399,9 @@ TEST_F(CleanTest, CleanDepFileAndRspFileWithSpaces) { EXPECT_EQ(4, cleaner.cleaned_files_count()); EXPECT_EQ(4u, fs_.files_removed_.size()); - EXPECT_EQ(0, fs_.Stat("out 1")); - EXPECT_EQ(0, fs_.Stat("out 2")); - EXPECT_EQ(0, fs_.Stat("out 1.d")); - EXPECT_EQ(0, fs_.Stat("out 2.rsp")); + string err; + EXPECT_EQ(0, fs_.Stat("out 1", &err)); + EXPECT_EQ(0, fs_.Stat("out 2", &err)); + EXPECT_EQ(0, fs_.Stat("out 1.d", &err)); + EXPECT_EQ(0, fs_.Stat("out 2.rsp", &err)); } diff --git a/src/disk_interface.cc b/src/disk_interface.cc index 9810708..70282c0 100644 --- a/src/disk_interface.cc +++ b/src/disk_interface.cc @@ -23,6 +23,7 @@ #include #ifdef _WIN32 +#include #include #include // _mkdir #endif @@ -67,16 +68,13 @@ TimeStamp TimeStampFromFileTime(const FILETIME& filetime) { return (TimeStamp)mtime; } -TimeStamp StatSingleFile(const string& path, bool quiet) { +TimeStamp StatSingleFile(const string& path, string* err) { WIN32_FILE_ATTRIBUTE_DATA attrs; if (!GetFileAttributesEx(path.c_str(), GetFileExInfoStandard, &attrs)) { - DWORD err = GetLastError(); - if (err == ERROR_FILE_NOT_FOUND || err == ERROR_PATH_NOT_FOUND) + DWORD win_err = GetLastError(); + if (win_err == ERROR_FILE_NOT_FOUND || win_err == ERROR_PATH_NOT_FOUND) return 0; - if (!quiet) { - Error("GetFileAttributesEx(%s): %s", path.c_str(), - GetLastErrorString().c_str()); - } + *err = "GetFileAttributesEx(" + path + "): " + GetLastErrorString(); return -1; } return TimeStampFromFileTime(attrs.ftLastWriteTime); @@ -98,7 +96,7 @@ bool IsWindows7OrLater() { #endif bool StatAllFilesInDir(const string& dir, map* stamps, - bool quiet) { + string* err) { // FindExInfoBasic is 30% faster than FindExInfoStandard. static bool can_use_basic_info = IsWindows7OrLater(); // This is not in earlier SDKs. @@ -111,13 +109,10 @@ bool StatAllFilesInDir(const string& dir, map* stamps, FindExSearchNameMatch, NULL, 0); if (find_handle == INVALID_HANDLE_VALUE) { - DWORD err = GetLastError(); - if (err == ERROR_FILE_NOT_FOUND || err == ERROR_PATH_NOT_FOUND) + DWORD win_err = GetLastError(); + if (win_err == ERROR_FILE_NOT_FOUND || win_err == ERROR_PATH_NOT_FOUND) return true; - if (!quiet) { - Error("FindFirstFileExA(%s): %s", dir.c_str(), - GetLastErrorString().c_str()); - } + *err = "FindFirstFileExA(" + dir + "): " + GetLastErrorString(); return false; } do { @@ -139,9 +134,12 @@ bool DiskInterface::MakeDirs(const string& path) { string dir = DirName(path); if (dir.empty()) return true; // Reached root; assume it's there. - TimeStamp mtime = Stat(dir); - if (mtime < 0) - return false; // Error. + string err; + TimeStamp mtime = Stat(dir, &err); + if (mtime < 0) { + Error("%s", err.c_str()); + return false; + } if (mtime > 0) return true; // Exists already; we're done. @@ -154,19 +152,19 @@ bool DiskInterface::MakeDirs(const string& path) { // RealDiskInterface ----------------------------------------------------------- -TimeStamp RealDiskInterface::Stat(const string& path) const { +TimeStamp RealDiskInterface::Stat(const string& path, string* err) const { #ifdef _WIN32 // MSDN: "Naming Files, Paths, and Namespaces" // http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx if (!path.empty() && path[0] != '\\' && path.size() > MAX_PATH) { - if (!quiet_) { - Error("Stat(%s): Filename longer than %i characters", - path.c_str(), MAX_PATH); - } + ostringstream err_stream; + err_stream << "Stat(" << path << "): Filename longer than " << MAX_PATH + << " characters"; + *err = err_stream.str(); return -1; } if (!use_cache_) - return StatSingleFile(path, quiet_); + return StatSingleFile(path, err); string dir = DirName(path); string base(path.substr(dir.size() ? dir.size() + 1 : 0)); @@ -177,7 +175,7 @@ TimeStamp RealDiskInterface::Stat(const string& path) const { Cache::iterator ci = cache_.find(dir); if (ci == cache_.end()) { ci = cache_.insert(make_pair(dir, DirCache())).first; - if (!StatAllFilesInDir(dir.empty() ? "." : dir, &ci->second, quiet_)) { + if (!StatAllFilesInDir(dir.empty() ? "." : dir, &ci->second, err)) { cache_.erase(ci); return -1; } @@ -189,9 +187,7 @@ TimeStamp RealDiskInterface::Stat(const string& path) const { if (stat(path.c_str(), &st) < 0) { if (errno == ENOENT || errno == ENOTDIR) return 0; - if (!quiet_) { - Error("stat(%s): %s", path.c_str(), strerror(errno)); - } + *err = "stat(" + path + "): " + strerror(errno); return -1; } return st.st_mtime; diff --git a/src/disk_interface.h b/src/disk_interface.h index a13bced..b61d192 100644 --- a/src/disk_interface.h +++ b/src/disk_interface.h @@ -30,7 +30,7 @@ struct DiskInterface { /// stat() a file, returning the mtime, or 0 if missing and -1 on /// other errors. - virtual TimeStamp Stat(const string& path) const = 0; + virtual TimeStamp Stat(const string& path, string* err) const = 0; /// Create a directory, returning false on failure. virtual bool MakeDir(const string& path) = 0; @@ -56,21 +56,18 @@ struct DiskInterface { /// Implementation of DiskInterface that actually hits the disk. struct RealDiskInterface : public DiskInterface { - RealDiskInterface() : quiet_(false) + RealDiskInterface() #ifdef _WIN32 - , use_cache_(false) + : use_cache_(false) #endif {} virtual ~RealDiskInterface() {} - virtual TimeStamp Stat(const string& path) const; + virtual TimeStamp Stat(const string& path, string* err) const; virtual bool MakeDir(const string& path); virtual bool WriteFile(const string& path, const string& contents); virtual string ReadFile(const string& path, string* err); virtual int RemoveFile(const string& path); - /// Whether to print on errors. Used to make a test quieter. - bool quiet_; - /// Whether stat information can be cached. Only has an effect on Windows. void AllowStatCache(bool allow); diff --git a/src/disk_interface_test.cc b/src/disk_interface_test.cc index 658fffd..9d210b4 100644 --- a/src/disk_interface_test.cc +++ b/src/disk_interface_test.cc @@ -47,49 +47,64 @@ struct DiskInterfaceTest : public testing::Test { }; TEST_F(DiskInterfaceTest, StatMissingFile) { - EXPECT_EQ(0, disk_.Stat("nosuchfile")); + string err; + EXPECT_EQ(0, disk_.Stat("nosuchfile", &err)); + EXPECT_EQ("", err); // On Windows, the errno for a file in a nonexistent directory // is different. - EXPECT_EQ(0, disk_.Stat("nosuchdir/nosuchfile")); + EXPECT_EQ(0, disk_.Stat("nosuchdir/nosuchfile", &err)); + EXPECT_EQ("", err); // On POSIX systems, the errno is different if a component of the // path prefix is not a directory. ASSERT_TRUE(Touch("notadir")); - EXPECT_EQ(0, disk_.Stat("notadir/nosuchfile")); + EXPECT_EQ(0, disk_.Stat("notadir/nosuchfile", &err)); + EXPECT_EQ("", err); } TEST_F(DiskInterfaceTest, StatBadPath) { - disk_.quiet_ = true; + string err; #ifdef _WIN32 string bad_path("cc:\\foo"); - EXPECT_EQ(-1, disk_.Stat(bad_path)); + EXPECT_EQ(-1, disk_.Stat(bad_path, &err)); + EXPECT_NE("", err); #else string too_long_name(512, 'x'); - EXPECT_EQ(-1, disk_.Stat(too_long_name)); + EXPECT_EQ(-1, disk_.Stat(too_long_name, &err)); + EXPECT_NE("", err); #endif - disk_.quiet_ = false; } TEST_F(DiskInterfaceTest, StatExistingFile) { + string err; ASSERT_TRUE(Touch("file")); - EXPECT_GT(disk_.Stat("file"), 1); + EXPECT_GT(disk_.Stat("file", &err), 1); + EXPECT_EQ("", err); } TEST_F(DiskInterfaceTest, StatExistingDir) { + string err; ASSERT_TRUE(disk_.MakeDir("subdir")); ASSERT_TRUE(disk_.MakeDir("subdir/subsubdir")); - EXPECT_GT(disk_.Stat("."), 1); - EXPECT_GT(disk_.Stat("subdir"), 1); - EXPECT_GT(disk_.Stat("subdir/subsubdir"), 1); + EXPECT_GT(disk_.Stat(".", &err), 1); + EXPECT_EQ("", err); + EXPECT_GT(disk_.Stat("subdir", &err), 1); + EXPECT_EQ("", err); + EXPECT_GT(disk_.Stat("subdir/subsubdir", &err), 1); + EXPECT_EQ("", err); - EXPECT_EQ(disk_.Stat("subdir"), disk_.Stat("subdir/.")); - EXPECT_EQ(disk_.Stat("subdir"), disk_.Stat("subdir/subsubdir/..")); - EXPECT_EQ(disk_.Stat("subdir/subsubdir"), disk_.Stat("subdir/subsubdir/.")); + EXPECT_EQ(disk_.Stat("subdir", &err), + disk_.Stat("subdir/.", &err)); + EXPECT_EQ(disk_.Stat("subdir", &err), + disk_.Stat("subdir/subsubdir/..", &err)); + EXPECT_EQ(disk_.Stat("subdir/subsubdir", &err), + disk_.Stat("subdir/subsubdir/.", &err)); } #ifdef _WIN32 TEST_F(DiskInterfaceTest, StatCache) { + string err; disk_.AllowStatCache(true); ASSERT_TRUE(Touch("file1")); @@ -100,27 +115,43 @@ TEST_F(DiskInterfaceTest, StatCache) { ASSERT_TRUE(Touch("subdir\\SUBFILE2")); ASSERT_TRUE(Touch("subdir\\SUBFILE3")); - EXPECT_GT(disk_.Stat("FIle1"), 1); - EXPECT_GT(disk_.Stat("file1"), 1); + EXPECT_GT(disk_.Stat("FIle1", &err), 1); + EXPECT_EQ("", err); + EXPECT_GT(disk_.Stat("file1", &err), 1); + EXPECT_EQ("", err); - EXPECT_GT(disk_.Stat("subdir/subfile2"), 1); - EXPECT_GT(disk_.Stat("sUbdir\\suBFile1"), 1); + EXPECT_GT(disk_.Stat("subdir/subfile2", &err), 1); + EXPECT_EQ("", err); + EXPECT_GT(disk_.Stat("sUbdir\\suBFile1", &err), 1); + EXPECT_EQ("", err); - EXPECT_GT(disk_.Stat("."), 1); - EXPECT_GT(disk_.Stat("subdir"), 1); - EXPECT_GT(disk_.Stat("subdir/subsubdir"), 1); + EXPECT_GT(disk_.Stat(".", &err), 1); + EXPECT_EQ("", err); + EXPECT_GT(disk_.Stat("subdir", &err), 1); + EXPECT_EQ("", err); + EXPECT_GT(disk_.Stat("subdir/subsubdir", &err), 1); + EXPECT_EQ("", err); - EXPECT_EQ(disk_.Stat("subdir"), disk_.Stat("subdir/.")); - EXPECT_EQ(disk_.Stat("subdir"), disk_.Stat("subdir/subsubdir/..")); - EXPECT_EQ(disk_.Stat("subdir/subsubdir"), disk_.Stat("subdir/subsubdir/.")); + EXPECT_EQ(disk_.Stat("subdir", &err), + disk_.Stat("subdir/.", &err)); + EXPECT_EQ("", err); + EXPECT_EQ(disk_.Stat("subdir", &err), + disk_.Stat("subdir/subsubdir/..", &err)); + EXPECT_EQ("", err); + EXPECT_EQ(disk_.Stat("subdir/subsubdir", &err), + disk_.Stat("subdir/subsubdir/.", &err)); + EXPECT_EQ("", err); // Test error cases. - disk_.quiet_ = true; string bad_path("cc:\\foo"); - EXPECT_EQ(-1, disk_.Stat(bad_path)); - EXPECT_EQ(-1, disk_.Stat(bad_path)); - EXPECT_EQ(0, disk_.Stat("nosuchfile")); - EXPECT_EQ(0, disk_.Stat("nosuchdir/nosuchfile")); + EXPECT_EQ(-1, disk_.Stat(bad_path, &err)); + EXPECT_NE("", err); err.clear(); + EXPECT_EQ(-1, disk_.Stat(bad_path, &err)); + EXPECT_NE("", err); err.clear(); + EXPECT_EQ(0, disk_.Stat("nosuchfile", &err)); + EXPECT_EQ("", err); + EXPECT_EQ(0, disk_.Stat("nosuchdir/nosuchfile", &err)); + EXPECT_EQ("", err); } #endif @@ -168,7 +199,7 @@ struct StatTest : public StateTestWithBuiltinRules, StatTest() : scan_(&state_, NULL, NULL, this) {} // DiskInterface implementation. - virtual TimeStamp Stat(const string& path) const; + virtual TimeStamp Stat(const string& path, string* err) const; virtual bool WriteFile(const string& path, const string& contents) { assert(false); return true; @@ -191,7 +222,7 @@ struct StatTest : public StateTestWithBuiltinRules, mutable vector stats_; }; -TimeStamp StatTest::Stat(const string& path) const { +TimeStamp StatTest::Stat(const string& path, string* err) const { stats_.push_back(path); map::const_iterator i = mtimes_.find(path); if (i == mtimes_.end()) diff --git a/src/graph.cc b/src/graph.cc index d99c8bd..fcfeba0 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -29,13 +29,7 @@ bool Node::Stat(DiskInterface* disk_interface, string* err) { METRIC_RECORD("node stat"); - mtime_ = disk_interface->Stat(path_); - if (mtime_ == -1) { - // TODO: Let DiskInterface::Stat() take err instead of it calling Error(). - *err = "stat failed"; - return false; - } - return true; + return (mtime_ = disk_interface->Stat(path_, err)) != -1; } bool DependencyScan::RecomputeDirty(Edge* edge, string* err) { diff --git a/src/manifest_parser_perftest.cc b/src/manifest_parser_perftest.cc index ca62fb2..6b56ab0 100644 --- a/src/manifest_parser_perftest.cc +++ b/src/manifest_parser_perftest.cc @@ -42,15 +42,19 @@ struct RealFileReader : public ManifestParser::FileReader { } }; -bool WriteFakeManifests(const string& dir) { +bool WriteFakeManifests(const string& dir, string* err) { RealDiskInterface disk_interface; - if (disk_interface.Stat(dir + "/build.ninja") > 0) - return true; + TimeStamp mtime = disk_interface.Stat(dir + "/build.ninja", err); + if (mtime != 0) // 0 means that the file doesn't exist yet. + return mtime != -1; + string command = "python misc/write_fake_manifests.py " + dir; printf("Creating manifest data..."); fflush(stdout); - int err = system(("python misc/write_fake_manifests.py " + dir).c_str()); + int exit_code = system(command.c_str()); printf("done.\n"); - return err == 0; + if (exit_code != 0) + *err = "Failed to run " + command; + return exit_code == 0; } int LoadManifests(bool measure_command_evaluation) { @@ -93,8 +97,9 @@ int main(int argc, char* argv[]) { const char kManifestDir[] = "build/manifest_perftest"; - if (!WriteFakeManifests(kManifestDir)) { - fprintf(stderr, "Failed to write test data\n"); + string err; + if (!WriteFakeManifests(kManifestDir, &err)) { + fprintf(stderr, "Failed to write test data: %s\n", err.c_str()); return 1; } diff --git a/src/ninja.cc b/src/ninja.cc index 1e43137..e5bf98d 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -143,6 +143,8 @@ struct NinjaMain : public BuildLogUser { virtual bool IsPathDead(StringPiece s) const { Node* n = state_.LookupNode(s); + if (!n || !n->in_edge()) + return false; // Just checking n isn't enough: If an old output is both in the build log // and in the deps log, it will have a Node object in state_. (It will also // have an in edge if one of its inputs is another output that's in the deps @@ -152,7 +154,11 @@ struct NinjaMain : public BuildLogUser { // which seems good enough for this corner case.) // Do keep entries around for files which still exist on disk, for // generators that want to use this information. - return (!n || !n->in_edge()) && disk_interface_.Stat(s.AsString()) == 0; + string err; + TimeStamp mtime = disk_interface_.Stat(s.AsString(), &err); + if (mtime == -1) + Error("%s", err.c_str()); // Log and ignore Stat() errors. + return mtime == 0; } }; @@ -481,7 +487,10 @@ int NinjaMain::ToolDeps(int argc, char** argv) { continue; } - TimeStamp mtime = disk_interface.Stat((*it)->path()); + string err; + TimeStamp mtime = disk_interface.Stat((*it)->path(), &err); + if (mtime == -1) + Error("%s", err.c_str()); // Log and ignore Stat() errors; printf("%s: #deps %d, deps mtime %d (%s)\n", (*it)->path().c_str(), deps->node_count, deps->mtime, (!mtime || mtime > deps->mtime ? "STALE":"VALID")); diff --git a/src/test.cc b/src/test.cc index ddecd3d..aed8db7 100644 --- a/src/test.cc +++ b/src/test.cc @@ -145,10 +145,12 @@ void VirtualFileSystem::Create(const string& path, files_created_.insert(path); } -TimeStamp VirtualFileSystem::Stat(const string& path) const { +TimeStamp VirtualFileSystem::Stat(const string& path, string* err) const { FileMap::const_iterator i = files_.find(path); - if (i != files_.end()) + if (i != files_.end()) { + *err = i->second.stat_error; return i->second.mtime; + } return 0; } diff --git a/src/test.h b/src/test.h index 3ce510d..156e68a 100644 --- a/src/test.h +++ b/src/test.h @@ -142,7 +142,7 @@ struct VirtualFileSystem : public DiskInterface { } // DiskInterface - virtual TimeStamp Stat(const string& path) const; + virtual TimeStamp Stat(const string& path, string* err) const; virtual bool WriteFile(const string& path, const string& contents); virtual bool MakeDir(const string& path); virtual string ReadFile(const string& path, string* err); @@ -151,6 +151,7 @@ struct VirtualFileSystem : public DiskInterface { /// An entry for a single in-memory file. struct Entry { int mtime; + string stat_error; // If mtime is -1. string contents; }; -- cgit v0.12