diff --git a/include/vcpkg/archives.h b/include/vcpkg/archives.h index aeb9b7a832..ce1d05feaa 100644 --- a/include/vcpkg/archives.h +++ b/include/vcpkg/archives.h @@ -36,12 +36,7 @@ namespace vcpkg MessageSink& status_sink, const Path& archive, const Path& to_path); - // set `to_path` to `archive` contents. - void set_directory_to_archive_contents(const Filesystem& fs, - const ToolCache& tools, - MessageSink& status_sink, - const Path& archive, - const Path& to_path); + // extract `archive` to a sibling temporary subdirectory of `to_path` and returns that path Path extract_archive_to_temp_subdirectory(const Filesystem& fs, const ToolCache& tools, MessageSink& status_sink, diff --git a/include/vcpkg/base/files.h b/include/vcpkg/base/files.h index 7b43319223..944f3db093 100644 --- a/include/vcpkg/base/files.h +++ b/include/vcpkg/base/files.h @@ -246,6 +246,16 @@ namespace vcpkg void rename_with_retry(const Path& old_path, const Path& new_path, std::error_code& ec) const; void rename_with_retry(const Path& old_path, const Path& new_path, LineInfo li) const; + // Rename old_path -> new_path, but consider new_path already existing as acceptable. + // Traditionally used to interact with downloads, or git tree cache, where multiple + // instances of vcpkg may be trying to do the same action at the same time. + // + // Returns whether the rename actually happened. + // + // If `old_path` and `new_path` resolve to the same file, the behavior is undefined. + bool rename_cas_like(const Path& old_path, const Path& new_path, std::error_code& ec) const; + bool rename_cas_like(const Path& old_path, const Path& new_path, LineInfo li) const; + virtual void rename_or_copy(const Path& old_path, const Path& new_path, StringLiteral temp_suffix, diff --git a/src/vcpkg-test/files.cpp b/src/vcpkg-test/files.cpp index 08d5affb5e..7544893ed0 100644 --- a/src/vcpkg-test/files.cpp +++ b/src/vcpkg-test/files.cpp @@ -914,6 +914,100 @@ TEST_CASE ("copy_file", "[files]") CHECK_EC_ON_FILE(temp_dir, ec); } +TEST_CASE ("rename", "[files]") +{ + urbg_t urbg; + + auto& fs = setup(); + + auto temp_dir = base_temporary_directory() / get_random_filename(urbg); + INFO("temp dir is: " << temp_dir.native()); + + static constexpr StringLiteral FileTxt = "file.txt"; + fs.remove_all(temp_dir, VCPKG_LINE_INFO); + fs.create_directory(temp_dir, VCPKG_LINE_INFO); + auto temp_dir_a = temp_dir / "a"; + fs.create_directory(temp_dir_a, VCPKG_LINE_INFO); + auto temp_dir_a_file = temp_dir_a / FileTxt; + auto temp_dir_b = temp_dir / "b"; + auto temp_dir_b_file = temp_dir_b / FileTxt; + + static constexpr StringLiteral text_file_contents = "hello there"; + fs.write_contents(temp_dir_a_file, text_file_contents, VCPKG_LINE_INFO); + + // try rename_with_retry + { + fs.rename_with_retry(temp_dir_a, temp_dir_b, VCPKG_LINE_INFO); + REQUIRE(!fs.exists(temp_dir_a, VCPKG_LINE_INFO)); + REQUIRE(fs.read_contents(temp_dir_b_file, VCPKG_LINE_INFO) == text_file_contents); + + // put things back + fs.rename(temp_dir_b, temp_dir_a, VCPKG_LINE_INFO); + REQUIRE(fs.read_contents(temp_dir_a_file, VCPKG_LINE_INFO) == text_file_contents); + REQUIRE(!fs.exists(temp_dir_b, VCPKG_LINE_INFO)); + } + + // try rename_cas_like directory, target does not exist + { + REQUIRE(fs.rename_cas_like(temp_dir_a, temp_dir_b, VCPKG_LINE_INFO)); + REQUIRE(!fs.exists(temp_dir_a, VCPKG_LINE_INFO)); + REQUIRE(fs.read_contents(temp_dir_b_file, VCPKG_LINE_INFO) == text_file_contents); + + // put things back + fs.rename(temp_dir_b, temp_dir_a, VCPKG_LINE_INFO); + REQUIRE(fs.read_contents(temp_dir_a_file, VCPKG_LINE_INFO) == text_file_contents); + REQUIRE(!fs.exists(temp_dir_b, VCPKG_LINE_INFO)); + } + + // try rename_cas_like directory, target exists + { + fs.create_directory(temp_dir_b, VCPKG_LINE_INFO); + fs.write_contents(temp_dir_b_file, text_file_contents, VCPKG_LINE_INFO); + + // Note that the VCPKG_LINE_INFO overload implicitly tests that ec got cleared + REQUIRE(!fs.rename_cas_like(temp_dir_a, temp_dir_b, VCPKG_LINE_INFO)); + REQUIRE(!fs.exists(temp_dir_a, VCPKG_LINE_INFO)); + REQUIRE(fs.read_contents(temp_dir_b_file, VCPKG_LINE_INFO) == text_file_contents); + + // put things back + fs.rename(temp_dir_b, temp_dir_a, VCPKG_LINE_INFO); + REQUIRE(fs.read_contents(temp_dir_a_file, VCPKG_LINE_INFO) == text_file_contents); + REQUIRE(!fs.exists(temp_dir_b, VCPKG_LINE_INFO)); + } + + // try rename_cas_like file, target does not exist + { + fs.create_directory(temp_dir_b, VCPKG_LINE_INFO); + REQUIRE(fs.rename_cas_like(temp_dir_a_file, temp_dir_b_file, VCPKG_LINE_INFO)); + REQUIRE(!fs.exists(temp_dir_a_file, VCPKG_LINE_INFO)); + REQUIRE(fs.read_contents(temp_dir_b_file, VCPKG_LINE_INFO) == text_file_contents); + + // put things back + fs.rename(temp_dir_b_file, temp_dir_a_file, VCPKG_LINE_INFO); + REQUIRE(fs.read_contents(temp_dir_a_file, VCPKG_LINE_INFO) == text_file_contents); + REQUIRE(!fs.exists(temp_dir_b_file, VCPKG_LINE_INFO)); + fs.remove(temp_dir_b, VCPKG_LINE_INFO); + } + + // try rename_cas_like file, target exists + { + fs.create_directory(temp_dir_b, VCPKG_LINE_INFO); + fs.write_contents(temp_dir_b_file, text_file_contents, VCPKG_LINE_INFO); + // Note that the VCPKG_LINE_INFO overload implicitly tests that ec got cleared + // Also note that POSIX rename() will just delete the target like we want by itself so + // this returns true. + REQUIRE(fs.rename_cas_like(temp_dir_a_file, temp_dir_b_file, VCPKG_LINE_INFO)); + REQUIRE(!fs.exists(temp_dir_a_file, VCPKG_LINE_INFO)); + REQUIRE(fs.read_contents(temp_dir_b_file, VCPKG_LINE_INFO) == text_file_contents); + + // put things back + fs.rename(temp_dir_b_file, temp_dir_a_file, VCPKG_LINE_INFO); + REQUIRE(fs.read_contents(temp_dir_a_file, VCPKG_LINE_INFO) == text_file_contents); + REQUIRE(!fs.exists(temp_dir_b_file, VCPKG_LINE_INFO)); + fs.remove(temp_dir_b, VCPKG_LINE_INFO); + } +} + TEST_CASE ("copy_symlink", "[files]") { urbg_t urbg; diff --git a/src/vcpkg/archives.cpp b/src/vcpkg/archives.cpp index 802d6b3f22..6ce10f0b42 100644 --- a/src/vcpkg/archives.cpp +++ b/src/vcpkg/archives.cpp @@ -310,18 +310,6 @@ namespace vcpkg msg::path = archive); } - void set_directory_to_archive_contents(const Filesystem& fs, - const ToolCache& tools, - MessageSink& status_sink, - const Path& archive, - const Path& to_path) - - { - fs.remove_all(to_path, VCPKG_LINE_INFO); - Path to_path_partial = extract_archive_to_temp_subdirectory(fs, tools, status_sink, archive, to_path); - fs.rename_with_retry(to_path_partial, to_path, VCPKG_LINE_INFO); - } - bool ZipTool::compress_directory_to_zip(DiagnosticContext& context, const Filesystem& fs, const Path& source, diff --git a/src/vcpkg/base/files.cpp b/src/vcpkg/base/files.cpp index 73139ad88c..48d2ce025f 100644 --- a/src/vcpkg/base/files.cpp +++ b/src/vcpkg/base/files.cpp @@ -2008,6 +2008,44 @@ namespace vcpkg } } + bool Filesystem::rename_cas_like(const Path& old_path, const Path& new_path, LineInfo li) const + { + std::error_code ec; + bool result = this->rename_cas_like(old_path, new_path, ec); + if (ec) + { + exit_filesystem_call_error(li, ec, __func__, {old_path, new_path}); + } + + return result; + } + + bool Filesystem::rename_cas_like(const Path& old_path, const Path& new_path, std::error_code& ec) const + { + this->rename(old_path, new_path, ec); + using namespace std::chrono_literals; + for (const auto& delay : {10ms, 100ms, 1000ms, 10000ms}) + { + if (!ec) + { + return true; + } + else if (ec == std::make_error_condition(std::errc::directory_not_empty) || + ec == std::make_error_condition(std::errc::file_exists) || this->exists(new_path, ec)) + { + // either the rename failed with a target already exists error, or the target explicitly exists, + // assume another process 'won' the 'CAS'. + this->remove_all(old_path, ec); + return false; + } + + std::this_thread::sleep_for(delay); + this->rename(old_path, new_path, ec); + } + + return false; + } + bool Filesystem::remove(const Path& target, LineInfo li) const { std::error_code ec; diff --git a/src/vcpkg/configure-environment.cpp b/src/vcpkg/configure-environment.cpp index 97df6be9fc..42fa377c16 100644 --- a/src/vcpkg/configure-environment.cpp +++ b/src/vcpkg/configure-environment.cpp @@ -155,7 +155,8 @@ namespace vcpkg auto& fs = paths.get_filesystem(); // if artifacts is deployed in development, with Visual Studio, or with the One Liner, it will be deployed here - Path vcpkg_artifacts_path = get_exe_path_of_current_process(); + const Path exe_path = get_exe_path_of_current_process(); + Path vcpkg_artifacts_path = exe_path; vcpkg_artifacts_path.replace_filename("vcpkg-artifacts"); vcpkg_artifacts_path.make_preferred(); Path vcpkg_artifacts_main_path = vcpkg_artifacts_path / "main.js"; @@ -177,9 +178,9 @@ namespace vcpkg #endif // ^^^ !VCPKG_STANDALONE_BUNDLE_SHA if (out_of_date) { - fs.remove_all(vcpkg_artifacts_path, VCPKG_LINE_INFO); - auto temp = get_exe_path_of_current_process(); - temp.replace_filename("vcpkg-artifacts-temp"); + // This is racy; the reason for the temp-path-then-rename dance is to get only the + // 'vcpkg-artifacts' directory out of the standalone bundle, while extracting + // the standalone bundle over VCPKG_ROOT would overwrite scripts/triplets/etc. auto maybe_tarball = download_vcpkg_standalone_bundle( console_diagnostic_context, paths.get_asset_cache_settings(), fs, paths.downloads); auto tarball = maybe_tarball.get(); @@ -188,10 +189,12 @@ namespace vcpkg Checks::exit_fail(VCPKG_LINE_INFO); } - set_directory_to_archive_contents(fs, paths.get_tool_cache(), null_sink, *tarball, temp); + Path temp = extract_archive_to_temp_subdirectory( + fs, paths.get_tool_cache(), null_sink, *tarball, vcpkg_artifacts_path); + fs.remove_all(vcpkg_artifacts_path, VCPKG_LINE_INFO); fs.rename_with_retry(temp / "vcpkg-artifacts", vcpkg_artifacts_path, VCPKG_LINE_INFO); - fs.remove(*tarball, VCPKG_LINE_INFO); fs.remove_all(temp, VCPKG_LINE_INFO); + fs.remove(*tarball, VCPKG_LINE_INFO); #if defined(VCPKG_STANDALONE_BUNDLE_SHA) fs.write_contents(vcpkg_artifacts_version_path, VCPKG_BASE_VERSION_AS_STRING, VCPKG_LINE_INFO); #endif // ^^^ VCPKG_STANDALONE_BUNDLE_SHA @@ -233,7 +236,7 @@ namespace vcpkg } cmd.string_arg("--vcpkg-root").string_arg(paths.root); - cmd.string_arg("--z-vcpkg-command").string_arg(get_exe_path_of_current_process()); + cmd.string_arg("--z-vcpkg-command").string_arg(exe_path); cmd.string_arg("--z-vcpkg-artifacts-root").string_arg(paths.artifacts()); cmd.string_arg("--z-vcpkg-downloads").string_arg(paths.downloads); diff --git a/src/vcpkg/tools.cpp b/src/vcpkg/tools.cpp index 2927a67682..21236e1a6b 100644 --- a/src/vcpkg/tools.cpp +++ b/src/vcpkg/tools.cpp @@ -871,12 +871,14 @@ namespace vcpkg if (tool_data.is_archive) { status_sink.println(Color::none, msgExtractingTool, msg::tool_name = tool_data.name); - set_directory_to_archive_contents(fs, *this, status_sink, download_path, tool_dir_path); + Path to_path_partial = + extract_archive_to_temp_subdirectory(fs, *this, status_sink, download_path, tool_dir_path); + fs.rename_cas_like(to_path_partial, tool_dir_path, IgnoreErrors{}); } else { fs.create_directories(exe_path.parent_path(), IgnoreErrors{}); - fs.rename(download_path, exe_path, IgnoreErrors{}); + fs.rename_cas_like(download_path, exe_path, IgnoreErrors{}); } if (!fs.exists(exe_path, IgnoreErrors{})) diff --git a/src/vcpkg/vcpkgpaths.cpp b/src/vcpkg/vcpkgpaths.cpp index f92dd29126..4938ce8e99 100644 --- a/src/vcpkg/vcpkgpaths.cpp +++ b/src/vcpkg/vcpkgpaths.cpp @@ -1199,12 +1199,6 @@ namespace vcpkg return format_filesystem_call_error(ec, "create_directory", {git_tree_temp}); } - fs.remove_all(destination, ec); - if (ec) - { - return format_filesystem_call_error(ec, "remove_all", {destination}); - } - auto git_archive = git_cmd_builder(dot_git_dir, git_tree_temp) .string_arg("read-tree") .string_arg("-m") @@ -1232,11 +1226,11 @@ namespace vcpkg return error; } - fs.rename_with_retry(git_tree_temp, destination, ec); + fs.rename_cas_like(git_tree_temp, destination, ec); if (ec) { return error_prefix().append( - format_filesystem_call_error(ec, "rename_with_retry", {git_tree_temp, destination})); + format_filesystem_call_error(ec, "rename_cas_like", {git_tree_temp, destination})); } fs.remove(git_tree_index, IgnoreErrors{});