From 9412cb05008437b2ce534871ec86772b107ce23c Mon Sep 17 00:00:00 2001 From: Nicolas Despres Date: Mon, 2 May 2011 16:46:31 +0200 Subject: Return non-zero status when errors occur. Clean all was not returning non-zero when an error occur (like when failing to remove a directory). This patch fix the problem and add a test for it. --- src/clean.cc | 50 +++++++++++++++++++++++++++++++++++--------------- src/clean.h | 13 +++++++++---- src/clean_test.cc | 16 ++++++++++++---- src/ninja.cc | 3 +-- src/ninja.h | 8 +++++--- src/ninja_jumble.cc | 8 ++++---- src/ninja_test.cc | 9 +++++---- src/test.cc | 9 ++++++--- src/test.h | 2 +- 9 files changed, 78 insertions(+), 40 deletions(-) diff --git a/src/clean.cc b/src/clean.cc index 5072484..e86793e 100644 --- a/src/clean.cc +++ b/src/clean.cc @@ -31,6 +31,7 @@ Cleaner::Cleaner(State* state, const BuildConfig& config) , removed_() , cleaned_files_count_(0) , disk_interface_(new RealDiskInterface) + , status_(0) { } @@ -42,10 +43,11 @@ Cleaner::Cleaner(State* state, , removed_() , cleaned_files_count_(0) , disk_interface_(disk_interface) + , status_(0) { } -bool Cleaner::RemoveFile(const string& path) { +int Cleaner::RemoveFile(const string& path) { return disk_interface_->RemoveFile(path); } @@ -66,8 +68,11 @@ void Cleaner::Remove(const string& path) { if (FileExists(path)) Report(path); } else { - if (RemoveFile(path)) + int ret = RemoveFile(path); + if (ret == 0) Report(path); + else if (ret == -1) + status_ = 1; } } } @@ -93,7 +98,8 @@ void Cleaner::PrintFooter() { printf("%d files.\n", cleaned_files_count_); } -void Cleaner::CleanAll() { +int Cleaner::CleanAll() { + Reset(); PrintHeader(); for (vector::iterator e = state_->edges_.begin(); e != state_->edges_.end(); @@ -103,6 +109,7 @@ void Cleaner::CleanAll() { ++out_node) Remove((*out_node)->file_->path_); PrintFooter(); + return status_; } void Cleaner::DoCleanTarget(Node* target) { @@ -116,28 +123,32 @@ void Cleaner::DoCleanTarget(Node* target) { } } -void Cleaner::CleanTarget(Node* target) { +int Cleaner::CleanTarget(Node* target) { assert(target); + Reset(); PrintHeader(); DoCleanTarget(target); PrintFooter(); + return status_; } int Cleaner::CleanTarget(const char* target) { assert(target); + + Reset(); Node* node = state_->LookupNode(target); if (node) { CleanTarget(node); - return 0; } else { Error("unknown target '%s'", target); - return 1; + status_ = 1; } + return status_; } int Cleaner::CleanTargets(int target_count, char* targets[]) { - int status = 0; + Reset(); PrintHeader(); for (int i = 0; i < target_count; ++i) { const char* target_name = targets[i]; @@ -148,11 +159,11 @@ int Cleaner::CleanTargets(int target_count, char* targets[]) { DoCleanTarget(target); } else { Error("unknown target '%s'", target_name); - status = 1; + status_ = 1; } } PrintFooter(); - return status; + return status_; } void Cleaner::DoCleanRule(const Rule* rule) { @@ -168,31 +179,34 @@ void Cleaner::DoCleanRule(const Rule* rule) { Remove((*out_node)->file_->path_); } -void Cleaner::CleanRule(const Rule* rule) { +int Cleaner::CleanRule(const Rule* rule) { assert(rule); + Reset(); PrintHeader(); DoCleanRule(rule); PrintFooter(); + return status_; } int Cleaner::CleanRule(const char* rule) { assert(rule); + Reset(); const Rule* r = state_->LookupRule(rule); if (r) { CleanRule(r); - return 0; } else { Error("unknown rule '%s'", rule); - return 1; + status_ = 1; } + return status_; } int Cleaner::CleanRules(int rule_count, char* rules[]) { assert(rules); - int status = 0; + Reset(); PrintHeader(); for (int i = 0; i < rule_count; ++i) { const char* rule_name = rules[i]; @@ -203,9 +217,15 @@ int Cleaner::CleanRules(int rule_count, char* rules[]) { DoCleanRule(rule); } else { Error("unknown rule '%s'", rule_name); - status = 1; + status_ = 1; } } PrintFooter(); - return status; + return status_; +} + +void Cleaner::Reset() { + status_ = 0; + cleaned_files_count_ = 0; + removed_.clear(); } diff --git a/src/clean.h b/src/clean.h index 6194aa0..8f0cfa4 100644 --- a/src/clean.h +++ b/src/clean.h @@ -40,7 +40,8 @@ public: DiskInterface* disk_interface); /// Clean the given @a target and all the file built for it. - void CleanTarget(Node* target); + /// @return non-zero if an error occurs. + int CleanTarget(Node* target); /// Clean the given target @a target. /// @return non-zero if an error occurs. int CleanTarget(const char* target); @@ -49,10 +50,12 @@ public: int CleanTargets(int target_count, char* targets[]); /// Clean all built files. - void CleanAll(); + /// @return non-zero if an error occurs. + int CleanAll(); /// Clean all the file built with the given rule @a rule. - void CleanRule(const Rule* rule); + /// @return non-zero if an error occurs. + int CleanRule(const Rule* rule); /// Clean the file produced by the given @a rule. /// @return non-zero if an error occurs. int CleanRule(const char* rule); @@ -74,7 +77,7 @@ public: private: /// Remove the file @a path. /// @return whether the file has been removed. - bool RemoveFile(const string& path); + int RemoveFile(const string& path); /// @returns whether the file @a path exists. bool FileExists(const string& path); void Report(const string& path); @@ -87,6 +90,7 @@ private: void PrintHeader(); void PrintFooter(); void DoCleanRule(const Rule* rule); + void Reset(); private: State* state_; @@ -94,6 +98,7 @@ private: set removed_; int cleaned_files_count_; DiskInterface* disk_interface_; + int status_; }; #endif // NINJA_CLEAN_H_ diff --git a/src/clean_test.cc b/src/clean_test.cc index 0e2114e..d16941c 100644 --- a/src/clean_test.cc +++ b/src/clean_test.cc @@ -40,7 +40,7 @@ TEST_F(CleanTest, CleanAll) { Cleaner cleaner(&state_, config_, &fs_); ASSERT_EQ(0, cleaner.cleaned_files_count()); - cleaner.CleanAll(); + EXPECT_EQ(0, cleaner.CleanAll()); EXPECT_EQ(4, cleaner.cleaned_files_count()); EXPECT_EQ(4, fs_.files_removed_.size()); } @@ -56,7 +56,7 @@ TEST_F(CleanTest, CleanAll) { Cleaner cleaner(&state_, config_, &fs_); ASSERT_EQ(0, cleaner.cleaned_files_count()); - cleaner.CleanAll(); + EXPECT_EQ(0, cleaner.CleanAll()); EXPECT_EQ(0, cleaner.cleaned_files_count()); EXPECT_EQ(0, fs_.files_removed_.size()); } @@ -78,7 +78,7 @@ TEST_F(CleanTest, CleanAllDryRun) { Cleaner cleaner(&state_, config_, &fs_); ASSERT_EQ(0, cleaner.cleaned_files_count()); - cleaner.CleanAll(); + EXPECT_EQ(0, cleaner.CleanAll()); EXPECT_EQ(4, cleaner.cleaned_files_count()); EXPECT_EQ(0, fs_.files_removed_.size()); } @@ -94,7 +94,7 @@ TEST_F(CleanTest, CleanAllDryRun) { Cleaner cleaner(&state_, config_, &fs_); ASSERT_EQ(0, cleaner.cleaned_files_count()); - cleaner.CleanAll(); + EXPECT_EQ(0, cleaner.CleanAll()); EXPECT_EQ(4, cleaner.cleaned_files_count()); EXPECT_EQ(0, fs_.files_removed_.size()); } @@ -253,3 +253,11 @@ TEST_F(CleanTest, CleanRuleDryRun) { EXPECT_EQ(0, fs_.files_removed_.size()); } } + +TEST_F(CleanTest, CleanFailure) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, + "build dir: cat src1\n")); + fs_.MakeDir("dir"); + Cleaner cleaner(&state_, config_, &fs_); + EXPECT_NE(0, cleaner.CleanAll()); +} diff --git a/src/ninja.cc b/src/ninja.cc index fbae091..26b6e5e 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -335,8 +335,7 @@ int CmdClean(State* state, } } else { - cleaner.CleanAll(); - return 0; + return cleaner.CleanAll(); } } diff --git a/src/ninja.h b/src/ninja.h index 0f4a029..8927768 100644 --- a/src/ninja.h +++ b/src/ninja.h @@ -51,8 +51,10 @@ struct DiskInterface { virtual string ReadFile(const string& path, string* err) = 0; /// Remove the file named @a path. It behaves like 'rm -f path' so no errors /// are reported if it does not exists. - /// @returns whether the file has been removed. - virtual bool RemoveFile(const string& path) = 0; + /// @returns 0 if the file has been removed, + /// 1 if the file does not exists, and + /// -1 if an error occurs. + virtual int RemoveFile(const string& path) = 0; /// Create all the parent directories for path; like mkdir -p /// `basename path`. @@ -65,7 +67,7 @@ struct RealDiskInterface : public DiskInterface { virtual int Stat(const string& path); virtual bool MakeDir(const string& path); virtual string ReadFile(const string& path, string* err); - virtual bool RemoveFile(const string& path); + virtual int RemoveFile(const string& path); }; /// Mapping of path -> FileStat. diff --git a/src/ninja_jumble.cc b/src/ninja_jumble.cc index 1230546..537f48e 100644 --- a/src/ninja_jumble.cc +++ b/src/ninja_jumble.cc @@ -107,17 +107,17 @@ bool RealDiskInterface::MakeDir(const string& path) { return true; } -bool RealDiskInterface::RemoveFile(const string& path) { +int RealDiskInterface::RemoveFile(const string& path) { if (remove(path.c_str()) < 0) { switch (errno) { case ENOENT: - return false; + return 1; default: Error("remove(%s): %s", path.c_str(), strerror(errno)); - return false; + return -1; } } else { - return true; + return 0; } } diff --git a/src/ninja_test.cc b/src/ninja_test.cc index e8e7f08..8d53343 100644 --- a/src/ninja_test.cc +++ b/src/ninja_test.cc @@ -114,9 +114,9 @@ struct StatTest : public StateTestWithBuiltinRules, assert(false); return ""; } - virtual bool RemoveFile(const string& path) { + virtual int RemoveFile(const string& path) { assert(false); - return false; + return 0; } map mtimes_; @@ -262,6 +262,7 @@ TEST_F(DiskInterfaceTest, RemoveFile) { string cmd = "touch "; cmd += kFileName; ASSERT_EQ(0, system(cmd.c_str())); - EXPECT_TRUE(disk_.RemoveFile(kFileName)); - EXPECT_FALSE(disk_.RemoveFile(kFileName)); + EXPECT_EQ(0, disk_.RemoveFile(kFileName)); + EXPECT_EQ(1, disk_.RemoveFile(kFileName)); + EXPECT_EQ(-1, disk_.RemoveFile("does not exist")); } diff --git a/src/test.cc b/src/test.cc index 4dc2f47..719cec3 100644 --- a/src/test.cc +++ b/src/test.cc @@ -59,13 +59,16 @@ string VirtualFileSystem::ReadFile(const string& path, string* err) { return ""; } -bool VirtualFileSystem::RemoveFile(const string& path) { +int VirtualFileSystem::RemoveFile(const string& path) { + if (find(directories_made_.begin(), directories_made_.end(), path) + != directories_made_.end()) + return -1; FileMap::iterator i = files_.find(path); if (i != files_.end()) { files_.erase(i); files_removed_.insert(path); - return true; + return 0; } else { - return false; + return 1; } } diff --git a/src/test.h b/src/test.h index 0af8874..c516508 100644 --- a/src/test.h +++ b/src/test.h @@ -45,7 +45,7 @@ struct VirtualFileSystem : public DiskInterface { virtual int Stat(const string& path); virtual bool MakeDir(const string& path); virtual string ReadFile(const string& path, string* err); - virtual bool RemoveFile(const string& path); + virtual int RemoveFile(const string& path); /// An entry for a single in-memory file. struct Entry { -- cgit v0.12