bugfix: Also unlink symlinks before installing

Make sure that all CopyFile, WriteFile, and CreateSymlink
functions properly unlink the target file (if it exists and
overwrite requested) to avoid interferences of the install
command. With this change, the clean up step for install-cas
and the within GraphTraverser can new be omitted.

(cherry-picked from 04e2f0aa0ccfe4f39c5f6c713bde182c6b7704dd)

4 files changed, 30 insertions, 19 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 22ea953c..2d237475 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -21,6 +21,8 @@ Bug fixes on top of release `1.2.2`.
   some targets.
 - Fixed a race condition in an internal cache of `just execute` used for keeping
   track of running operations.
+- Also symlinks are properly unlinked before installing on that
+  location.
 
 ## Release `1.2.2` (2023-10-17)
 
diff --git a/src/buildtool/file_system/file_system_manager.hpp b/src/buildtool/file_system/file_system_manager.hpp
index 6b6b675f..d905f301 100644
--- a/src/buildtool/file_system/file_system_manager.hpp
+++ b/src/buildtool/file_system/file_system_manager.hpp
@@ -153,6 +153,11 @@ class FileSystemManager {
                             link.parent_path().string());
                 return false;
             }
+            if (not RemoveFile(link)) {
+                Logger::Log(
+                    log_failure_at, "can not remove file {}", link.string());
+                return false;
+            }
 #ifdef __unix__
             std::filesystem::create_directory_symlink(to, link);
             return std::filesystem::is_symlink(link);
@@ -344,7 +349,10 @@ class FileSystemManager {
                                   kSetEpochTime,
                                   kSetWritable>(src, dst, fd_less, opt);
             case ObjectType::Symlink:
-                return CopySymlinkAs<kSetEpochTime>(src, dst);
+                return CopySymlinkAs<kSetEpochTime>(
+                    src,
+                    dst,
+                    opt == std::filesystem::copy_options::overwrite_existing);
             case ObjectType::Tree:
                 break;
         }
@@ -441,7 +449,8 @@ class FileSystemManager {
             if (!std::filesystem::exists(status)) {
                 return true;
             }
-            if (!std::filesystem::is_regular_file(status)) {
+            if (!std::filesystem::is_regular_file(status) and
+                !std::filesystem::is_symlink(status)) {
                 return false;
             }
             return std::filesystem::remove(file);
@@ -1009,6 +1018,11 @@ class FileSystemManager {
                 std::filesystem::is_symlink(dst)) {
                 return false;
             }
+            if (not RemoveFile(dst)) {
+                Logger::Log(
+                    LogLevel::Error, "cannot remove file {}", dst.string());
+                return false;
+            }
             return std::filesystem::copy_file(src, dst, opt);
         } catch (std::exception const& e) {
             Logger::Log(LogLevel::Error,
@@ -1023,6 +1037,11 @@ class FileSystemManager {
     [[nodiscard]] static auto WriteFileImpl(
         std::string const& content,
         std::filesystem::path const& file) noexcept -> bool {
+        if (not FileSystemManager::RemoveFile(file)) {
+            Logger::Log(
+                LogLevel::Error, "can not remove file {}", file.string());
+            return false;
+        }
         try {
             std::ofstream writer{file};
             if (!writer.is_open()) {
@@ -1139,6 +1158,12 @@ class FileSystemManager {
         [[nodiscard]] static auto CopyFile(char const* src,
                                            char const* dst,
                                            bool skip_existing) noexcept -> int {
+            if (not skip_existing) {
+                // remove dst if it exists
+                if (unlink(dst) != 0 and errno != ENOENT) {
+                    return PackError(ERROR_OPEN_OUTPUT, errno);
+                }
+            }
             // NOLINTNEXTLINE(hicpp-signed-bitwise)
             auto write_flags = kWriteFlags | (skip_existing ? O_EXCL : 0);
             auto out = FdOpener{dst, write_flags, kWritePerms};
diff --git a/src/buildtool/graph_traverser/graph_traverser.hpp b/src/buildtool/graph_traverser/graph_traverser.hpp
index 9f99e6f2..e2587701 100644
--- a/src/buildtool/graph_traverser/graph_traverser.hpp
+++ b/src/buildtool/graph_traverser/graph_traverser.hpp
@@ -532,15 +532,7 @@ class GraphTraverser {
         std::vector<std::filesystem::path> output_paths{};
         output_paths.reserve(rel_paths.size());
         for (auto const& rel_path : rel_paths) {
-            auto output_path = clargs_.stage->output_dir / rel_path;
-            if (FileSystemManager::IsFile(output_path) and
-                not FileSystemManager::RemoveFile(output_path)) {
-                Logger::Log(LogLevel::Error,
-                            "Could not clean output path {}",
-                            output_path.string());
-                return std::nullopt;
-            }
-            output_paths.emplace_back(std::move(output_path));
+            output_paths.emplace_back(clargs_.stage->output_dir / rel_path);
         }
         return output_paths;
     }
diff --git a/src/buildtool/main/install_cas.cpp b/src/buildtool/main/install_cas.cpp
index 622e7a1f..b0a2d2f6 100644
--- a/src/buildtool/main/install_cas.cpp
+++ b/src/buildtool/main/install_cas.cpp
@@ -103,14 +103,6 @@ auto FetchAndInstallArtifacts(
                         output_path.parent_path().string());
             return false;
         }
-        if (FileSystemManager::Exists(output_path)) {
-            if (not FileSystemManager::RemoveFile(output_path)) {
-                Logger::Log(LogLevel::Error,
-                            "Failed to remote target location {}.",
-                            output_path.string());
-                return false;
-            }
-        }
         if (not api->RetrieveToPaths(
                 {object_info}, {output_path}, alternative_api)) {
             Logger::Log(LogLevel::Error, "failed to retrieve artifact.");