From ee6b22707a60f3c5edd334705b787817483dd2db Mon Sep 17 00:00:00 2001 From: Billy Robert O'Neal III Date: Wed, 8 Oct 2025 16:20:57 -0700 Subject: [PATCH 1/3] Fix ZipBinaryProvider unpack reporting + parallelize more * Fixes the incorrect tracking bug introduced here: https://github.com/microsoft/vcpkg-tool/pull/1715/files#r2414987620 * Gets clean_prepare_dir and the last_write_time fixup into the parallelized region Related to https://github.com/microsoft/vcpkg/pull/43309 * Plumbs error handling through to one place * Deletes decompress_in_parallel / cmd_execute_and_capture_output_parallel --- include/vcpkg/archives.h | 2 - include/vcpkg/base/files.h | 5 +- include/vcpkg/base/message-data.inc.h | 4 +- include/vcpkg/base/parallel-algorithms.h | 6 - include/vcpkg/base/system.process.h | 4 - locales/messages.json | 6 +- src/vcpkg-test/system.cpp | 28 ----- src/vcpkg/archives.cpp | 15 --- src/vcpkg/base/files.cpp | 56 +++++++--- src/vcpkg/base/system.process.cpp | 18 --- src/vcpkg/binarycaching.cpp | 134 +++++++++++++++-------- 11 files changed, 138 insertions(+), 140 deletions(-) diff --git a/include/vcpkg/archives.h b/include/vcpkg/archives.h index ce1d05feaa..52b51f5a19 100644 --- a/include/vcpkg/archives.h +++ b/include/vcpkg/archives.h @@ -66,6 +66,4 @@ namespace vcpkg Optional seven_zip; #endif }; - - std::vector> decompress_in_parallel(View jobs); } diff --git a/include/vcpkg/base/files.h b/include/vcpkg/base/files.h index 423435fca6..2c4ee81e3f 100644 --- a/include/vcpkg/base/files.h +++ b/include/vcpkg/base/files.h @@ -153,7 +153,7 @@ namespace vcpkg virtual std::vector get_files_recursive(const Path& dir, std::error_code& ec) const = 0; std::vector get_files_recursive(const Path& dir, LineInfo li) const; - ExpectedL> try_get_files_recursive(const Path& dir) const; + Optional> try_get_files_recursive(DiagnosticContext& context, const Path& dir) const; virtual std::vector get_files_recursive_lexically_proximate(const Path& dir, std::error_code& ec) const = 0; @@ -325,8 +325,7 @@ namespace vcpkg virtual int64_t last_write_time(const Path& target, std::error_code& ec) const = 0; int64_t last_write_time(const Path& target, LineInfo li) const noexcept; - virtual void last_write_time(const Path& target, int64_t new_time, std::error_code& ec) const = 0; - void last_write_time(const Path& target, int64_t new_time, LineInfo li) const noexcept; + virtual bool last_write_time(DiagnosticContext& context, const Path& target, int64_t new_time) const = 0; using ReadOnlyFilesystem::current_path; virtual void current_path(const Path& new_current_path, std::error_code&) const = 0; diff --git a/include/vcpkg/base/message-data.inc.h b/include/vcpkg/base/message-data.inc.h index a662065088..ea3edfa4a4 100644 --- a/include/vcpkg/base/message-data.inc.h +++ b/include/vcpkg/base/message-data.inc.h @@ -1268,6 +1268,7 @@ DECLARE_MESSAGE(ExportedZipArchive, (msg::path), "", "Zip archive exported at: { DECLARE_MESSAGE(ExportingAlreadyBuiltPackages, (), "", "The following packages are already built and will be exported:") DECLARE_MESSAGE(ExportingPackage, (msg::package_name), "", "Exporting {package_name}...") DECLARE_MESSAGE(ExtendedDocumentationAtUrl, (msg::url), "", "Extended documentation available at '{url}'.") +DECLARE_MESSAGE(ExtractedInto, (msg::path), "", "extracted into {path}") DECLARE_MESSAGE(ExtractHelp, (), "", "Extracts an archive.") DECLARE_MESSAGE(ExtractingTool, (msg::tool_name), "", "Extracting {tool_name}...") DECLARE_MESSAGE(FailedPostBuildChecks, @@ -2849,7 +2850,6 @@ DECLARE_MESSAGE(TwoFeatureFlagsSpecified, (msg::value), "'{value}' is a feature flag.", "Both '{value}' and -'{value}' were specified as feature flags.") -DECLARE_MESSAGE(UnableToClearPath, (msg::path), "", "unable to delete {path}") DECLARE_MESSAGE(UnableToReadAppDatas, (), "", "both %LOCALAPPDATA% and %APPDATA% were unreadable") DECLARE_MESSAGE(UnableToReadEnvironmentVariable, (msg::env_var), "", "unable to read {env_var}") DECLARE_MESSAGE(UndeterminedToolChainForTriplet, @@ -3348,11 +3348,13 @@ DECLARE_MESSAGE(WaitUntilPackagesUploaded, "", "Waiting for {count} remaining binary cache submissions...") DECLARE_MESSAGE(WarningsTreatedAsErrors, (), "", "previous warnings being interpreted as errors") +DECLARE_MESSAGE(WhileClearingThis, (), "", "while clearing this directory") DECLARE_MESSAGE(WhileCheckingOutBaseline, (msg::commit_sha), "", "while checking out baseline {commit_sha}") DECLARE_MESSAGE(WhileCheckingOutPortTreeIsh, (msg::package_name, msg::git_tree_sha), "", "while checking out port {package_name} with git tree {git_tree_sha}") +DECLARE_MESSAGE(WhileExtractingThisArchive, (), "", "while extracting this archive") DECLARE_MESSAGE(WhileGettingLocalTreeIshObjectsForPorts, (), "", "while getting local treeish objects for ports") DECLARE_MESSAGE(WhileLookingForSpec, (msg::spec), "", "while looking for {spec}:") DECLARE_MESSAGE(WhileLoadingBaselineVersionForPort, diff --git a/include/vcpkg/base/parallel-algorithms.h b/include/vcpkg/base/parallel-algorithms.h index 406683e278..a253760e29 100644 --- a/include/vcpkg/base/parallel-algorithms.h +++ b/include/vcpkg/base/parallel-algorithms.h @@ -161,10 +161,4 @@ namespace vcpkg { execute_in_parallel(c.size(), [&](size_t offset) { cb(c[offset]); }); } - - template - void parallel_transform(const Container& c, RanItTarget out_begin, F cb) noexcept - { - execute_in_parallel(c.size(), [&](size_t offset) { out_begin[offset] = cb(c[offset]); }); - } } diff --git a/include/vcpkg/base/system.process.h b/include/vcpkg/base/system.process.h index 7ab3ce41b3..14db566fd8 100644 --- a/include/vcpkg/base/system.process.h +++ b/include/vcpkg/base/system.process.h @@ -166,10 +166,6 @@ namespace vcpkg settings); } - std::vector> cmd_execute_and_capture_output_parallel(View commands); - std::vector> cmd_execute_and_capture_output_parallel( - View commands, const RedirectedProcessLaunchSettings& settings); - Optional cmd_execute_and_stream_lines(DiagnosticContext& context, const Command& cmd, const std::function& per_line_cb); diff --git a/locales/messages.json b/locales/messages.json index faf086129f..6e13cd7781 100644 --- a/locales/messages.json +++ b/locales/messages.json @@ -761,6 +761,8 @@ "ExtendedDocumentationAtUrl": "Extended documentation available at '{url}'.", "_ExtendedDocumentationAtUrl.comment": "An example of {url} is https://github.com/microsoft/vcpkg.", "ExtractHelp": "Extracts an archive.", + "ExtractedInto": "extracted into {path}", + "_ExtractedInto.comment": "An example of {path} is /foo/bar.", "ExtractingTool": "Extracting {tool_name}...", "_ExtractingTool.comment": "An example of {tool_name} is signtool.", "FailedPostBuildChecks": "Found {count} post-build check problem(s). These are usually caused by bugs in portfile.cmake or the upstream build system. Please correct these before submitting this port to the curated registry.", @@ -1499,8 +1501,6 @@ "TripletLabel": "Triplet:", "TwoFeatureFlagsSpecified": "Both '{value}' and -'{value}' were specified as feature flags.", "_TwoFeatureFlagsSpecified.comment": "'{value}' is a feature flag.", - "UnableToClearPath": "unable to delete {path}", - "_UnableToClearPath.comment": "An example of {path} is /foo/bar.", "UnableToReadAppDatas": "both %LOCALAPPDATA% and %APPDATA% were unreadable", "UnableToReadEnvironmentVariable": "unable to read {env_var}", "_UnableToReadEnvironmentVariable.comment": "An example of {env_var} is VCPKG_DEFAULT_TRIPLET.", @@ -1755,6 +1755,8 @@ "_WhileCheckingOutBaseline.comment": "An example of {commit_sha} is 7cfad47ae9f68b183983090afd6337cd60fd4949.", "WhileCheckingOutPortTreeIsh": "while checking out port {package_name} with git tree {git_tree_sha}", "_WhileCheckingOutPortTreeIsh.comment": "An example of {package_name} is zlib. An example of {git_tree_sha} is 7cfad47ae9f68b183983090afd6337cd60fd4949.", + "WhileClearingThis": "while clearing this directory", + "WhileExtractingThisArchive": "while extracting this archive", "WhileGettingLocalTreeIshObjectsForPorts": "while getting local treeish objects for ports", "WhileLoadingBaselineVersionForPort": "while loading baseline version for {package_name}", "_WhileLoadingBaselineVersionForPort.comment": "An example of {package_name} is zlib.", diff --git a/src/vcpkg-test/system.cpp b/src/vcpkg-test/system.cpp index 9e50027d0c..1f5f3f3344 100644 --- a/src/vcpkg-test/system.cpp +++ b/src/vcpkg-test/system.cpp @@ -134,34 +134,6 @@ TEST_CASE ("cmdlinebuilder", "[system]") #endif } -TEST_CASE ("cmd_execute_and_capture_output_parallel", "[system]") -{ - std::vector vec; - for (size_t i = 0; i < 50; ++i) - { -#if defined(_WIN32) - vec.push_back(Command("cmd.exe").string_arg("/d").string_arg("/c").string_arg(fmt::format("echo {}", i))); -#else - vec.push_back(Command("echo").string_arg(std::string(i, 'a'))); -#endif - } - - auto res = cmd_execute_and_capture_output_parallel(vec); - - for (size_t i = 0; i != res.size(); ++i) - { - auto out = res[i].get(); - REQUIRE(out != nullptr); - REQUIRE(out->exit_code == 0); - -#if defined(_WIN32) - REQUIRE(out->output == (fmt::format("{}\r\n", i))); -#else - REQUIRE(out->output == (std::string(i, 'a') + "\n")); -#endif - } -} - TEST_CASE ("append_shell_escaped", "[system]") { Command cmd; diff --git a/src/vcpkg/archives.cpp b/src/vcpkg/archives.cpp index bdb409fbee..c50370be93 100644 --- a/src/vcpkg/archives.cpp +++ b/src/vcpkg/archives.cpp @@ -368,19 +368,4 @@ namespace vcpkg #endif return cmd; } - - std::vector> decompress_in_parallel(View jobs) - { - RedirectedProcessLaunchSettings settings; - settings.environment = get_clean_environment(); - auto results = cmd_execute_and_capture_output_parallel(jobs, settings); - std::vector> filtered_results; - filtered_results.reserve(jobs.size()); - for (std::size_t idx = 0; idx < jobs.size(); ++idx) - { - filtered_results.push_back(flatten(results[idx], jobs[idx].command_line())); - } - - return filtered_results; - } } diff --git a/src/vcpkg/base/files.cpp b/src/vcpkg/base/files.cpp index 75150d2cac..264c339793 100644 --- a/src/vcpkg/base/files.cpp +++ b/src/vcpkg/base/files.cpp @@ -1664,16 +1664,23 @@ namespace vcpkg std::vector ReadOnlyFilesystem::get_files_recursive(const Path& dir, LineInfo li) const { - return this->try_get_files_recursive(dir).value_or_exit(li); + return this->try_get_files_recursive(console_diagnostic_context, dir).value_or_exit(li); } - ExpectedL> ReadOnlyFilesystem::try_get_files_recursive(const Path& dir) const + Optional> ReadOnlyFilesystem::try_get_files_recursive(DiagnosticContext& context, + const Path& dir) const { std::error_code ec; auto maybe_files = this->get_files_recursive(dir, ec); if (ec) { - return format_filesystem_call_error(ec, __func__, {dir}); + context.report(DiagnosticLine{DiagKind::Error, + dir, + msg::format(msgSystemApiErrorMessage, + msg::system_api = "get_files_recursive", + msg::exit_code = ec.value(), + msg::error_msg = ec.message())}); + return nullopt; } return maybe_files; @@ -2279,16 +2286,6 @@ namespace vcpkg return result; } - void Filesystem::last_write_time(const Path& target, int64_t new_time, vcpkg::LineInfo li) const noexcept - { - std::error_code ec; - this->last_write_time(target, new_time, ec); - if (ec) - { - exit_filesystem_call_error(li, ec, __func__, {target}); - } - } - void Filesystem::write_lines(const Path& file_path, const std::vector& lines, LineInfo li) const { std::error_code ec; @@ -3926,20 +3923,38 @@ namespace vcpkg #endif // ^^^ !_WIN32 } - void last_write_time(const Path& target, int64_t new_time, std::error_code& ec) const override + virtual bool last_write_time(DiagnosticContext& context, const Path& target, int64_t new_time) const override { + std::error_code ec; #if defined(_WIN32) stdfs::last_write_time(to_stdfs_path(target), stdfs::file_time_type::time_point{stdfs::file_time_type::time_point::duration { new_time }}, ec); + if (ec) + { + context.report(DiagnosticLine{DiagKind::Error, + target, + msg::format(msgSystemApiErrorMessage, + msg::system_api = "std::fs::last_write_time", + msg::exit_code = ec.value(), + msg::error_msg = ec.message())}); + return false; + } + return true; #else // ^^^ _WIN32 // !_WIN32 vvv PosixFd fd(target.c_str(), O_WRONLY, ec); if (ec) { - return; + context.report(DiagnosticLine{DiagKind::Error, + target, + msg::format(msgSystemApiErrorMessage, + msg::system_api = "open", + msg::exit_code = ec.value(), + msg::error_msg = ec.message())}); + return false; } timespec times[2]; // last access and modification time times[0].tv_nsec = UTIME_OMIT; @@ -3947,8 +3962,17 @@ namespace vcpkg times[1].tv_sec = new_time / 1'000'000'000; if (futimens(fd.get(), times)) { - ec.assign(errno, std::system_category()); + auto local_errno = errno; + context + .report(DiagnosticLine{DiagKind::Error, + target, + msg::format(msgSystemApiErrorMessage, + msg::system_api = "futimens", + msg::exit_code = local_errno, + msg::error_msg = std::system_category().message(local_errno)}); } + + return true; #endif // ^^^ !_WIN32 } diff --git a/src/vcpkg/base/system.process.cpp b/src/vcpkg/base/system.process.cpp index 117f7a95a3..98c9e4e455 100644 --- a/src/vcpkg/base/system.process.cpp +++ b/src/vcpkg/base/system.process.cpp @@ -4,7 +4,6 @@ #include #include #include -#include #include #include #include @@ -746,23 +745,6 @@ namespace vcpkg static const Environment clean_env = get_modified_clean_environment({}); return clean_env; } - - std::vector> cmd_execute_and_capture_output_parallel(View commands) - { - RedirectedProcessLaunchSettings default_redirected_process_launch_settings; - return cmd_execute_and_capture_output_parallel(commands, default_redirected_process_launch_settings); - } - - std::vector> cmd_execute_and_capture_output_parallel( - View commands, const RedirectedProcessLaunchSettings& settings) - { - std::vector> res(commands.size(), LocalizedString{}); - - parallel_transform( - commands, res.begin(), [&](const Command& cmd) { return cmd_execute_and_capture_output(cmd, settings); }); - - return res; - } } // namespace vcpkg namespace diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 44cfbb01e0..5c5d8595f6 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -199,13 +200,39 @@ namespace return make_feedref(info.spec, info.version, info.package_abi, prefix); } - void clean_prepare_dir(const Filesystem& fs, const Path& dir) + bool clean_prepare_dir(DiagnosticContext& context, const Filesystem& fs, const Path& dir) { - fs.remove_all(dir, VCPKG_LINE_INFO); - if (!fs.create_directories(dir, VCPKG_LINE_INFO)) + if (fs.remove_all(context, dir) && fs.create_directories(context, dir)) { - Checks::msg_exit_with_error(VCPKG_LINE_INFO, msgUnableToClearPath, msg::path = dir); + return true; } + + context.report(DiagnosticLine{DiagKind::Note, dir, msg::format(msgWhileClearingThis)}); + return false; + } + + bool directory_last_write_time(DiagnosticContext& context, const Filesystem& fs, const Path& dir) + { + auto now = fs.file_time_now(); + auto maybe_paths = fs.try_get_files_recursive(context, dir); + if (auto paths = maybe_paths.get()) + { + bool all_success = true; + for (auto&& path : *paths) + { + if (!fs.last_write_time(context, path, now)) + { + all_success = false; + } + } + + if (all_success) + { + return true; + } + } + + return false; } Path make_temp_archive_path(const Path& buildtrees, const PackageSpec& spec, const std::string& abi) @@ -298,67 +325,84 @@ namespace { ZipReadBinaryProvider(ZipTool zip, const Filesystem& fs) : m_zip(std::move(zip)), m_fs(fs) { } + struct UnzipJob + { + const Path* package_dir; + const ZipResource* zip_resource; + uint64_t zip_size; + size_t action_idx; + FullyBufferedDiagnosticContext fbdc; + bool success = false; + }; + void fetch(View actions, Span out_status) const override { const ElapsedTimer timer; std::vector> zip_paths(actions.size(), nullopt); acquire_zips(actions, zip_paths); - std::vector> jobs_with_size; - std::vector action_idxs; + std::vector jobs; + jobs.reserve(actions.size()); for (size_t i = 0; i < actions.size(); ++i) { - if (!zip_paths[i]) continue; - const auto& pkg_path = actions[i]->package_dir.value_or_exit(VCPKG_LINE_INFO); - clean_prepare_dir(m_fs, pkg_path); - jobs_with_size.emplace_back(m_zip.decompress_zip_archive_cmd(pkg_path, zip_paths[i].get()->path), - m_fs.file_size(zip_paths[i].get()->path, VCPKG_LINE_INFO)); - action_idxs.push_back(i); + if (auto zip_resource = zip_paths[i].get()) + { + jobs.push_back({&actions[i]->package_dir.value_or_exit(VCPKG_LINE_INFO), + zip_resource, + m_fs.file_size(zip_resource->path, IgnoreErrors{}), + i}); + } } - std::sort(jobs_with_size.begin(), jobs_with_size.end(), [](const auto& l, const auto& r) { - return l.second > r.second; - }); - std::vector sorted_jobs; - for (auto&& e : jobs_with_size) - { - sorted_jobs.push_back(std::move(e.first)); - } - auto job_results = decompress_in_parallel(sorted_jobs); + std::sort( + jobs.begin(), jobs.end(), [](const UnzipJob& l, const UnzipJob& r) { return l.zip_size > r.zip_size; }); - for (size_t j = 0; j < sorted_jobs.size(); ++j) - { - const auto i = action_idxs[j]; - const auto& zip_path = zip_paths[i].value_or_exit(VCPKG_LINE_INFO); - if (job_results[j]) + parallel_for_each(jobs, [this, &out_status](UnzipJob& job) { + WarningDiagnosticContext wdc{job.fbdc}; + if (clean_prepare_dir(wdc, m_fs, *job.package_dir)) { + auto cmd = m_zip.decompress_zip_archive_cmd(*job.package_dir, job.zip_resource->path); + auto maybe_output = cmd_execute_and_capture_output(wdc, cmd); + if (check_zero_exit_code(wdc, cmd, maybe_output) #ifdef _WIN32 - // On windows the ziptool does restore file times, we don't want that because this breaks file time - // based change detection. - const auto& pkg_path = actions[i]->package_dir.value_or_exit(VCPKG_LINE_INFO); - auto now = m_fs.file_time_now(); - for (auto&& path : m_fs.get_files_recursive(pkg_path, VCPKG_LINE_INFO)) + // On windows the ziptool does restore file times, we don't want that because this breaks file + // time based change detection. + && directory_last_write_time(wdc, m_fs, *job.package_dir) +#endif // ^^^ _WIN32 + ) { - m_fs.last_write_time(path, now, VCPKG_LINE_INFO); + out_status[job.action_idx] = RestoreResult::restored; + job.success = true; + } + else + { + wdc.report(DiagnosticLine{ + DiagKind::Note, job.zip_resource->path, msg::format(msgWhileExtractingThisArchive)}); } -#endif - Debug::print("Restored ", zip_path.path, '\n'); - out_status[i] = RestoreResult::restored; } - else + + if (job.zip_resource->to_remove == RemoveWhen::always) { - Debug::print("Failed to decompress archive package: ", zip_path.path, '\n'); + m_fs.remove(job.zip_resource->path, IgnoreErrors{}); } + }); - post_decompress(zip_path); - } - } - - void post_decompress(const ZipResource& r) const - { - if (r.to_remove == RemoveWhen::always) + if (Debug::g_debugging) { - m_fs.remove(r.path, IgnoreErrors{}); + for (auto&& job : jobs) + { + if (job.success) + { + console_diagnostic_context.report( + DiagnosticLine{DiagKind::Note, + job.zip_resource->path, + msg::format(msgExtractedInto, msg::path = *job.package_dir)}); + } + else + { + job.fbdc.print_to(out_sink); + } + } } } From cb2748832b0ba6e50813909d639e060e768681cb Mon Sep 17 00:00:00 2001 From: Billy Robert O'Neal III Date: Wed, 8 Oct 2025 16:39:16 -0700 Subject: [PATCH 2/3] Avoid -Wunused-function --- src/vcpkg/binarycaching.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 5c5d8595f6..429c778332 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -211,6 +211,7 @@ namespace return false; } +#ifdef _WIN32 bool directory_last_write_time(DiagnosticContext& context, const Filesystem& fs, const Path& dir) { auto now = fs.file_time_now(); @@ -234,6 +235,7 @@ namespace return false; } +#endif // ^^^ _WIN32 Path make_temp_archive_path(const Path& buildtrees, const PackageSpec& spec, const std::string& abi) { From e00461db0b9d29f9ce9635b10c8beb144b8d8b1b Mon Sep 17 00:00:00 2001 From: Billy Robert O'Neal III Date: Wed, 8 Oct 2025 16:46:58 -0700 Subject: [PATCH 3/3] Correct missing ) --- src/vcpkg/base/files.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/vcpkg/base/files.cpp b/src/vcpkg/base/files.cpp index 264c339793..0e339b41dd 100644 --- a/src/vcpkg/base/files.cpp +++ b/src/vcpkg/base/files.cpp @@ -3963,13 +3963,13 @@ namespace vcpkg if (futimens(fd.get(), times)) { auto local_errno = errno; - context - .report(DiagnosticLine{DiagKind::Error, - target, - msg::format(msgSystemApiErrorMessage, - msg::system_api = "futimens", - msg::exit_code = local_errno, - msg::error_msg = std::system_category().message(local_errno)}); + context.report( + DiagnosticLine{DiagKind::Error, + target, + msg::format(msgSystemApiErrorMessage, + msg::system_api = "futimens", + msg::exit_code = local_errno, + msg::error_msg = std::system_category().message(local_errno))}); } return true;