From 45376637404bbb971fc09e80fe3f7d924cb152ef Mon Sep 17 00:00:00 2001 From: siddhantCodes Date: Sun, 12 May 2024 18:40:16 +0530 Subject: [PATCH 1/4] inline the usage of `nix::renameFile` use `std::filesystem::rename` everywhere and remove `nix::renameFile` --- src/libstore/indirect-root-store.cc | 2 +- src/libstore/local-binary-cache-store.cc | 2 +- src/libstore/local-store.cc | 2 +- src/libstore/unix/build/derivation-goal.cc | 2 +- src/libstore/unix/build/local-derivation-goal.cc | 6 +++--- src/libstore/unix/builtins/unpack-channel.cc | 2 +- src/libutil/file-system.cc | 11 +++-------- src/libutil/file-system.hh | 2 -- 8 files changed, 11 insertions(+), 18 deletions(-) diff --git a/src/libstore/indirect-root-store.cc b/src/libstore/indirect-root-store.cc index 082a458ab..844d0d6ed 100644 --- a/src/libstore/indirect-root-store.cc +++ b/src/libstore/indirect-root-store.cc @@ -12,7 +12,7 @@ void IndirectRootStore::makeSymlink(const Path & link, const Path & target) createSymlink(target, tempLink); /* Atomically replace the old one. */ - renameFile(tempLink, link); + std::filesystem::rename(tempLink, link); } Path IndirectRootStore::addPermRoot(const StorePath & storePath, const Path & _gcRoot) diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index 3a48f4480..6743c633e 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -64,7 +64,7 @@ protected: AutoDelete del(tmp, false); StreamToSourceAdapter source(istream); writeFile(tmp, source); - renameFile(tmp, path2); + std::filesystem::rename(tmp, path2); del.cancel(); } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 800e69309..1f31f9bd8 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1779,7 +1779,7 @@ void LocalStore::addBuildLog(const StorePath & drvPath, std::string_view log) writeFile(tmpFile, compress("bzip2", log)); - renameFile(tmpFile, logPath); + std::filesystem::rename(tmpFile, logPath); } std::optional LocalStore::getVersion() diff --git a/src/libstore/unix/build/derivation-goal.cc b/src/libstore/unix/build/derivation-goal.cc index 8d6e35015..89518b055 100644 --- a/src/libstore/unix/build/derivation-goal.cc +++ b/src/libstore/unix/build/derivation-goal.cc @@ -783,7 +783,7 @@ static void movePath(const Path & src, const Path & dst) if (changePerm) chmod_(src, st.st_mode | S_IWUSR); - renameFile(src, dst); + std::filesystem::rename(src, dst); if (changePerm) chmod_(dst, st.st_mode); diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 3b010350d..331794292 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -285,7 +285,7 @@ static void movePath(const Path & src, const Path & dst) if (changePerm) chmod_(src, st.st_mode | S_IWUSR); - renameFile(src, dst); + std::filesystem::rename(src, dst); if (changePerm) chmod_(dst, st.st_mode); @@ -372,7 +372,7 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull() if (buildMode != bmCheck && status.known->isValid()) continue; auto p = worker.store.toRealPath(status.known->path); if (pathExists(chrootRootDir + p)) - renameFile((chrootRootDir + p), p); + std::filesystem::rename((chrootRootDir + p), p); } return diskFull; @@ -2569,7 +2569,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() // that there's no stale file descriptor pointing to it Path tmpOutput = actualPath + ".tmp"; copyFile(actualPath, tmpOutput, true); - renameFile(tmpOutput, actualPath); + std::filesystem::rename(tmpOutput, actualPath); auto newInfo0 = newInfoFromCA(DerivationOutput::CAFloating { .method = dof.ca.method, diff --git a/src/libstore/unix/builtins/unpack-channel.cc b/src/libstore/unix/builtins/unpack-channel.cc index 47bf5d49c..bdc724a70 100644 --- a/src/libstore/unix/builtins/unpack-channel.cc +++ b/src/libstore/unix/builtins/unpack-channel.cc @@ -24,7 +24,7 @@ void builtinUnpackChannel( auto entries = readDirectory(out); if (entries.size() != 1) throw Error("channel tarball '%s' contains more than one file", src); - renameFile(entries[0].path().string(), (out + "/" + channelName)); + std::filesystem::rename(entries[0].path().string(), (out + "/" + channelName)); } } diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 39efa19fe..b9f96b88f 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -588,7 +588,7 @@ void replaceSymlink(const Path & target, const Path & link) throw; } - renameFile(tmp, link); + std::filesystem::rename(tmp, link); break; } @@ -651,15 +651,10 @@ void copyFile(const Path & oldPath, const Path & newPath, bool andDelete) return copy(fs::directory_entry(fs::path(oldPath)), fs::path(newPath), andDelete); } -void renameFile(const Path & oldName, const Path & newName) -{ - fs::rename(oldName, newName); -} - void moveFile(const Path & oldName, const Path & newName) { try { - renameFile(oldName, newName); + std::filesystem::rename(oldName, newName); } catch (fs::filesystem_error & e) { auto oldPath = fs::path(oldName); auto newPath = fs::path(newName); @@ -674,7 +669,7 @@ void moveFile(const Path & oldName, const Path & newName) fs::remove(newPath); warn("Can’t rename %s as %s, copying instead", oldName, newName); copy(fs::directory_entry(oldPath), tempCopyTarget, true); - renameFile( + std::filesystem::rename( os_string_to_string(PathViewNG { tempCopyTarget }), os_string_to_string(PathViewNG { newPath })); } diff --git a/src/libutil/file-system.hh b/src/libutil/file-system.hh index 4536accc3..9f3db05c3 100644 --- a/src/libutil/file-system.hh +++ b/src/libutil/file-system.hh @@ -177,8 +177,6 @@ void createSymlink(const Path & target, const Path & link); */ void replaceSymlink(const Path & target, const Path & link); -void renameFile(const Path & src, const Path & dst); - /** * Similar to 'renameFile', but fallback to a copy+remove if `src` and `dst` * are on a different filesystem. From d3b7367c802337d5430103bff550b3c447cc5b0a Mon Sep 17 00:00:00 2001 From: siddhantCodes Date: Sun, 12 May 2024 18:58:05 +0530 Subject: [PATCH 2/4] inline usage of `nix::getFileType` and remove it --- src/libstore/gc.cc | 2 +- src/libutil/file-system.cc | 6 ------ src/libutil/file-system.hh | 2 -- 3 files changed, 1 insertion(+), 9 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 3cd4fb839..877608a84 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -225,7 +225,7 @@ void LocalStore::findRoots(const Path & path, std::filesystem::file_type type, R try { if (type == std::filesystem::file_type::unknown) - type = getFileType(path); + type = std::filesystem::symlink_status(path).type(); if (type == std::filesystem::file_type::directory) { for (auto & i : readDirectory(path)) diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index b9f96b88f..cc93da996 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -236,12 +236,6 @@ std::vector readDirectory(const Path & path) } -fs::file_type getFileType(const Path & path) -{ - return fs::symlink_status(path).type(); -} - - std::string readFile(const Path & path) { AutoCloseFD fd = toDescriptor(open(path.c_str(), O_RDONLY diff --git a/src/libutil/file-system.hh b/src/libutil/file-system.hh index 9f3db05c3..669de704b 100644 --- a/src/libutil/file-system.hh +++ b/src/libutil/file-system.hh @@ -128,8 +128,6 @@ Descriptor openDirectory(const std::filesystem::path & path); */ std::vector readDirectory(const Path & path); -std::filesystem::file_type getFileType(const Path & path); - /** * Read the contents of a file into a string. */ From ccf94545db454c6b2b964cee8a72835764f64898 Mon Sep 17 00:00:00 2001 From: siddhantCodes Date: Sun, 12 May 2024 19:20:17 +0530 Subject: [PATCH 3/4] rename `copy` -> `copyFile` and remove old copyFile the old `copyFile` was just a wrapper that was calling the `copy` function. This wrapper function is removed and the `copy` function is renamed to `copyFile`. --- src/libstore/unix/build/local-derivation-goal.cc | 9 +++++++-- src/libutil/file-system.cc | 11 +++-------- src/libutil/file-system.hh | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 331794292..9e2b0bd34 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -421,7 +421,9 @@ static void doBind(const Path & source, const Path & target, bool optional = fal } else if (S_ISLNK(st.st_mode)) { // Symlinks can (apparently) not be bind-mounted, so just copy it createDirs(dirOf(target)); - copyFile(source, target, /* andDelete */ false); + copyFile( + std::filesystem::directory_entry(std::filesystem::path(source)), + std::filesystem::path(target), false); } else { createDirs(dirOf(target)); writeFile(target, ""); @@ -2568,7 +2570,10 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() // Replace the output by a fresh copy of itself to make sure // that there's no stale file descriptor pointing to it Path tmpOutput = actualPath + ".tmp"; - copyFile(actualPath, tmpOutput, true); + copyFile( + std::filesystem::directory_entry(std::filesystem::path(actualPath)), + std::filesystem::path(tmpOutput), true); + std::filesystem::rename(tmpOutput, actualPath); auto newInfo0 = newInfoFromCA(DerivationOutput::CAFloating { diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index cc93da996..294887587 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -605,7 +605,7 @@ static void setWriteTime(const fs::path & p, const struct stat & st) } #endif -void copy(const fs::directory_entry & from, const fs::path & to, bool andDelete) +void copyFile(const fs::directory_entry & from, const fs::path & to, bool andDelete) { #ifndef _WIN32 // TODO: Rewrite the `is_*` to use `symlink_status()` @@ -624,7 +624,7 @@ void copy(const fs::directory_entry & from, const fs::path & to, bool andDelete) } else if (fs::is_directory(fromStatus)) { fs::create_directory(to); for (auto & entry : fs::directory_iterator(from.path())) { - copy(entry, to / entry.path().filename(), andDelete); + copyFile(entry, to / entry.path().filename(), andDelete); } } else { throw Error("file '%s' has an unsupported type", from.path()); @@ -640,11 +640,6 @@ void copy(const fs::directory_entry & from, const fs::path & to, bool andDelete) } } -void copyFile(const Path & oldPath, const Path & newPath, bool andDelete) -{ - return copy(fs::directory_entry(fs::path(oldPath)), fs::path(newPath), andDelete); -} - void moveFile(const Path & oldName, const Path & newName) { try { @@ -662,7 +657,7 @@ void moveFile(const Path & oldName, const Path & newName) if (e.code().value() == EXDEV) { fs::remove(newPath); warn("Can’t rename %s as %s, copying instead", oldName, newName); - copy(fs::directory_entry(oldPath), tempCopyTarget, true); + copyFile(fs::directory_entry(oldPath), tempCopyTarget, true); std::filesystem::rename( os_string_to_string(PathViewNG { tempCopyTarget }), os_string_to_string(PathViewNG { newPath })); diff --git a/src/libutil/file-system.hh b/src/libutil/file-system.hh index 669de704b..acc921ebe 100644 --- a/src/libutil/file-system.hh +++ b/src/libutil/file-system.hh @@ -190,7 +190,7 @@ void moveFile(const Path & src, const Path & dst); * with the guaranty that the destination will be “fresh”, with no stale inode * or file descriptor pointing to it). */ -void copyFile(const Path & oldPath, const Path & newPath, bool andDelete); +void copyFile(const std::filesystem::directory_entry & from, const std::filesystem::path & to, bool andDelete); /** * Automatic cleanup of resources. From 62e1ea2f4b563d73fac8d48feae0e9968c9c5bc9 Mon Sep 17 00:00:00 2001 From: siddhantCodes Date: Mon, 13 May 2024 16:10:21 +0530 Subject: [PATCH 4/4] use `path` for `from` arg in `nix::copyFile` --- .../unix/build/local-derivation-goal.cc | 4 ++-- src/libutil/file-system.cc | 20 +++++++++---------- src/libutil/file-system.hh | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 9e2b0bd34..16095cf5d 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -422,7 +422,7 @@ static void doBind(const Path & source, const Path & target, bool optional = fal // Symlinks can (apparently) not be bind-mounted, so just copy it createDirs(dirOf(target)); copyFile( - std::filesystem::directory_entry(std::filesystem::path(source)), + std::filesystem::path(source), std::filesystem::path(target), false); } else { createDirs(dirOf(target)); @@ -2571,7 +2571,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() // that there's no stale file descriptor pointing to it Path tmpOutput = actualPath + ".tmp"; copyFile( - std::filesystem::directory_entry(std::filesystem::path(actualPath)), + std::filesystem::path(actualPath), std::filesystem::path(tmpOutput), true); std::filesystem::rename(tmpOutput, actualPath); diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 294887587..f5628bfdb 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -605,29 +605,29 @@ static void setWriteTime(const fs::path & p, const struct stat & st) } #endif -void copyFile(const fs::directory_entry & from, const fs::path & to, bool andDelete) +void copyFile(const fs::path & from, const fs::path & to, bool andDelete) { #ifndef _WIN32 // TODO: Rewrite the `is_*` to use `symlink_status()` - auto statOfFrom = lstat(from.path().c_str()); + auto statOfFrom = lstat(from.c_str()); #endif - auto fromStatus = from.symlink_status(); + auto fromStatus = fs::symlink_status(from); // Mark the directory as writable so that we can delete its children if (andDelete && fs::is_directory(fromStatus)) { - fs::permissions(from.path(), fs::perms::owner_write, fs::perm_options::add | fs::perm_options::nofollow); + fs::permissions(from, fs::perms::owner_write, fs::perm_options::add | fs::perm_options::nofollow); } if (fs::is_symlink(fromStatus) || fs::is_regular_file(fromStatus)) { - fs::copy(from.path(), to, fs::copy_options::copy_symlinks | fs::copy_options::overwrite_existing); + fs::copy(from, to, fs::copy_options::copy_symlinks | fs::copy_options::overwrite_existing); } else if (fs::is_directory(fromStatus)) { fs::create_directory(to); - for (auto & entry : fs::directory_iterator(from.path())) { + for (auto & entry : fs::directory_iterator(from)) { copyFile(entry, to / entry.path().filename(), andDelete); } } else { - throw Error("file '%s' has an unsupported type", from.path()); + throw Error("file '%s' has an unsupported type", from); } #ifndef _WIN32 @@ -635,8 +635,8 @@ void copyFile(const fs::directory_entry & from, const fs::path & to, bool andDel #endif if (andDelete) { if (!fs::is_symlink(fromStatus)) - fs::permissions(from.path(), fs::perms::owner_write, fs::perm_options::add | fs::perm_options::nofollow); - fs::remove(from.path()); + fs::permissions(from, fs::perms::owner_write, fs::perm_options::add | fs::perm_options::nofollow); + fs::remove(from); } } @@ -657,7 +657,7 @@ void moveFile(const Path & oldName, const Path & newName) if (e.code().value() == EXDEV) { fs::remove(newPath); warn("Can’t rename %s as %s, copying instead", oldName, newName); - copyFile(fs::directory_entry(oldPath), tempCopyTarget, true); + copyFile(oldPath, tempCopyTarget, true); std::filesystem::rename( os_string_to_string(PathViewNG { tempCopyTarget }), os_string_to_string(PathViewNG { newPath })); diff --git a/src/libutil/file-system.hh b/src/libutil/file-system.hh index acc921ebe..a3f412224 100644 --- a/src/libutil/file-system.hh +++ b/src/libutil/file-system.hh @@ -190,7 +190,7 @@ void moveFile(const Path & src, const Path & dst); * with the guaranty that the destination will be “fresh”, with no stale inode * or file descriptor pointing to it). */ -void copyFile(const std::filesystem::directory_entry & from, const std::filesystem::path & to, bool andDelete); +void copyFile(const std::filesystem::path & from, const std::filesystem::path & to, bool andDelete); /** * Automatic cleanup of resources.