-
Notifications
You must be signed in to change notification settings - Fork 311
Optimize rename_or_delete to stop retrying after EXDEV #1659
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -231,15 +233,16 @@ namespace | |||
std::error_code ec; | |||
if (can_attempt_rename) | |||
{ | |||
// if we are already on the same filesystem we can just rename into place | |||
m_fs.rename_or_delete(zip_path, archive_path, ec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before #802, files were copied to the binary cache, meaning the original file in zip_path
did still exist after this function ended. However, this file is gone with rename_or_delete()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvement!
@@ -2045,6 +2045,11 @@ namespace vcpkg | |||
this->remove_all(old_path, ec); | |||
return false; | |||
} | |||
else if (ec == std::make_error_condition(std::errc::cross_device_link)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only that ec
was already lost at this point, due to L2041.
Fixes microsoft/vcpkg#45130
Please note that there is already code in place to handle
std::errc::cross_device_link
for binary caching:vcpkg-tool/src/vcpkg/binarycaching.cpp
Lines 232 to 249 in d092b51
However, copy-and-delete was only tried after calling
fs.rename_or_delete()
which tries to move the file after increasing timeouts. I decided to not move the copy-and-delete behavior tofs.rename_or_delete()
because the file is first copied to a temproary location inbinarycaching.cpp
.Files on different filesystems can't be renamed:
Linux: https://man7.org/linux/man-pages/man2/rename.2.html
Windows seems to support moving files but not folders across devices: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-movefile
std::errc::cross_device_link
: https://en.cppreference.com/w/cpp/error/errno_macrosStack Overflow: https://stackoverflow.com/questions/43206198/what-does-the-exdev-cross-device-link-not-permitted-error-mean