From 8b57548de9836fce3663b99d0140146f12668e8c Mon Sep 17 00:00:00 2001 From: Maksim Denisov Date: Thu, 17 Apr 2025 16:56:57 +0200 Subject: FileSystemManager: Always remove directories recursively --- src/buildtool/execution_api/local/local_action.cpp | 4 ++-- src/buildtool/file_system/file_system_manager.hpp | 15 +++++++-------- src/buildtool/storage/compactifier.cpp | 3 +-- src/buildtool/storage/garbage_collector.cpp | 3 +-- src/buildtool/storage/repository_garbage_collector.cpp | 6 ++---- src/utils/cpp/tmp_dir.cpp | 2 +- 6 files changed, 14 insertions(+), 19 deletions(-) (limited to 'src') diff --git a/src/buildtool/execution_api/local/local_action.cpp b/src/buildtool/execution_api/local/local_action.cpp index 9884a5ec..7ea0ce34 100644 --- a/src/buildtool/execution_api/local/local_action.cpp +++ b/src/buildtool/execution_api/local/local_action.cpp @@ -55,7 +55,7 @@ class BuildCleanupAnchor { auto operator=(BuildCleanupAnchor const&) -> BuildCleanupAnchor& = delete; auto operator=(BuildCleanupAnchor&&) -> BuildCleanupAnchor& = delete; ~BuildCleanupAnchor() { - if (not FileSystemManager::RemoveDirectory(build_path_, true)) { + if (not FileSystemManager::RemoveDirectory(build_path_)) { Logger::Log(LogLevel::Error, "Could not cleanup build directory {}", build_path_.string()); @@ -347,7 +347,7 @@ auto LocalAction::StageInputs( auto LocalAction::CreateDirectoryStructure( std::filesystem::path const& exec_path) const noexcept -> bool { // clean execution directory - if (not FileSystemManager::RemoveDirectory(exec_path, true)) { + if (not FileSystemManager::RemoveDirectory(exec_path)) { logger_.Emit(LogLevel::Error, "failed to clean exec_path"); return false; } diff --git a/src/buildtool/file_system/file_system_manager.hpp b/src/buildtool/file_system/file_system_manager.hpp index fe316451..a6d15c84 100644 --- a/src/buildtool/file_system/file_system_manager.hpp +++ b/src/buildtool/file_system/file_system_manager.hpp @@ -44,6 +44,7 @@ #include #include #include +#include #include #include #include @@ -510,9 +511,9 @@ class FileSystemManager { } } - [[nodiscard]] static auto RemoveDirectory(std::filesystem::path const& dir, - bool recursively = false) noexcept - -> bool { + /// \brief Remove directory recursively. + [[nodiscard]] static auto RemoveDirectory( + std::filesystem::path const& dir) noexcept -> bool { try { auto status = std::filesystem::symlink_status(dir); if (not std::filesystem::exists(status)) { @@ -521,11 +522,9 @@ class FileSystemManager { if (not std::filesystem::is_directory(status)) { return false; } - if (recursively) { - return (std::filesystem::remove_all(dir) != - static_cast(-1)); - } - return std::filesystem::remove(dir); + // If it doesn't throw, it succeeds: + std::ignore = std::filesystem::remove_all(dir); + return true; } catch (std::exception const& e) { Logger::Log(LogLevel::Error, "removing directory {}:\n{}", diff --git a/src/buildtool/storage/compactifier.cpp b/src/buildtool/storage/compactifier.cpp index 66fa6035..ab1f557e 100644 --- a/src/buildtool/storage/compactifier.cpp +++ b/src/buildtool/storage/compactifier.cpp @@ -163,8 +163,7 @@ template std::string const d_name = directory.filename(); if (d_name.size() != FileStorageData::kDirectoryNameLength or not FromHexString(d_name)) { - static constexpr bool kRecursively = true; - if (FileSystemManager::RemoveDirectory(directory, kRecursively)) { + if (FileSystemManager::RemoveDirectory(directory)) { return true; } diff --git a/src/buildtool/storage/garbage_collector.cpp b/src/buildtool/storage/garbage_collector.cpp index f1254eed..1ac1bf93 100644 --- a/src/buildtool/storage/garbage_collector.cpp +++ b/src/buildtool/storage/garbage_collector.cpp @@ -39,8 +39,7 @@ auto RemoveDirs(const std::vector& directories) -> bool { bool success = true; for (auto const& d : directories) { if (FileSystemManager::IsDirectory(d)) { - if (not FileSystemManager::RemoveDirectory(d, - /*recursively=*/true)) { + if (not FileSystemManager::RemoveDirectory(d)) { Logger::Log(LogLevel::Warning, "Failed to remove directory {}", d.string()); diff --git a/src/buildtool/storage/repository_garbage_collector.cpp b/src/buildtool/storage/repository_garbage_collector.cpp index be3d7857..95b90f9d 100644 --- a/src/buildtool/storage/repository_garbage_collector.cpp +++ b/src/buildtool/storage/repository_garbage_collector.cpp @@ -60,8 +60,7 @@ auto RepositoryGarbageCollector::TriggerGarbageCollection( return false; } if (FileSystemManager::IsDirectory(remove_me)) { - if (not FileSystemManager::RemoveDirectory(remove_me, - /*recursively=*/true)) { + if (not FileSystemManager::RemoveDirectory(remove_me)) { Logger::Log(LogLevel::Error, "Failed to remove directory {}", remove_me.string()); @@ -136,8 +135,7 @@ auto RepositoryGarbageCollector::TriggerGarbageCollection( return false; } if (FileSystemManager::IsDirectory(remove_me)) { - if (not FileSystemManager::RemoveDirectory(remove_me, - /*recursively=*/true)) { + if (not FileSystemManager::RemoveDirectory(remove_me)) { Logger::Log(LogLevel::Error, "Failed to remove directory {}", remove_me.string()); diff --git a/src/utils/cpp/tmp_dir.cpp b/src/utils/cpp/tmp_dir.cpp index 8d6e0724..9a38aca2 100644 --- a/src/utils/cpp/tmp_dir.cpp +++ b/src/utils/cpp/tmp_dir.cpp @@ -93,5 +93,5 @@ auto TmpDir::CreateImpl(TmpDir::Ptr parent, TmpDir::~TmpDir() noexcept { // try to remove the tmp dir and all its content - std::ignore = FileSystemManager::RemoveDirectory(tmp_dir_, true); + std::ignore = FileSystemManager::RemoveDirectory(tmp_dir_); } -- cgit v1.2.3