From 95f0438c7a37a05189918d814a10f8440b7f36f1 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 15 Feb 2023 14:51:21 +0100 Subject: [PATCH 01/40] Binary cache: async push_success --- include/vcpkg/binarycaching.h | 47 +++++- include/vcpkg/binarycaching.private.h | 7 +- include/vcpkg/build.h | 1 - include/vcpkg/fwd/binarycaching.h | 1 + src/vcpkg-test/binarycaching.cpp | 20 ++- src/vcpkg.cpp | 2 + src/vcpkg/binarycaching.cpp | 232 +++++++++++++++++--------- src/vcpkg/build.cpp | 9 +- src/vcpkg/install.cpp | 9 +- 9 files changed, 227 insertions(+), 101 deletions(-) diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index d2843f4c8d..acbbac9c0f 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -9,10 +9,14 @@ #include #include +#include +#include #include +#include #include #include +#include #include #include @@ -42,6 +46,21 @@ namespace vcpkg const IBinaryProvider* m_available_provider = nullptr; // meaningful iff m_status == available }; + struct BinaryPackageInformation + { + explicit BinaryPackageInformation(const InstallPlanAction& action); + std::string package_abi; + // The following fields are only needed for the nuget binary cache + PackageSpec spec; + SourceControlFile& source_control_file; + std::string& raw_version; + std::string compiler_id; + std::string compiler_version; + std::string triplet_abi; + InternalFeatureSet feature_list; + std::vector package_dependencies; + }; + struct IBinaryProvider { virtual ~IBinaryProvider() = default; @@ -52,7 +71,9 @@ namespace vcpkg /// Called upon a successful build of `action` to store those contents in the binary cache. /// Prerequisite: action has a package_abi() - virtual void push_success(const InstallPlanAction& action) const = 0; + virtual void push_success(const BinaryPackageInformation& info, + const Path& packages_dir, + MessageSink& msg_sink) = 0; /// Gives the IBinaryProvider an opportunity to batch any downloading or server communication for /// executing `actions`. @@ -77,7 +98,7 @@ namespace vcpkg std::vector headers_for_get; LocalizedString valid() const; - std::string instantiate_variables(const InstallPlanAction& action) const; + std::string instantiate_variables(const BinaryPackageInformation& info) const; }; struct BinaryConfigParserState @@ -121,9 +142,13 @@ namespace vcpkg struct BinaryCache { - BinaryCache() = default; + static BinaryCache* current_instance; + static void wait_for_async_complete(); + BinaryCache(Filesystem& filesystem); explicit BinaryCache(const VcpkgCmdArguments& args, const VcpkgPaths& paths); + ~BinaryCache(); + void install_providers(std::vector>&& providers); void install_providers_for(const VcpkgCmdArguments& args, const VcpkgPaths& paths); @@ -131,7 +156,7 @@ namespace vcpkg RestoreResult try_restore(const InstallPlanAction& action); /// Called upon a successful build of `action` to store those contents in the binary cache. - void push_success(const InstallPlanAction& action); + void push_success(const InstallPlanAction& action, Path package_dir); /// Gives the IBinaryProvider an opportunity to batch any downloading or server communication for /// executing `actions`. @@ -143,8 +168,22 @@ namespace vcpkg std::vector precheck(View actions); private: + struct ActionToPush + { + BinaryPackageInformation info; + bool clean_after_push; + Path packages_dir; + }; + void push_thread_main(); + std::unordered_map m_status; std::vector> m_providers; + std::condition_variable actions_to_push_notifier; + std::mutex actions_to_push_mutex; + std::queue actions_to_push; + std::thread push_thread; + std::atomic_bool end_push_thread; + Filesystem& filesystem; }; ExpectedS parse_download_configuration(const Optional& arg); diff --git a/include/vcpkg/binarycaching.private.h b/include/vcpkg/binarycaching.private.h index 886539fa9e..a0369b31b8 100644 --- a/include/vcpkg/binarycaching.private.h +++ b/include/vcpkg/binarycaching.private.h @@ -5,6 +5,7 @@ #include +#include #include namespace vcpkg @@ -35,6 +36,10 @@ namespace vcpkg { return {Strings::concat(prefix, spec.dir()), format_version_for_nugetref(raw_version, abi_tag)}; } + inline NugetReference make_nugetref(const BinaryPackageInformation& info, const std::string& prefix) + { + return make_nugetref(info.spec, info.raw_version, info.package_abi, prefix); + } inline NugetReference make_nugetref(const InstallPlanAction& action, const std::string& prefix) { return make_nugetref(action.spec, @@ -57,7 +62,7 @@ namespace vcpkg } std::string generate_nuspec(const Path& package_dir, - const InstallPlanAction& action, + const BinaryPackageInformation& info, const NugetReference& ref, details::NuGetRepoInfo rinfo = details::get_nuget_repo_info_from_env()); } diff --git a/include/vcpkg/build.h b/include/vcpkg/build.h index b85bcfd463..9753257cba 100644 --- a/include/vcpkg/build.h +++ b/include/vcpkg/build.h @@ -205,7 +205,6 @@ namespace vcpkg ExtendedBuildResult build_package(const VcpkgCmdArguments& args, const VcpkgPaths& paths, const InstallPlanAction& config, - BinaryCache& binary_cache, const IBuildLogsRecorder& build_logs_recorder, const StatusParagraphs& status_db); diff --git a/include/vcpkg/fwd/binarycaching.h b/include/vcpkg/fwd/binarycaching.h index de46295d3f..66665ceaaf 100644 --- a/include/vcpkg/fwd/binarycaching.h +++ b/include/vcpkg/fwd/binarycaching.h @@ -25,4 +25,5 @@ namespace vcpkg struct IBinaryProvider; struct BinaryCache; struct BinaryConfigParserState; + struct BinaryPackageInformation; } diff --git a/src/vcpkg-test/binarycaching.cpp b/src/vcpkg-test/binarycaching.cpp index edd9577dcf..c84840124b 100644 --- a/src/vcpkg-test/binarycaching.cpp +++ b/src/vcpkg-test/binarycaching.cpp @@ -24,9 +24,12 @@ struct KnowNothingBinaryProvider : IBinaryProvider return RestoreResult::unavailable; } - virtual void push_success(const InstallPlanAction& action) const override { CHECK(action.has_package_abi()); } + void push_success(const BinaryPackageInformation& info, const Path&, MessageSink&) override + { + CHECK_FALSE(info.package_abi.empty()); + } - virtual void prefetch(View actions, View cache_status) const override + void prefetch(View actions, View cache_status) const override { REQUIRE(actions.size() == cache_status.size()); for (size_t idx = 0; idx < cache_status.size(); ++idx) @@ -34,7 +37,7 @@ struct KnowNothingBinaryProvider : IBinaryProvider CHECK(actions[idx].has_package_abi() == (cache_status[idx] != nullptr)); } } - virtual void precheck(View actions, View cache_status) const override + void precheck(View actions, View cache_status) const override { REQUIRE(actions.size() == cache_status.size()); for (const auto c : cache_status) @@ -277,7 +280,7 @@ Build-Depends: bzip REQUIRE(ref.nupkg_filename() == "zlib2_x64-windows.1.5.0-vcpkgpackageabi.nupkg"); { - auto nuspec = generate_nuspec(pkgPath, ipa, ref, {}); + auto nuspec = generate_nuspec(pkgPath, BinaryPackageInformation{ipa}, ref, {}); std::string expected = R"( zlib2_x64-windows @@ -305,7 +308,7 @@ Features: a, b } { - auto nuspec = generate_nuspec(pkgPath, ipa, ref, {"urlvalue"}); + auto nuspec = generate_nuspec(pkgPath, BinaryPackageInformation{ipa}, ref, {"urlvalue"}); std::string expected = R"( zlib2_x64-windows @@ -333,7 +336,8 @@ Features: a, b REQUIRE_EQUAL_TEXT(nuspec, expected); } { - auto nuspec = generate_nuspec(pkgPath, ipa, ref, {"urlvalue", "branchvalue", "commitvalue"}); + auto nuspec = + generate_nuspec(pkgPath, BinaryPackageInformation{ipa}, ref, {"urlvalue", "branchvalue", "commitvalue"}); std::string expected = R"( zlib2_x64-windows @@ -365,7 +369,7 @@ Features: a, b TEST_CASE ("Provider nullptr checks", "[BinaryCache]") { // create a binary cache to test - BinaryCache uut; + BinaryCache uut(get_real_filesystem()); std::vector> providers; providers.emplace_back(std::make_unique()); uut.install_providers(std::move(providers)); @@ -391,7 +395,7 @@ Version: 1.5 InstallPlanAction& ipa_without_abi = install_plan.back(); // test that the binary cache does the right thing. See also CHECKs etc. in KnowNothingBinaryProvider - uut.push_success(ipa_without_abi); // should have no effects + uut.push_success(ipa_without_abi, {}); // should have no effects CHECK(uut.try_restore(ipa_without_abi) == RestoreResult::unavailable); uut.prefetch(install_plan); // should have no effects } diff --git a/src/vcpkg.cpp b/src/vcpkg.cpp index 39724dcc23..236f88ee6e 100644 --- a/src/vcpkg.cpp +++ b/src/vcpkg.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -154,6 +155,7 @@ namespace vcpkg::Checks // Implements link seam from basic_checks.h void on_final_cleanup_and_exit() { + BinaryCache::wait_for_async_complete(); const auto elapsed_us_inner = g_total_time.microseconds(); bool debugging = Debug::g_debugging; diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 71c5f347c3..aab4413167 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -297,45 +297,41 @@ namespace return RestoreResult::unavailable; } - void push_success(const InstallPlanAction& action) const override + void push_success(const BinaryPackageInformation& info, + const Path& packages_dir, + MessageSink& msg_sink) override { if (m_write_dirs.empty() && m_put_url_templates.empty()) { return; } - const auto& abi_tag = action.package_abi().value_or_exit(VCPKG_LINE_INFO); - auto& spec = action.spec; + const auto& abi_tag = info.package_abi; + auto& spec = info.spec; auto& fs = paths.get_filesystem(); const auto archive_subpath = make_archive_subpath(abi_tag); const auto tmp_archive_path = make_temp_archive_path(paths.buildtrees(), spec); - auto compress_result = compress_directory_to_zip( - fs, paths.get_tool_cache(), stdout_sink, paths.package_dir(spec), tmp_archive_path); + auto compress_result = + compress_directory_to_zip(fs, paths.get_tool_cache(), stdout_sink, packages_dir, tmp_archive_path); if (!compress_result) { - msg::println(Color::warning, - msg::format_warning(msgCompressFolderFailed, msg::path = paths.package_dir(spec)) - .append_raw(' ') - .append_raw(compress_result.error())); + msg_sink.println(Color::warning, + msg::format_warning(msgCompressFolderFailed, msg::path = packages_dir) + .append_raw(' ') + .append_raw(compress_result.error())); return; } - size_t http_remotes_pushed = 0; for (auto&& put_url_template : m_put_url_templates) { - auto url = put_url_template.instantiate_variables(action); + auto url = put_url_template.instantiate_variables(info); auto maybe_success = put_file(fs, url, m_secrets, put_url_template.headers_for_put, tmp_archive_path); if (maybe_success) { - http_remotes_pushed++; + m_http_remotes_pushed++; continue; } - msg::println(Color::warning, maybe_success.error()); - } - - if (!m_put_url_templates.empty()) - { - msg::println(msgUploadedBinaries, msg::count = http_remotes_pushed, msg::vendor = "HTTP remotes"); + msg_sink.println(Color::warning, maybe_success.error()); } for (const auto& archives_root_dir : m_write_dirs) @@ -354,14 +350,10 @@ namespace if (ec) { - msg::println(Color::warning, - msg::format(msgFailedToStoreBinaryCache, msg::path = archive_path) - .append_raw('\n') - .append_raw(ec.message())); - } - else - { - msg::println(msgStoredBinaryCache, msg::path = archive_path); + msg_sink.println(Color::warning, + msg::format(msgFailedToStoreBinaryCache, msg::path = archive_path) + .append_raw('\n') + .append_raw(ec.message())); } } // In the case of 1 write dir, the file will be moved instead of copied @@ -369,8 +361,20 @@ namespace { fs.remove(tmp_archive_path, IgnoreErrors{}); } + if (!m_put_url_templates.empty()) + { + msg_sink.println(msgUploadedBinaries, msg::count = m_http_remotes_pushed, msg::vendor = "HTTP remotes"); + } } + /*void print_upload_statistics(MessageSink& msg_sink) override + { + if (!m_put_url_templates.empty()) + { + msg_sink.println(msgUploadedBinaries, msg::count = m_http_remotes_pushed, msg::vendor = "HTTP remotes"); + } + };*/ + void precheck(View actions, View cache_status) const override { auto& fs = paths.get_filesystem(); @@ -411,6 +415,7 @@ namespace std::vector m_write_dirs; std::vector m_put_url_templates; std::vector m_secrets; + size_t m_http_remotes_pushed = 0; }; struct HttpGetBinaryProvider : IBinaryProvider { @@ -423,7 +428,7 @@ namespace RestoreResult try_restore(const InstallPlanAction&) const override { return RestoreResult::unavailable; } - void push_success(const InstallPlanAction&) const override { } + void push_success(const BinaryPackageInformation&, const Path&, MessageSink&) override { } void prefetch(View actions, View cache_status) const override { @@ -446,7 +451,7 @@ namespace auto&& action = actions[idx]; clean_prepare_dir(fs, paths.package_dir(action.spec)); - auto uri = url_template.instantiate_variables(action); + auto uri = url_template.instantiate_variables(BinaryPackageInformation{action}); url_paths.emplace_back(std::move(uri), make_temp_archive_path(paths.buildtrees(), action.spec)); url_indices.push_back(idx); } @@ -510,7 +515,7 @@ namespace continue; } - urls.push_back(url_template.instantiate_variables(actions[idx])); + urls.push_back(url_template.instantiate_variables(BinaryPackageInformation{actions[idx]})); url_indices.push_back(idx); } @@ -808,20 +813,21 @@ namespace RestoreResult try_restore(const InstallPlanAction&) const override { return RestoreResult::unavailable; } - void push_success(const InstallPlanAction& action) const override + void push_success(const BinaryPackageInformation& info, + const Path& packages_dir, + MessageSink& msg_sink) override { if (m_write_sources.empty() && m_write_configs.empty()) { return; } - auto& spec = action.spec; + auto& spec = info.spec; - NugetReference nuget_ref = make_nugetref(action, get_nuget_prefix()); + NugetReference nuget_ref = make_nugetref(info, get_nuget_prefix()); auto nuspec_path = paths.buildtrees() / spec.name() / (spec.triplet().to_string() + ".nuspec"); auto& fs = paths.get_filesystem(); - fs.write_contents( - nuspec_path, generate_nuspec(paths.package_dir(spec), action, nuget_ref), VCPKG_LINE_INFO); + fs.write_contents(nuspec_path, generate_nuspec(packages_dir, info, nuget_ref), VCPKG_LINE_INFO); const auto& nuget_exe = paths.get_tool_exe("nuget", stdout_sink); Command cmdline; @@ -843,7 +849,7 @@ namespace if (!run_nuget_commandline(cmdline)) { - msg::println(Color::error, msgPackingVendorFailed, msg::vendor = "NuGet"); + msg_sink.println(Color::error, msgPackingVendorFailed, msg::vendor = "NuGet"); return; } @@ -867,11 +873,12 @@ namespace { cmd.string_arg("-NonInteractive"); } - msg::println( + msg_sink.println( msgUploadingBinariesToVendor, msg::spec = spec, msg::vendor = "NuGet", msg::path = write_src); if (!run_nuget_commandline(cmd)) { - msg::println(Color::error, msgPushingVendorFailed, msg::vendor = "NuGet", msg::path = write_src); + msg_sink.println( + Color::error, msgPushingVendorFailed, msg::vendor = "NuGet", msg::path = write_src); } } for (auto&& write_cfg : m_write_configs) @@ -892,14 +899,15 @@ namespace { cmd.string_arg("-NonInteractive"); } - msg::println(Color::error, - msgUploadingBinariesUsingVendor, - msg::spec = spec, - msg::vendor = "NuGet config", - msg::path = write_cfg); + msg_sink.println(Color::error, + msgUploadingBinariesUsingVendor, + msg::spec = spec, + msg::vendor = "NuGet config", + msg::path = write_cfg); if (!run_nuget_commandline(cmd)) { - msg::println(Color::error, msgPushingVendorFailed, msg::vendor = "NuGet", msg::path = write_cfg); + msg_sink.println( + Color::error, msgPushingVendorFailed, msg::vendor = "NuGet", msg::path = write_cfg); } } @@ -1007,21 +1015,23 @@ namespace RestoreResult try_restore(const InstallPlanAction&) const override { return RestoreResult::unavailable; } - void push_success(const InstallPlanAction& action) const override + void push_success(const BinaryPackageInformation& info, + const Path& packages_dir, + MessageSink& msg_sink) override { if (m_write_prefixes.empty()) return; const ElapsedTimer timer; - const auto& abi = action.package_abi().value_or_exit(VCPKG_LINE_INFO); - auto& spec = action.spec; + const auto& abi = info.package_abi; + auto& spec = info.spec; const auto tmp_archive_path = make_temp_archive_path(paths.buildtrees(), spec); auto compression_result = compress_directory_to_zip( - paths.get_filesystem(), paths.get_tool_cache(), stdout_sink, paths.package_dir(spec), tmp_archive_path); + paths.get_filesystem(), paths.get_tool_cache(), stdout_sink, packages_dir, tmp_archive_path); if (!compression_result) { - vcpkg::msg::println(Color::warning, - msg::format_warning(msgCompressFolderFailed, msg::path = paths.package_dir(spec)) - .append_raw(' ') - .append_raw(compression_result.error())); + msg_sink.println(Color::warning, + msg::format_warning(msgCompressFolderFailed, msg::path = packages_dir) + .append_raw(' ') + .append_raw(compression_result.error())); return; } @@ -1034,10 +1044,10 @@ namespace } } - msg::println(msgUploadedPackagesToVendor, - msg::count = upload_count, - msg::elapsed = timer.elapsed(), - msg::vendor = vendor()); + msg_sink.println(msgUploadedPackagesToVendor, + msg::count = upload_count, + msg::elapsed = timer.elapsed(), + msg::vendor = vendor()); } void precheck(View actions, View cache_status) const override @@ -1273,26 +1283,25 @@ namespace vcpkg return {}; } - std::string UrlTemplate::instantiate_variables(const InstallPlanAction& action) const + std::string UrlTemplate::instantiate_variables(const BinaryPackageInformation& info) const { return api_stable_format(url_template, [&](std::string& out, StringView key) { if (key == "version") { - out += action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO) - .source_control_file->core_paragraph->raw_version; + out += info.raw_version; } else if (key == "name") { - out += action.spec.name(); + out += info.spec.name(); } else if (key == "triplet") { - out += action.spec.triplet().canonical_name(); + out += info.spec.triplet().canonical_name(); } else if (key == "sha") { - out += action.abi_info.value_or_exit(VCPKG_LINE_INFO).package_abi; + out += info.package_abi; } else { @@ -1304,11 +1313,36 @@ namespace vcpkg .value_or_exit(VCPKG_LINE_INFO); } + BinaryCache* BinaryCache::current_instance; + + void BinaryCache::wait_for_async_complete() + { + if (current_instance) + { + msg::write_unlocalized_text_to_stdout(Color::none, "Wait for end\n"); + current_instance->end_push_thread = true; + current_instance->actions_to_push_notifier.notify_all(); + current_instance->push_thread.join(); + msg::write_unlocalized_text_to_stdout(Color::none, "Wait for end done \n"); + current_instance = nullptr; + } + } + + BinaryCache::BinaryCache(Filesystem& filesystem) + : push_thread([this]() { push_thread_main(); }), end_push_thread{false}, filesystem(filesystem) + { + Checks::check_exit(VCPKG_LINE_INFO, current_instance == nullptr); + current_instance = this; + } + BinaryCache::BinaryCache(const VcpkgCmdArguments& args, const VcpkgPaths& paths) + : BinaryCache(paths.get_filesystem()) { install_providers_for(args, paths); } + BinaryCache::~BinaryCache() { BinaryCache::wait_for_async_complete(); } + void BinaryCache::install_providers(std::vector>&& providers) { Checks::check_exit( @@ -1387,17 +1421,24 @@ namespace vcpkg return RestoreResult::unavailable; } - void BinaryCache::push_success(const InstallPlanAction& action) + void BinaryCache::push_success(const InstallPlanAction& action, Path package_dir) { const auto abi = action.package_abi().get(); if (abi) { - for (auto&& provider : m_providers) + const auto clean_packages = action.build_options.clean_packages == CleanPackages::YES; + if (clean_packages) { - provider->push_success(action); + static int counter = 0; + Path new_packaged_dir = package_dir + "_push_" + std::to_string(++counter); + filesystem.remove_all(new_packaged_dir, VCPKG_LINE_INFO); + filesystem.rename(package_dir, new_packaged_dir, VCPKG_LINE_INFO); + package_dir = new_packaged_dir; } - m_status[*abi].mark_restored(); + std::unique_lock lock(actions_to_push_mutex); + actions_to_push.push(ActionToPush{BinaryPackageInformation{action}, clean_packages, package_dir}); + actions_to_push_notifier.notify_all(); } } @@ -1455,6 +1496,34 @@ namespace vcpkg return results; } + void BinaryCache::push_thread_main() + { + while (true) + { + std::unique_lock lock(actions_to_push_mutex); + actions_to_push_notifier.wait(lock, [this]() { return !actions_to_push.empty() || end_push_thread; }); + if (actions_to_push.empty()) + { + if (end_push_thread) break; + continue; + } + + auto entry = std::move(actions_to_push.front()); + actions_to_push.pop(); + lock.unlock(); // we don't touch actions_to_push anymore + + for (auto&& provider : m_providers) + { + provider->push_success(entry.info, entry.packages_dir, stdout_sink); + } + if (entry.clean_after_push) + { + filesystem.remove_all(entry.packages_dir, VCPKG_LINE_INFO); + } + actions_to_push_notifier.notify_all(); + } + } + bool CacheStatus::should_attempt_precheck(const IBinaryProvider* sender) const noexcept { switch (m_status) @@ -1565,6 +1634,21 @@ namespace vcpkg configs_to_write.clear(); secrets.clear(); } + + BinaryPackageInformation::BinaryPackageInformation(const InstallPlanAction& action) + : package_abi(action.package_abi().value_or_exit(VCPKG_LINE_INFO)) + , spec(action.spec) + , source_control_file( + *action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO).source_control_file) + , raw_version(source_control_file.core_paragraph->raw_version) + , compiler_id(action.abi_info.value_or_exit(VCPKG_LINE_INFO).compiler_info.value_or_exit(VCPKG_LINE_INFO).id) + , compiler_version( + action.abi_info.value_or_exit(VCPKG_LINE_INFO).compiler_info.value_or_exit(VCPKG_LINE_INFO).version) + , triplet_abi(action.abi_info.value_or_exit(VCPKG_LINE_INFO).triplet_abi.value_or_exit(VCPKG_LINE_INFO)) + , feature_list(action.feature_list) + , package_dependencies(action.package_dependencies) + { + } } namespace @@ -2366,15 +2450,13 @@ details::NuGetRepoInfo details::get_nuget_repo_info_from_env() } std::string vcpkg::generate_nuspec(const Path& package_dir, - const InstallPlanAction& action, + const BinaryPackageInformation& info, const vcpkg::NugetReference& ref, details::NuGetRepoInfo rinfo) { - auto& spec = action.spec; - auto& scf = *action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO).source_control_file; + auto& spec = info.spec; + auto& scf = info.source_control_file; auto& version = scf.core_paragraph->raw_version; - const auto& abi_info = action.abi_info.value_or_exit(VCPKG_LINE_INFO); - const auto& compiler_info = abi_info.compiler_info.value_or_exit(VCPKG_LINE_INFO); std::string description = Strings::concat("NOT FOR DIRECT USE. Automatically generated cache package.\n\n", Strings::join("\n ", scf.core_paragraph->description), @@ -2383,16 +2465,16 @@ std::string vcpkg::generate_nuspec(const Path& package_dir, "\nTriplet: ", spec.triplet().to_string(), "\nCXX Compiler id: ", - compiler_info.id, + info.compiler_id, "\nCXX Compiler version: ", - compiler_info.version, + info.compiler_version, "\nTriplet/Compiler hash: ", - abi_info.triplet_abi.value_or_exit(VCPKG_LINE_INFO), + info.triplet_abi, "\nFeatures:", - Strings::join(",", action.feature_list, [](const std::string& s) { return " " + s; }), + Strings::join(",", info.feature_list, [](const std::string& s) { return " " + s; }), "\nDependencies:\n"); - for (auto&& dep : action.package_dependencies) + for (auto&& dep : info.package_dependencies) { Strings::append(description, " ", dep.name(), '\n'); } diff --git a/src/vcpkg/build.cpp b/src/vcpkg/build.cpp index 065f2b73a2..d4bfcaa23d 100644 --- a/src/vcpkg/build.cpp +++ b/src/vcpkg/build.cpp @@ -144,7 +144,7 @@ namespace vcpkg::Build action->build_options.clean_packages = CleanPackages::NO; const ElapsedTimer build_timer; - const auto result = build_package(args, paths, *action, binary_cache, build_logs_recorder, status_db); + const auto result = build_package(args, paths, *action, build_logs_recorder, status_db); msg::print(msgElapsedForPackage, msg::spec = spec, msg::elapsed = build_timer); if (result.code == BuildResult::CASCADED_DUE_TO_MISSING_DEPENDENCIES) { @@ -174,6 +174,7 @@ namespace vcpkg::Build msg::print(create_user_troubleshooting_message(*action, paths)); return 1; } + binary_cache.push_success(*action, paths.package_dir(action->spec)); return 0; } @@ -1274,7 +1275,6 @@ namespace vcpkg ExtendedBuildResult build_package(const VcpkgCmdArguments& args, const VcpkgPaths& paths, const InstallPlanAction& action, - BinaryCache& binary_cache, const IBuildLogsRecorder& build_logs_recorder, const StatusParagraphs& status_db) { @@ -1330,11 +1330,6 @@ namespace vcpkg filesystem.create_directories(abi_package_dir, VCPKG_LINE_INFO); filesystem.copy_file(abi_file, abi_file_in_package, CopyOptions::none, VCPKG_LINE_INFO); - if (result.code == BuildResult::SUCCEEDED) - { - binary_cache.push_success(action); - } - return result; } diff --git a/src/vcpkg/install.cpp b/src/vcpkg/install.cpp index 104d0fb40a..83f8b8a5c5 100644 --- a/src/vcpkg/install.cpp +++ b/src/vcpkg/install.cpp @@ -339,7 +339,7 @@ namespace vcpkg else msg::println(msgBuildingPackage, msg::spec = action.displayname()); - auto result = build_package(args, paths, action, binary_cache, build_logs_recorder, status_db); + auto result = build_package(args, paths, action, build_logs_recorder, status_db); if (BuildResult::DOWNLOADED == result.code) { @@ -377,10 +377,9 @@ namespace vcpkg case InstallResult::FILE_CONFLICTS: code = BuildResult::FILE_CONFLICTS; break; default: Checks::unreachable(VCPKG_LINE_INFO); } - - if (action.build_options.clean_packages == CleanPackages::YES) + if (restore != RestoreResult::restored) { - fs.remove_all(paths.package_dir(action.spec), VCPKG_LINE_INFO); + binary_cache.push_success(action, paths.package_dir(action.spec)); } if (action.build_options.clean_downloads == CleanDownloads::YES) @@ -1018,7 +1017,7 @@ namespace vcpkg } } - BinaryCache binary_cache; + BinaryCache binary_cache(paths.get_filesystem()); if (!only_downloads) { binary_cache.install_providers_for(args, paths); From 2a542056a6fe2cd13a097b4a44a4129c0bd91acd Mon Sep 17 00:00:00 2001 From: autoantwort <41973254+autoantwort@users.noreply.github.com> Date: Thu, 2 Mar 2023 21:45:49 +0100 Subject: [PATCH 02/40] Apply suggestions from code review Co-authored-by: Robert Schumacher --- include/vcpkg/binarycaching.h | 1 - src/vcpkg/binarycaching.cpp | 6 +++--- src/vcpkg/install.cpp | 3 +++ 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index 713b4a483f..471bc29a0c 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -172,7 +172,6 @@ namespace vcpkg { BinaryPackageInformation info; bool clean_after_push; - Path packages_dir; }; void push_thread_main(); diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index a8e08fd354..5515f43c3d 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -313,7 +313,7 @@ namespace const auto archive_subpath = make_archive_subpath(abi_tag); const auto tmp_archive_path = make_temp_archive_path(paths.buildtrees(), spec); auto compress_result = - compress_directory_to_zip(fs, paths.get_tool_cache(), stdout_sink, packages_dir, tmp_archive_path); + compress_directory_to_zip(fs, paths.get_tool_cache(), msg_sink, packages_dir, tmp_archive_path); if (!compress_result) { msg_sink.println(Color::warning, @@ -1018,7 +1018,7 @@ namespace auto& spec = info.spec; const auto tmp_archive_path = make_temp_archive_path(paths.buildtrees(), spec); auto compression_result = compress_directory_to_zip( - paths.get_filesystem(), paths.get_tool_cache(), stdout_sink, packages_dir, tmp_archive_path); + paths.get_filesystem(), paths.get_tool_cache(), msg_sink, packages_dir, tmp_archive_path); if (!compression_result) { msg_sink.println(Color::warning, @@ -2443,7 +2443,7 @@ std::string vcpkg::generate_nuspec(const Path& package_dir, { auto& spec = info.spec; auto& scf = info.source_control_file; - auto& version = scf.core_paragraph->raw_version; + auto& version = info.raw_version; std::string description = Strings::concat("NOT FOR DIRECT USE. Automatically generated cache package.\n\n", Strings::join("\n ", scf.core_paragraph->description), diff --git a/src/vcpkg/install.cpp b/src/vcpkg/install.cpp index 5761678695..c28649c23d 100644 --- a/src/vcpkg/install.cpp +++ b/src/vcpkg/install.cpp @@ -380,6 +380,9 @@ namespace vcpkg if (restore != RestoreResult::restored) { binary_cache.push_success(action, paths.package_dir(action.spec)); + } else if (action.build_options.clean_packages == CleanPackages::YES) + { + fs.remove_all(paths.package_dir(action.spec), VCPKG_LINE_INFO); } if (action.build_options.clean_downloads == CleanDownloads::YES) From 0912655f611a68d022cb2e6afcfbb2de64b22d75 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Thu, 2 Mar 2023 23:29:16 +0100 Subject: [PATCH 03/40] Adapt code review --- include/vcpkg/binarycaching.h | 33 +++++---- include/vcpkg/binarycaching.private.h | 2 +- include/vcpkg/fwd/binarycaching.h | 2 +- src/vcpkg-test/binarycaching.cpp | 11 ++- src/vcpkg/binarycaching.cpp | 97 ++++++++++++++------------- src/vcpkg/install.cpp | 3 +- 6 files changed, 80 insertions(+), 68 deletions(-) diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index 471bc29a0c..726d39f3ea 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -48,17 +48,21 @@ namespace vcpkg struct BinaryPackageInformation { - explicit BinaryPackageInformation(const InstallPlanAction& action); + explicit BinaryPackageInformation(const InstallPlanAction& action, std::string&& nuspec = ""); std::string package_abi; - // The following fields are only needed for the nuget binary cache PackageSpec spec; - SourceControlFile& source_control_file; - std::string& raw_version; - std::string compiler_id; - std::string compiler_version; - std::string triplet_abi; - InternalFeatureSet feature_list; - std::vector package_dependencies; + std::string raw_version; + std::string nuspec; // only filled if BinaryCache has a provider that returns true for needs_nuspec_data() + }; + + struct BinaryProviderPushRequest + { + BinaryProviderPushRequest(BinaryPackageInformation&& info, Path package_dir) + : info(std::move(info)), package_dir(std::move(package_dir)) + { + } + BinaryPackageInformation info; + Path package_dir; }; struct IBinaryProvider @@ -71,9 +75,7 @@ namespace vcpkg /// Called upon a successful build of `action` to store those contents in the binary cache. /// Prerequisite: action has a package_abi() - virtual void push_success(const BinaryPackageInformation& info, - const Path& packages_dir, - MessageSink& msg_sink) = 0; + virtual void push_success(const BinaryProviderPushRequest& request, MessageSink& msg_sink) = 0; /// Gives the IBinaryProvider an opportunity to batch any downloading or server communication for /// executing `actions`. @@ -89,6 +91,8 @@ namespace vcpkg /// to the action at the same index in `actions`. The provider must mark the cache status as appropriate. /// Prerequisite: `actions` have package ABIs. virtual void precheck(View actions, View cache_status) const = 0; + + virtual bool needs_nuspec_data() const { return false; } }; struct UrlTemplate @@ -170,13 +174,14 @@ namespace vcpkg private: struct ActionToPush { - BinaryPackageInformation info; - bool clean_after_push; + BinaryProviderPushRequest request; + bool clean_after_push = false; }; void push_thread_main(); std::unordered_map m_status; std::vector> m_providers; + bool needs_nuspec_data = false; std::condition_variable actions_to_push_notifier; std::mutex actions_to_push_mutex; std::queue actions_to_push; diff --git a/include/vcpkg/binarycaching.private.h b/include/vcpkg/binarycaching.private.h index a0369b31b8..95cca71009 100644 --- a/include/vcpkg/binarycaching.private.h +++ b/include/vcpkg/binarycaching.private.h @@ -62,7 +62,7 @@ namespace vcpkg } std::string generate_nuspec(const Path& package_dir, - const BinaryPackageInformation& info, + const InstallPlanAction& action, const NugetReference& ref, details::NuGetRepoInfo rinfo = details::get_nuget_repo_info_from_env()); } diff --git a/include/vcpkg/fwd/binarycaching.h b/include/vcpkg/fwd/binarycaching.h index 66665ceaaf..70fb40c445 100644 --- a/include/vcpkg/fwd/binarycaching.h +++ b/include/vcpkg/fwd/binarycaching.h @@ -25,5 +25,5 @@ namespace vcpkg struct IBinaryProvider; struct BinaryCache; struct BinaryConfigParserState; - struct BinaryPackageInformation; + struct BinaryProviderPushRequest; } diff --git a/src/vcpkg-test/binarycaching.cpp b/src/vcpkg-test/binarycaching.cpp index c84840124b..3b509bdf07 100644 --- a/src/vcpkg-test/binarycaching.cpp +++ b/src/vcpkg-test/binarycaching.cpp @@ -24,9 +24,9 @@ struct KnowNothingBinaryProvider : IBinaryProvider return RestoreResult::unavailable; } - void push_success(const BinaryPackageInformation& info, const Path&, MessageSink&) override + void push_success(const BinaryProviderPushRequest& request, MessageSink&) override { - CHECK_FALSE(info.package_abi.empty()); + CHECK_FALSE(request.info.package_abi.empty()); } void prefetch(View actions, View cache_status) const override @@ -280,7 +280,7 @@ Build-Depends: bzip REQUIRE(ref.nupkg_filename() == "zlib2_x64-windows.1.5.0-vcpkgpackageabi.nupkg"); { - auto nuspec = generate_nuspec(pkgPath, BinaryPackageInformation{ipa}, ref, {}); + auto nuspec = generate_nuspec(pkgPath, ipa, ref, {}); std::string expected = R"( zlib2_x64-windows @@ -308,7 +308,7 @@ Features: a, b } { - auto nuspec = generate_nuspec(pkgPath, BinaryPackageInformation{ipa}, ref, {"urlvalue"}); + auto nuspec = generate_nuspec(pkgPath, ipa, ref, {"urlvalue"}); std::string expected = R"( zlib2_x64-windows @@ -336,8 +336,7 @@ Features: a, b REQUIRE_EQUAL_TEXT(nuspec, expected); } { - auto nuspec = - generate_nuspec(pkgPath, BinaryPackageInformation{ipa}, ref, {"urlvalue", "branchvalue", "commitvalue"}); + auto nuspec = generate_nuspec(pkgPath, ipa, ref, {"urlvalue", "branchvalue", "commitvalue"}); std::string expected = R"( zlib2_x64-windows diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 5515f43c3d..e535373031 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -298,33 +298,31 @@ namespace return RestoreResult::unavailable; } - void push_success(const BinaryPackageInformation& info, - const Path& packages_dir, - MessageSink& msg_sink) override + void push_success(const BinaryProviderPushRequest& request, MessageSink& msg_sink) override { if (m_write_dirs.empty() && m_put_url_templates.empty()) { return; } - const auto& abi_tag = info.package_abi; - auto& spec = info.spec; + const auto& abi_tag = request.info.package_abi; + auto& spec = request.info.spec; auto& fs = paths.get_filesystem(); const auto archive_subpath = make_archive_subpath(abi_tag); const auto tmp_archive_path = make_temp_archive_path(paths.buildtrees(), spec); auto compress_result = - compress_directory_to_zip(fs, paths.get_tool_cache(), msg_sink, packages_dir, tmp_archive_path); + compress_directory_to_zip(fs, paths.get_tool_cache(), msg_sink, request.package_dir, tmp_archive_path); if (!compress_result) { msg_sink.println(Color::warning, - msg::format_warning(msgCompressFolderFailed, msg::path = packages_dir) + msg::format_warning(msgCompressFolderFailed, msg::path = request.package_dir) .append_raw(' ') .append_raw(compress_result.error())); return; } for (auto&& put_url_template : m_put_url_templates) { - auto url = put_url_template.instantiate_variables(info); + auto url = put_url_template.instantiate_variables(request.info); auto maybe_success = put_file(fs, url, m_secrets, put_url_template.headers_for_put, tmp_archive_path); if (maybe_success) { @@ -429,7 +427,7 @@ namespace RestoreResult try_restore(const InstallPlanAction&) const override { return RestoreResult::unavailable; } - void push_success(const BinaryPackageInformation&, const Path&, MessageSink&) override { } + void push_success(const BinaryProviderPushRequest&, MessageSink&) override { } void prefetch(View actions, View cache_status) const override { @@ -452,7 +450,7 @@ namespace auto&& action = actions[idx]; clean_prepare_dir(fs, paths.package_dir(action.spec)); - auto uri = url_template.instantiate_variables(BinaryPackageInformation{action}); + auto uri = url_template.instantiate_variables(BinaryPackageInformation{action, ""}); url_paths.emplace_back(std::move(uri), make_temp_archive_path(paths.buildtrees(), action.spec)); url_indices.push_back(idx); } @@ -806,21 +804,27 @@ namespace RestoreResult try_restore(const InstallPlanAction&) const override { return RestoreResult::unavailable; } - void push_success(const BinaryPackageInformation& info, - const Path& packages_dir, - MessageSink& msg_sink) override + bool needs_nuspec_data() const override { return !m_write_sources.empty() || !m_write_configs.empty(); } + + void push_success(const BinaryProviderPushRequest& request, MessageSink& msg_sink) override { if (m_write_sources.empty() && m_write_configs.empty()) { return; } + if (request.info.nuspec.empty()) + { + Checks::unreachable( + VCPKG_LINE_INFO, + "request.info.nuspec must be non empty because needs_nuspec_data() should return true"); + } - auto& spec = info.spec; + auto& spec = request.info.spec; - NugetReference nuget_ref = make_nugetref(info, get_nuget_prefix()); + NugetReference nuget_ref = make_nugetref(request.info, get_nuget_prefix()); auto nuspec_path = paths.buildtrees() / spec.name() / (spec.triplet().to_string() + ".nuspec"); auto& fs = paths.get_filesystem(); - fs.write_contents(nuspec_path, generate_nuspec(packages_dir, info, nuget_ref), VCPKG_LINE_INFO); + fs.write_contents(nuspec_path, request.info.nuspec, VCPKG_LINE_INFO); const auto& nuget_exe = paths.get_tool_exe("nuget", stdout_sink); Command cmdline; @@ -1008,21 +1012,19 @@ namespace RestoreResult try_restore(const InstallPlanAction&) const override { return RestoreResult::unavailable; } - void push_success(const BinaryPackageInformation& info, - const Path& packages_dir, - MessageSink& msg_sink) override + void push_success(const BinaryProviderPushRequest& request, MessageSink& msg_sink) override { if (m_write_prefixes.empty()) return; const ElapsedTimer timer; - const auto& abi = info.package_abi; - auto& spec = info.spec; + const auto& abi = request.info.package_abi; + auto& spec = request.info.spec; const auto tmp_archive_path = make_temp_archive_path(paths.buildtrees(), spec); auto compression_result = compress_directory_to_zip( - paths.get_filesystem(), paths.get_tool_cache(), msg_sink, packages_dir, tmp_archive_path); + paths.get_filesystem(), paths.get_tool_cache(), msg_sink, request.package_dir, tmp_archive_path); if (!compression_result) { msg_sink.println(Color::warning, - msg::format_warning(msgCompressFolderFailed, msg::path = packages_dir) + msg::format_warning(msgCompressFolderFailed, msg::path = request.package_dir) .append_raw(' ') .append_raw(compression_result.error())); return; @@ -1350,6 +1352,7 @@ namespace vcpkg std::make_move_iterator(providers.begin()), std::make_move_iterator(providers.end())); } + needs_nuspec_data = Util::any_of(m_providers, [](auto& provider) { return provider->needs_nuspec_data(); }); } void BinaryCache::install_providers_for(const VcpkgCmdArguments& args, const VcpkgPaths& paths) @@ -1429,8 +1432,16 @@ namespace vcpkg package_dir = new_packaged_dir; } + std::string nuspec; + if (needs_nuspec_data) + { + NugetReference nuget_ref = make_nugetref(action, get_nuget_prefix()); + nuspec = generate_nuspec(package_dir, action, nuget_ref); + } std::unique_lock lock(actions_to_push_mutex); - actions_to_push.push(ActionToPush{BinaryPackageInformation{action}, clean_packages, package_dir}); + actions_to_push.push(ActionToPush{ + BinaryProviderPushRequest{BinaryPackageInformation{action, std::move(nuspec)}, package_dir}, + clean_packages}); actions_to_push_notifier.notify_all(); } } @@ -1508,11 +1519,11 @@ namespace vcpkg for (auto&& provider : m_providers) { - provider->push_success(entry.info, entry.packages_dir, stdout_sink); + provider->push_success(entry.request, stdout_sink); } if (entry.clean_after_push) { - filesystem.remove_all(entry.packages_dir, VCPKG_LINE_INFO); + filesystem.remove_all(entry.request.package_dir, VCPKG_LINE_INFO); } actions_to_push_notifier.notify_all(); } @@ -1629,18 +1640,12 @@ namespace vcpkg secrets.clear(); } - BinaryPackageInformation::BinaryPackageInformation(const InstallPlanAction& action) + BinaryPackageInformation::BinaryPackageInformation(const InstallPlanAction& action, std::string&& nuspec) : package_abi(action.package_abi().value_or_exit(VCPKG_LINE_INFO)) , spec(action.spec) - , source_control_file( - *action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO).source_control_file) - , raw_version(source_control_file.core_paragraph->raw_version) - , compiler_id(action.abi_info.value_or_exit(VCPKG_LINE_INFO).compiler_info.value_or_exit(VCPKG_LINE_INFO).id) - , compiler_version( - action.abi_info.value_or_exit(VCPKG_LINE_INFO).compiler_info.value_or_exit(VCPKG_LINE_INFO).version) - , triplet_abi(action.abi_info.value_or_exit(VCPKG_LINE_INFO).triplet_abi.value_or_exit(VCPKG_LINE_INFO)) - , feature_list(action.feature_list) - , package_dependencies(action.package_dependencies) + , raw_version(action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO) + .source_control_file->core_paragraph->raw_version) + , nuspec(std::move(nuspec)) { } } @@ -2437,13 +2442,15 @@ details::NuGetRepoInfo details::get_nuget_repo_info_from_env() } std::string vcpkg::generate_nuspec(const Path& package_dir, - const BinaryPackageInformation& info, + const InstallPlanAction& action, const vcpkg::NugetReference& ref, details::NuGetRepoInfo rinfo) { - auto& spec = info.spec; - auto& scf = info.source_control_file; - auto& version = info.raw_version; + auto& spec = action.spec; + auto& scf = *action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO).source_control_file; + auto& version = scf.core_paragraph->raw_version; + const auto& abi_info = action.abi_info.value_or_exit(VCPKG_LINE_INFO); + const auto& compiler_info = abi_info.compiler_info.value_or_exit(VCPKG_LINE_INFO); std::string description = Strings::concat("NOT FOR DIRECT USE. Automatically generated cache package.\n\n", Strings::join("\n ", scf.core_paragraph->description), @@ -2452,16 +2459,16 @@ std::string vcpkg::generate_nuspec(const Path& package_dir, "\nTriplet: ", spec.triplet().to_string(), "\nCXX Compiler id: ", - info.compiler_id, + compiler_info.id, "\nCXX Compiler version: ", - info.compiler_version, + compiler_info.version, "\nTriplet/Compiler hash: ", - info.triplet_abi, + abi_info.triplet_abi.value_or_exit(VCPKG_LINE_INFO), "\nFeatures:", - Strings::join(",", info.feature_list, [](const std::string& s) { return " " + s; }), + Strings::join(",", action.feature_list, [](const std::string& s) { return " " + s; }), "\nDependencies:\n"); - for (auto&& dep : info.package_dependencies) + for (auto&& dep : action.package_dependencies) { Strings::append(description, " ", dep.name(), '\n'); } diff --git a/src/vcpkg/install.cpp b/src/vcpkg/install.cpp index c28649c23d..9e2b9cf49b 100644 --- a/src/vcpkg/install.cpp +++ b/src/vcpkg/install.cpp @@ -380,7 +380,8 @@ namespace vcpkg if (restore != RestoreResult::restored) { binary_cache.push_success(action, paths.package_dir(action.spec)); - } else if (action.build_options.clean_packages == CleanPackages::YES) + } + else if (action.build_options.clean_packages == CleanPackages::YES) { fs.remove_all(paths.package_dir(action.spec), VCPKG_LINE_INFO); } From 5d7288cee7378eb978dc9daf1f7022f0d37dce1c Mon Sep 17 00:00:00 2001 From: autoantwort <41973254+autoantwort@users.noreply.github.com> Date: Thu, 2 Mar 2023 23:38:34 +0100 Subject: [PATCH 04/40] Update src/vcpkg/binarycaching.cpp Co-authored-by: Robert Schumacher --- src/vcpkg/binarycaching.cpp | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index e535373031..cc96902fd2 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -1503,19 +1503,21 @@ namespace vcpkg void BinaryCache::push_thread_main() { + decltype(actions_to_push) my_tasks; while (true) { - std::unique_lock lock(actions_to_push_mutex); - actions_to_push_notifier.wait(lock, [this]() { return !actions_to_push.empty() || end_push_thread; }); - if (actions_to_push.empty()) { - if (end_push_thread) break; - continue; - } + std::unique_lock lock(actions_to_push_mutex); + actions_to_push_notifier.wait(lock, [this]() { return !actions_to_push.empty() || end_push_thread; }); + if (actions_to_push.empty()) + { + if (end_push_thread) break; + continue; + } - auto entry = std::move(actions_to_push.front()); - actions_to_push.pop(); - lock.unlock(); // we don't touch actions_to_push anymore + std::swap(my_tasks, actions_to_push); + } + // Now, consume all of `my_tasks` before taking the lock again. Make sure to call my_tasks.clear()! for (auto&& provider : m_providers) { From 10189ac39c0e5e4871bdcbb2ea8c5b0028cb95e4 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Thu, 2 Mar 2023 23:42:03 +0100 Subject: [PATCH 05/40] Adapt code review --- include/vcpkg/binarycaching.h | 2 +- src/vcpkg/binarycaching.cpp | 21 ++++++++++++--------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index 726d39f3ea..ed6c6d7510 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -184,7 +184,7 @@ namespace vcpkg bool needs_nuspec_data = false; std::condition_variable actions_to_push_notifier; std::mutex actions_to_push_mutex; - std::queue actions_to_push; + std::vector actions_to_push; std::thread push_thread; std::atomic_bool end_push_thread; Filesystem& filesystem; diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index cc96902fd2..a90f8d1f95 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -1439,7 +1439,7 @@ namespace vcpkg nuspec = generate_nuspec(package_dir, action, nuget_ref); } std::unique_lock lock(actions_to_push_mutex); - actions_to_push.push(ActionToPush{ + actions_to_push.push_back(ActionToPush{ BinaryProviderPushRequest{BinaryPackageInformation{action, std::move(nuspec)}, package_dir}, clean_packages}); actions_to_push_notifier.notify_all(); @@ -1517,17 +1517,20 @@ namespace vcpkg std::swap(my_tasks, actions_to_push); } - // Now, consume all of `my_tasks` before taking the lock again. Make sure to call my_tasks.clear()! - - for (auto&& provider : m_providers) - { - provider->push_success(entry.request, stdout_sink); - } - if (entry.clean_after_push) + // Now, consume all of `my_tasks` before taking the lock again. + for (auto& action_to_push : my_tasks) { - filesystem.remove_all(entry.request.package_dir, VCPKG_LINE_INFO); + for (auto&& provider : m_providers) + { + provider->push_success(action_to_push.request, stdout_sink); + } + if (action_to_push.clean_after_push) + { + filesystem.remove_all(action_to_push.request.package_dir, VCPKG_LINE_INFO); + } } actions_to_push_notifier.notify_all(); + my_tasks.clear(); } } From 2567607b6c14ea6df73283d8c468cea38db61717 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Thu, 2 Mar 2023 23:44:38 +0100 Subject: [PATCH 06/40] Remove unnecessary actions_to_push_notifier.notify_all() --- src/vcpkg/binarycaching.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index a90f8d1f95..5b1a746bb3 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -1529,7 +1529,6 @@ namespace vcpkg filesystem.remove_all(action_to_push.request.package_dir, VCPKG_LINE_INFO); } } - actions_to_push_notifier.notify_all(); my_tasks.clear(); } } From ecdd0008ed9e8a5f849acaa2e1b38c4397da6101 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Fri, 3 Mar 2023 00:32:30 +0100 Subject: [PATCH 07/40] Prevent deadlock and don't be on the crtl+c path --- src/vcpkg.cpp | 2 -- src/vcpkg/base/checks.cpp | 4 ++++ src/vcpkg/binarycaching.cpp | 9 +++++---- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/vcpkg.cpp b/src/vcpkg.cpp index e19c1ae240..8f8bdd853c 100644 --- a/src/vcpkg.cpp +++ b/src/vcpkg.cpp @@ -10,7 +10,6 @@ #include #include -#include #include #include #include @@ -157,7 +156,6 @@ namespace vcpkg::Checks // Implements link seam from basic_checks.h void on_final_cleanup_and_exit() { - BinaryCache::wait_for_async_complete(); const auto elapsed_us_inner = g_total_time.microseconds(); bool debugging = Debug::g_debugging; diff --git a/src/vcpkg/base/checks.cpp b/src/vcpkg/base/checks.cpp index 5473c19d5e..ab6ec1781d 100644 --- a/src/vcpkg/base/checks.cpp +++ b/src/vcpkg/base/checks.cpp @@ -3,6 +3,8 @@ #include #include +#include + #include namespace @@ -61,6 +63,8 @@ namespace vcpkg [[noreturn]] void Checks::exit_with_code(const LineInfo& line_info, const int exit_code) { + BinaryCache::wait_for_async_complete(); + Debug::println(locale_invariant_lineinfo(line_info)); final_cleanup_and_exit(exit_code); } diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 5b1a746bb3..4ad7369dd2 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -1314,12 +1314,13 @@ namespace vcpkg { if (current_instance) { + auto instance = current_instance; + current_instance = nullptr; msg::write_unlocalized_text_to_stdout(Color::none, "Wait for end\n"); - current_instance->end_push_thread = true; - current_instance->actions_to_push_notifier.notify_all(); - current_instance->push_thread.join(); + instance->end_push_thread = true; + instance->actions_to_push_notifier.notify_all(); + instance->push_thread.join(); msg::write_unlocalized_text_to_stdout(Color::none, "Wait for end done \n"); - current_instance = nullptr; } } From 8e7ae619f59ab925d662bcc3bb0139f34af98901 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Fri, 3 Mar 2023 18:49:05 +0100 Subject: [PATCH 08/40] Add and use BGMessageSink to print IBinaryProvider::push_success messages between package installs Co-authored-by: Robert Schumacher --- include/vcpkg/base/messages.h | 27 +++++++++++++++++ include/vcpkg/binarycaching.h | 3 ++ src/vcpkg/base/messages.cpp | 55 +++++++++++++++++++++++++++++++++++ src/vcpkg/binarycaching.cpp | 11 +++++-- src/vcpkg/install.cpp | 1 + 5 files changed, 95 insertions(+), 2 deletions(-) diff --git a/include/vcpkg/base/messages.h b/include/vcpkg/base/messages.h index 933b7996b7..ebd85d6bb7 100644 --- a/include/vcpkg/base/messages.h +++ b/include/vcpkg/base/messages.h @@ -8,9 +8,11 @@ #include #include +#include #include #include #include +#include namespace vcpkg { @@ -424,6 +426,31 @@ namespace vcpkg extern MessageSink& stdout_sink; extern MessageSink& stderr_sink; + struct BGMessageSink : MessageSink + { + MessageSink& out_sink; + + BGMessageSink(MessageSink& out_sink) : out_sink(out_sink) { } + ~BGMessageSink() { publish_directly_to_out_sink(); } + // must be called from producer + void print(Color c, StringView sv) override; + + // must be called from consumer (synchronizer of out) + void print_published(); + + void publish_directly_to_out_sink(); + + private: + std::mutex m_lock; + // guarded by m_lock + std::vector> m_published; + // unguarded, buffers messages until newline is reached + std::vector> m_unpublished; + + std::mutex m_print_directly_lock; + bool m_print_directly_to_out_sink = false; + }; + DECLARE_MESSAGE(ABaseline, (), "", "a baseline"); DECLARE_MESSAGE(ABaselineObject, (), "", "a baseline object"); DECLARE_MESSAGE(ABoolean, (), "", "a boolean"); diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index ed6c6d7510..5f2b288219 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -162,6 +162,8 @@ namespace vcpkg /// Called upon a successful build of `action` to store those contents in the binary cache. void push_success(const InstallPlanAction& action, Path package_dir); + void print_push_success_messages(); + /// Gives the IBinaryProvider an opportunity to batch any downloading or server communication for /// executing `actions`. void prefetch(View actions); @@ -179,6 +181,7 @@ namespace vcpkg }; void push_thread_main(); + BGMessageSink bg_msg_sink; std::unordered_map m_status; std::vector> m_providers; bool needs_nuspec_data = false; diff --git a/src/vcpkg/base/messages.cpp b/src/vcpkg/base/messages.cpp index c0d07350b8..33831feca0 100644 --- a/src/vcpkg/base/messages.cpp +++ b/src/vcpkg/base/messages.cpp @@ -498,6 +498,61 @@ namespace vcpkg MessageSink& stderr_sink = stderr_sink_instance; MessageSink& stdout_sink = stdout_sink_instance; + void BGMessageSink::print(Color c, StringView sv) + { + std::lock_guard print_lk(m_print_directly_lock); + if (m_print_directly_to_out_sink) + { + out_sink.print(c, sv); + return; + } + + std::string s = sv.to_string(); + auto pos = s.find_last_of('\n'); + if (pos != std::string::npos) + { + m_published.insert(m_published.end(), + std::make_move_iterator(m_unpublished.begin()), + std::make_move_iterator(m_unpublished.end())); + m_published.emplace_back(c, s.substr(0, pos + 1)); + m_unpublished.clear(); + if (s.size() > pos + 1) + { + m_unpublished.emplace_back(c, s.substr(pos + 1)); + } + } + else + { + m_unpublished.emplace_back(c, std::move(s)); + } + } + + void BGMessageSink::print_published() + { + std::lock_guard lk(m_lock); + for (auto&& m : m_published) + { + out_sink.print(m.first, m.second); + } + m_published.clear(); + } + + void BGMessageSink::publish_directly_to_out_sink() + { + std::lock_guard print_lk(m_print_directly_lock); + std::lock_guard lk(m_lock); + + m_print_directly_to_out_sink = true; + for (auto& messages : {&m_published, &m_unpublished}) + { + for (auto&& m : *messages) + { + out_sink.print(m.first, m.second); + } + messages->clear(); + } + } + REGISTER_MESSAGE(ABaseline); REGISTER_MESSAGE(ABoolean); REGISTER_MESSAGE(ABaselineObject); diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 4ad7369dd2..29ffa2baac 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -1317,6 +1317,7 @@ namespace vcpkg auto instance = current_instance; current_instance = nullptr; msg::write_unlocalized_text_to_stdout(Color::none, "Wait for end\n"); + instance->bg_msg_sink.publish_directly_to_out_sink(); instance->end_push_thread = true; instance->actions_to_push_notifier.notify_all(); instance->push_thread.join(); @@ -1325,7 +1326,11 @@ namespace vcpkg } BinaryCache::BinaryCache(Filesystem& filesystem) - : push_thread([this]() { push_thread_main(); }), end_push_thread{false}, filesystem(filesystem) + : bg_msg_sink(stdout_sink) + , push_thread([this]() { push_thread_main(); }) + , end_push_thread{false} + , filesystem(filesystem) + { Checks::check_exit(VCPKG_LINE_INFO, current_instance == nullptr); current_instance = this; @@ -1447,6 +1452,8 @@ namespace vcpkg } } + void BinaryCache::print_push_success_messages() { bg_msg_sink.print_published(); } + void BinaryCache::prefetch(View actions) { std::vector cache_status{actions.size()}; @@ -1523,7 +1530,7 @@ namespace vcpkg { for (auto&& provider : m_providers) { - provider->push_success(action_to_push.request, stdout_sink); + provider->push_success(action_to_push.request, bg_msg_sink); } if (action_to_push.clean_after_push) { diff --git a/src/vcpkg/install.cpp b/src/vcpkg/install.cpp index 9e2b9cf49b..629ead0be0 100644 --- a/src/vcpkg/install.cpp +++ b/src/vcpkg/install.cpp @@ -546,6 +546,7 @@ namespace vcpkg binary_cache.prefetch(action_plan.install_actions); for (auto&& action : action_plan.install_actions) { + binary_cache.print_push_success_messages(); TrackedPackageInstallGuard this_install(action_index++, action_count, results, action); auto result = perform_install_plan_action(args, paths, action, status_db, binary_cache, build_logs_recorder); From 850d7c9340d979d9c698edf97da8fd1d6abfaee3 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Fri, 3 Mar 2023 19:05:00 +0100 Subject: [PATCH 09/40] Restore old upload message --- src/vcpkg/binarycaching.cpp | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 29ffa2baac..850cdacabf 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -320,13 +320,14 @@ namespace .append_raw(compress_result.error())); return; } + size_t http_remotes_pushed = 0; for (auto&& put_url_template : m_put_url_templates) { auto url = put_url_template.instantiate_variables(request.info); auto maybe_success = put_file(fs, url, m_secrets, put_url_template.headers_for_put, tmp_archive_path); if (maybe_success) { - m_http_remotes_pushed++; + http_remotes_pushed++; continue; } @@ -362,18 +363,10 @@ namespace } if (!m_put_url_templates.empty()) { - msg_sink.println(msgUploadedBinaries, msg::count = m_http_remotes_pushed, msg::vendor = "HTTP remotes"); + msg_sink.println(msgUploadedBinaries, msg::count = http_remotes_pushed, msg::vendor = "HTTP remotes"); } } - /*void print_upload_statistics(MessageSink& msg_sink) override - { - if (!m_put_url_templates.empty()) - { - msg_sink.println(msgUploadedBinaries, msg::count = m_http_remotes_pushed, msg::vendor = "HTTP remotes"); - } - };*/ - void precheck(View actions, View cache_status) const override { auto& fs = paths.get_filesystem(); @@ -414,7 +407,6 @@ namespace std::vector m_write_dirs; std::vector m_put_url_templates; std::vector m_secrets; - size_t m_http_remotes_pushed = 0; }; struct HttpGetBinaryProvider : IBinaryProvider { From 548be38051928148a4a763164c79801e8b91ea79 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Sat, 4 Mar 2023 22:11:20 +0100 Subject: [PATCH 10/40] Don't join yourself --- src/vcpkg/binarycaching.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 850cdacabf..53cbdc39f0 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -1308,7 +1308,11 @@ namespace vcpkg { auto instance = current_instance; current_instance = nullptr; - msg::write_unlocalized_text_to_stdout(Color::none, "Wait for end\n"); + if (std::this_thread::get_id() == instance->push_thread.get_id()) + { + // Don't join yourself + return; + } instance->bg_msg_sink.publish_directly_to_out_sink(); instance->end_push_thread = true; instance->actions_to_push_notifier.notify_all(); From 6dbbf0643ea40b131bbca3b90dce79f876f1dbd6 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Sat, 4 Mar 2023 22:24:29 +0100 Subject: [PATCH 11/40] Print messages about remaining packages to upload --- include/vcpkg/binarycaching.h | 1 + src/vcpkg/binarycaching.cpp | 19 ++++++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index 5f2b288219..4ace8e9547 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -190,6 +190,7 @@ namespace vcpkg std::vector actions_to_push; std::thread push_thread; std::atomic_bool end_push_thread; + std::atomic_int remaining_packages_to_push = 0; Filesystem& filesystem; }; diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 53cbdc39f0..03106f0d02 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -1313,11 +1313,21 @@ namespace vcpkg // Don't join yourself return; } + bool have_remaining_packages = instance->remaining_packages_to_push > 0; + if (have_remaining_packages) + { + msg::write_unlocalized_text_to_stdout(Color::none, + fmt::format("Wait until the remaining {} packages are uploaded\n", + instance->remaining_packages_to_push)); + } instance->bg_msg_sink.publish_directly_to_out_sink(); instance->end_push_thread = true; instance->actions_to_push_notifier.notify_all(); instance->push_thread.join(); - msg::write_unlocalized_text_to_stdout(Color::none, "Wait for end done \n"); + if (have_remaining_packages) + { + msg::write_unlocalized_text_to_stdout(Color::none, "All packages uploaded\n"); + } } } @@ -1441,6 +1451,7 @@ namespace vcpkg nuspec = generate_nuspec(package_dir, action, nuget_ref); } std::unique_lock lock(actions_to_push_mutex); + remaining_packages_to_push++; actions_to_push.push_back(ActionToPush{ BinaryProviderPushRequest{BinaryPackageInformation{action, std::move(nuspec)}, package_dir}, clean_packages}); @@ -1524,6 +1535,11 @@ namespace vcpkg // Now, consume all of `my_tasks` before taking the lock again. for (auto& action_to_push : my_tasks) { + if (end_push_thread) + { + msg::write_unlocalized_text_to_stdout( + Color::none, fmt::format("Upload remaining {} package(s)\n", remaining_packages_to_push)); + } for (auto&& provider : m_providers) { provider->push_success(action_to_push.request, bg_msg_sink); @@ -1532,6 +1548,7 @@ namespace vcpkg { filesystem.remove_all(action_to_push.request.package_dir, VCPKG_LINE_INFO); } + remaining_packages_to_push--; } my_tasks.clear(); } From 74b86fd14b5b10e448e8e134caa0295ce4b4d8f9 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Sun, 5 Mar 2023 20:27:31 +0100 Subject: [PATCH 12/40] Localization --- include/vcpkg/base/messages.h | 6 ++++++ locales/messages.json | 5 +++++ src/vcpkg/base/messages.cpp | 3 +++ src/vcpkg/binarycaching.cpp | 9 +++------ 4 files changed, 17 insertions(+), 6 deletions(-) diff --git a/include/vcpkg/base/messages.h b/include/vcpkg/base/messages.h index ebd85d6bb7..3c546af176 100644 --- a/include/vcpkg/base/messages.h +++ b/include/vcpkg/base/messages.h @@ -575,6 +575,7 @@ namespace vcpkg "example of {value} is 'foo bar {'", "unbalanced brace in format string \"{value}\""); DECLARE_MESSAGE(AllPackagesAreUpdated, (), "", "All installed packages are up-to-date with the local portfile."); + DECLARE_MESSAGE(AllPackagesUploaded, (), "", "All packages uploaded"); DECLARE_MESSAGE(AlreadyInstalled, (msg::spec), "", "{spec} is already installed"); DECLARE_MESSAGE(AlreadyInstalledNotHead, (msg::spec), @@ -3067,6 +3068,7 @@ namespace vcpkg (msg::spec, msg::vendor, msg::path), "", "Uploading binaries for '{spec}' using '{vendor}' \"{path}\"."); + DECLARE_MESSAGE(UploadRemainingPackages, (msg::count), "", "Upload remaining {count} package(s)"); DECLARE_MESSAGE(UseEnvVar, (msg::env_var), "An example of env_var is \"HTTP(S)_PROXY\"" @@ -3298,6 +3300,10 @@ namespace vcpkg DECLARE_MESSAGE(VSNoInstances, (), "", "Could not locate a complete Visual Studio instance"); DECLARE_MESSAGE(WaitingForChildrenToExit, (), "", "Waiting for child processes to exit..."); DECLARE_MESSAGE(WaitingToTakeFilesystemLock, (msg::path), "", "waiting to take filesystem lock on {path}..."); + DECLARE_MESSAGE(WaitUntilPackagesUploaded, + (msg::count), + "", + "Wait until the remaining {count} packages are uploaded"); DECLARE_MESSAGE(WarningMessageMustUsePrintWarning, (msg::value), "{value} is is a localized message name like WarningMessageMustUsePrintWarning", diff --git a/locales/messages.json b/locales/messages.json index 7d89d9ca91..a76d554873 100644 --- a/locales/messages.json +++ b/locales/messages.json @@ -107,6 +107,7 @@ "AllFormatArgsUnbalancedBraces": "unbalanced brace in format string \"{value}\"", "_AllFormatArgsUnbalancedBraces.comment": "example of {value} is 'foo bar {'", "AllPackagesAreUpdated": "All installed packages are up-to-date with the local portfile.", + "AllPackagesUploaded": "All packages uploaded", "AlreadyInstalled": "{spec} is already installed", "_AlreadyInstalled.comment": "An example of {spec} is zlib:x64-windows.", "AlreadyInstalledNotHead": "{spec} is already installed -- not building from HEAD", @@ -1397,6 +1398,8 @@ "_UpdateBaselineUpdatedBaseline.comment": "example of {old_value}, {new_value} is '5507daa796359fe8d45418e694328e878ac2b82f' An example of {url} is https://github.com/microsoft/vcpkg.", "UpgradeInManifest": "The upgrade command does not currently support manifest mode. Instead, modify your vcpkg.json and run install.", "UpgradeRunWithNoDryRun": "If you are sure you want to rebuild the above packages, run this command with the --no-dry-run option.", + "UploadRemainingPackages": "Upload remaining {count} package(s)", + "_UploadRemainingPackages.comment": "An example of {count} is 42.", "UploadedBinaries": "Uploaded binaries to {count} {vendor}.", "_UploadedBinaries.comment": "An example of {count} is 42. An example of {vendor} is Azure.", "UploadedPackagesToVendor": "Uploaded {count} package(s) to {vendor} in {elapsed}", @@ -1498,6 +1501,8 @@ "VersionTableHeader": "Version", "VersionVerifiedOK": "OK: {package_name}@{version} -> {commit_sha}", "_VersionVerifiedOK.comment": "An example of {package_name} is zlib. An example of {version} is 1.3.8. An example of {commit_sha} is 7cfad47ae9f68b183983090afd6337cd60fd4949.", + "WaitUntilPackagesUploaded": "Wait until the remaining {count} packages are uploaded", + "_WaitUntilPackagesUploaded.comment": "An example of {count} is 42.", "WaitingForChildrenToExit": "Waiting for child processes to exit...", "WaitingToTakeFilesystemLock": "waiting to take filesystem lock on {path}...", "_WaitingToTakeFilesystemLock.comment": "An example of {path} is /foo/bar.", diff --git a/src/vcpkg/base/messages.cpp b/src/vcpkg/base/messages.cpp index 33831feca0..dc31a0f805 100644 --- a/src/vcpkg/base/messages.cpp +++ b/src/vcpkg/base/messages.cpp @@ -609,6 +609,7 @@ namespace vcpkg REGISTER_MESSAGE(AllFormatArgsRawArgument); REGISTER_MESSAGE(AllFormatArgsUnbalancedBraces); REGISTER_MESSAGE(AllPackagesAreUpdated); + REGISTER_MESSAGE(AllPackagesUploaded); REGISTER_MESSAGE(AlreadyInstalled); REGISTER_MESSAGE(AlreadyInstalledNotHead); REGISTER_MESSAGE(AnArtifactsGitRegistryUrl); @@ -1318,6 +1319,7 @@ namespace vcpkg REGISTER_MESSAGE(UploadedPackagesToVendor); REGISTER_MESSAGE(UploadingBinariesToVendor); REGISTER_MESSAGE(UploadingBinariesUsingVendor); + REGISTER_MESSAGE(UploadRemainingPackages); REGISTER_MESSAGE(UseEnvVar); REGISTER_MESSAGE(UserWideIntegrationDeleted); REGISTER_MESSAGE(UserWideIntegrationRemoved); @@ -1375,6 +1377,7 @@ namespace vcpkg REGISTER_MESSAGE(VSNoInstances); REGISTER_MESSAGE(WaitingForChildrenToExit); REGISTER_MESSAGE(WaitingToTakeFilesystemLock); + REGISTER_MESSAGE(WaitUntilPackagesUploaded); REGISTER_MESSAGE(WarningMessageMustUsePrintWarning); REGISTER_MESSAGE(WarningsTreatedAsErrors); REGISTER_MESSAGE(WarnOnParseConfig); diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 03106f0d02..67a135795a 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -1316,9 +1316,7 @@ namespace vcpkg bool have_remaining_packages = instance->remaining_packages_to_push > 0; if (have_remaining_packages) { - msg::write_unlocalized_text_to_stdout(Color::none, - fmt::format("Wait until the remaining {} packages are uploaded\n", - instance->remaining_packages_to_push)); + msg::println(msgWaitUntilPackagesUploaded, msg::count = instance->remaining_packages_to_push); } instance->bg_msg_sink.publish_directly_to_out_sink(); instance->end_push_thread = true; @@ -1326,7 +1324,7 @@ namespace vcpkg instance->push_thread.join(); if (have_remaining_packages) { - msg::write_unlocalized_text_to_stdout(Color::none, "All packages uploaded\n"); + msg::println(msgAllPackagesUploaded); } } } @@ -1537,8 +1535,7 @@ namespace vcpkg { if (end_push_thread) { - msg::write_unlocalized_text_to_stdout( - Color::none, fmt::format("Upload remaining {} package(s)\n", remaining_packages_to_push)); + msg::println(msgUploadRemainingPackages, msg::count = remaining_packages_to_push); } for (auto&& provider : m_providers) { From 5171d3e67d244a9c4a8eb7e301479c0a4b003306 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Sun, 5 Mar 2023 21:26:23 +0100 Subject: [PATCH 13/40] Improve messages --- include/vcpkg/base/messages.h | 2 +- locales/messages.json | 2 +- src/vcpkg/binarycaching.cpp | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/include/vcpkg/base/messages.h b/include/vcpkg/base/messages.h index 3c546af176..8f6af9b70d 100644 --- a/include/vcpkg/base/messages.h +++ b/include/vcpkg/base/messages.h @@ -3303,7 +3303,7 @@ namespace vcpkg DECLARE_MESSAGE(WaitUntilPackagesUploaded, (msg::count), "", - "Wait until the remaining {count} packages are uploaded"); + "Wait until the remaining packages ({count}) are uploaded"); DECLARE_MESSAGE(WarningMessageMustUsePrintWarning, (msg::value), "{value} is is a localized message name like WarningMessageMustUsePrintWarning", diff --git a/locales/messages.json b/locales/messages.json index a76d554873..e07495fc4f 100644 --- a/locales/messages.json +++ b/locales/messages.json @@ -1501,7 +1501,7 @@ "VersionTableHeader": "Version", "VersionVerifiedOK": "OK: {package_name}@{version} -> {commit_sha}", "_VersionVerifiedOK.comment": "An example of {package_name} is zlib. An example of {version} is 1.3.8. An example of {commit_sha} is 7cfad47ae9f68b183983090afd6337cd60fd4949.", - "WaitUntilPackagesUploaded": "Wait until the remaining {count} packages are uploaded", + "WaitUntilPackagesUploaded": "Wait until the remaining packages ({count}) are uploaded", "_WaitUntilPackagesUploaded.comment": "An example of {count} is 42.", "WaitingForChildrenToExit": "Waiting for child processes to exit...", "WaitingToTakeFilesystemLock": "waiting to take filesystem lock on {path}...", diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 67a135795a..9486ba147a 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -1316,6 +1316,7 @@ namespace vcpkg bool have_remaining_packages = instance->remaining_packages_to_push > 0; if (have_remaining_packages) { + instance->bg_msg_sink.print_published(); msg::println(msgWaitUntilPackagesUploaded, msg::count = instance->remaining_packages_to_push); } instance->bg_msg_sink.publish_directly_to_out_sink(); From d69ed8f3bb04d116ada551c8f7042857bab897e9 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Sun, 5 Mar 2023 21:28:33 +0100 Subject: [PATCH 14/40] No singleton and explicit calls to wait_for_async_complete() See https://github.com/microsoft/vcpkg-tool/pull/908#discussion_r1121006770 --- include/vcpkg/binarycaching.h | 4 +-- src/vcpkg/base/checks.cpp | 4 --- src/vcpkg/binarycaching.cpp | 40 ++++++++++------------------- src/vcpkg/build.cpp | 6 ++--- src/vcpkg/commands.ci.cpp | 2 +- src/vcpkg/commands.setinstalled.cpp | 3 ++- src/vcpkg/commands.upgrade.cpp | 1 + src/vcpkg/install.cpp | 3 ++- 8 files changed, 24 insertions(+), 39 deletions(-) diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index 4ace8e9547..e1d3d6089b 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -146,8 +146,6 @@ namespace vcpkg struct BinaryCache { - static BinaryCache* current_instance; - static void wait_for_async_complete(); BinaryCache(Filesystem& filesystem); explicit BinaryCache(const VcpkgCmdArguments& args, const VcpkgPaths& paths); @@ -173,6 +171,8 @@ namespace vcpkg /// Returns a vector where each index corresponds to the matching index in `actions`. std::vector precheck(View actions); + void wait_for_async_complete(); + private: struct ActionToPush { diff --git a/src/vcpkg/base/checks.cpp b/src/vcpkg/base/checks.cpp index ab6ec1781d..5473c19d5e 100644 --- a/src/vcpkg/base/checks.cpp +++ b/src/vcpkg/base/checks.cpp @@ -3,8 +3,6 @@ #include #include -#include - #include namespace @@ -63,8 +61,6 @@ namespace vcpkg [[noreturn]] void Checks::exit_with_code(const LineInfo& line_info, const int exit_code) { - BinaryCache::wait_for_async_complete(); - Debug::println(locale_invariant_lineinfo(line_info)); final_cleanup_and_exit(exit_code); } diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 9486ba147a..b610d9066b 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -1300,33 +1300,21 @@ namespace vcpkg .value_or_exit(VCPKG_LINE_INFO); } - BinaryCache* BinaryCache::current_instance; - void BinaryCache::wait_for_async_complete() { - if (current_instance) + bool have_remaining_packages = remaining_packages_to_push > 0; + if (have_remaining_packages) { - auto instance = current_instance; - current_instance = nullptr; - if (std::this_thread::get_id() == instance->push_thread.get_id()) - { - // Don't join yourself - return; - } - bool have_remaining_packages = instance->remaining_packages_to_push > 0; - if (have_remaining_packages) - { - instance->bg_msg_sink.print_published(); - msg::println(msgWaitUntilPackagesUploaded, msg::count = instance->remaining_packages_to_push); - } - instance->bg_msg_sink.publish_directly_to_out_sink(); - instance->end_push_thread = true; - instance->actions_to_push_notifier.notify_all(); - instance->push_thread.join(); - if (have_remaining_packages) - { - msg::println(msgAllPackagesUploaded); - } + bg_msg_sink.print_published(); + msg::println(msgWaitUntilPackagesUploaded, msg::count = remaining_packages_to_push); + } + bg_msg_sink.publish_directly_to_out_sink(); + end_push_thread = true; + actions_to_push_notifier.notify_all(); + push_thread.join(); + if (have_remaining_packages) + { + msg::println(msgAllPackagesUploaded); } } @@ -1337,8 +1325,6 @@ namespace vcpkg , filesystem(filesystem) { - Checks::check_exit(VCPKG_LINE_INFO, current_instance == nullptr); - current_instance = this; } BinaryCache::BinaryCache(const VcpkgCmdArguments& args, const VcpkgPaths& paths) @@ -1347,7 +1333,7 @@ namespace vcpkg install_providers_for(args, paths); } - BinaryCache::~BinaryCache() { BinaryCache::wait_for_async_complete(); } + BinaryCache::~BinaryCache() { wait_for_async_complete(); } void BinaryCache::install_providers(std::vector>&& providers) { diff --git a/src/vcpkg/build.cpp b/src/vcpkg/build.cpp index 075aa422f0..7e54e07061 100644 --- a/src/vcpkg/build.cpp +++ b/src/vcpkg/build.cpp @@ -66,9 +66,9 @@ namespace vcpkg::Build const IBuildLogsRecorder& build_logs_recorder, const VcpkgPaths& paths) { - Checks::exit_with_code( - VCPKG_LINE_INFO, - perform_ex(args, full_spec, host_triplet, provider, binary_cache, build_logs_recorder, paths)); + auto code = perform_ex(args, full_spec, host_triplet, provider, binary_cache, build_logs_recorder, paths); + binary_cache.wait_for_async_complete(); + Checks::exit_with_code(VCPKG_LINE_INFO, code); } const CommandStructure COMMAND_STRUCTURE = { diff --git a/src/vcpkg/commands.ci.cpp b/src/vcpkg/commands.ci.cpp index b18d18ed8d..5f86ea99b7 100644 --- a/src/vcpkg/commands.ci.cpp +++ b/src/vcpkg/commands.ci.cpp @@ -550,7 +550,7 @@ namespace vcpkg::Commands::CI it_xunit->second, xunitTestResults.build_xml(target_triplet), VCPKG_LINE_INFO); } } - + binary_cache.wait_for_async_complete(); Checks::exit_success(VCPKG_LINE_INFO); } diff --git a/src/vcpkg/commands.setinstalled.cpp b/src/vcpkg/commands.setinstalled.cpp index c168767f60..b6385c0f7e 100644 --- a/src/vcpkg/commands.setinstalled.cpp +++ b/src/vcpkg/commands.setinstalled.cpp @@ -126,6 +126,7 @@ namespace vcpkg::Commands::SetInstalled summary.print_failed(); if (!only_downloads) { + binary_cache.wait_for_async_complete(); Checks::exit_fail(VCPKG_LINE_INFO); } } @@ -142,7 +143,7 @@ namespace vcpkg::Commands::SetInstalled } } } - + binary_cache.wait_for_async_complete(); Checks::exit_success(VCPKG_LINE_INFO); } diff --git a/src/vcpkg/commands.upgrade.cpp b/src/vcpkg/commands.upgrade.cpp index aff3666cb4..f1f523e7dc 100644 --- a/src/vcpkg/commands.upgrade.cpp +++ b/src/vcpkg/commands.upgrade.cpp @@ -225,6 +225,7 @@ namespace vcpkg::Commands::Upgrade summary.print(); } + binary_cache.wait_for_async_complete(); Checks::exit_success(VCPKG_LINE_INFO); } diff --git a/src/vcpkg/install.cpp b/src/vcpkg/install.cpp index 629ead0be0..ebd131cb5e 100644 --- a/src/vcpkg/install.cpp +++ b/src/vcpkg/install.cpp @@ -558,6 +558,7 @@ namespace vcpkg issue_body_path, create_github_issue(args, result, paths, action), VCPKG_LINE_INFO); return issue_body_path; })); + binary_cache.wait_for_async_complete(); Checks::exit_fail(VCPKG_LINE_INFO); } @@ -1309,7 +1310,7 @@ namespace vcpkg Install::print_usage_information(*bpgh, printed_usages, fs, paths.installed()); } } - + binary_cache.wait_for_async_complete(); Checks::exit_with_code(VCPKG_LINE_INFO, summary.failed()); } From d46a4d60c2f6cf426b7a24b91667b61bc7a6a328 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 22 Mar 2023 13:08:13 +0100 Subject: [PATCH 15/40] Apply code review --- include/vcpkg/base/message-data.inc.h | 1 - include/vcpkg/base/message_sinks.h | 9 ++++--- include/vcpkg/base/messages.h | 1 - locales/messages.json | 1 - src/vcpkg/base/message_sinks.cpp | 11 +++++--- src/vcpkg/binarycaching.cpp | 39 +++++++++++++-------------- 6 files changed, 32 insertions(+), 30 deletions(-) diff --git a/include/vcpkg/base/message-data.inc.h b/include/vcpkg/base/message-data.inc.h index cae3183173..2c27989187 100644 --- a/include/vcpkg/base/message-data.inc.h +++ b/include/vcpkg/base/message-data.inc.h @@ -107,7 +107,6 @@ DECLARE_MESSAGE(AllFormatArgsUnbalancedBraces, "example of {value} is 'foo bar {'", "unbalanced brace in format string \"{value}\"") DECLARE_MESSAGE(AllPackagesAreUpdated, (), "", "All installed packages are up-to-date with the local portfile.") -DECLARE_MESSAGE(AllPackagesUploaded, (), "", "All packages uploaded") DECLARE_MESSAGE(AlreadyInstalled, (msg::spec), "", "{spec} is already installed") DECLARE_MESSAGE(AlreadyInstalledNotHead, (msg::spec), diff --git a/include/vcpkg/base/message_sinks.h b/include/vcpkg/base/message_sinks.h index ccfd774307..db68bfae5a 100644 --- a/include/vcpkg/base/message_sinks.h +++ b/include/vcpkg/base/message_sinks.h @@ -5,6 +5,8 @@ #include #include +#include + namespace vcpkg { @@ -90,8 +92,6 @@ namespace vcpkg struct BGMessageSink : MessageSink { - MessageSink& out_sink; - BGMessageSink(MessageSink& out_sink) : out_sink(out_sink) { } ~BGMessageSink() { publish_directly_to_out_sink(); } // must be called from producer @@ -103,10 +103,13 @@ namespace vcpkg void publish_directly_to_out_sink(); private: + MessageSink& out_sink; + std::mutex m_lock; // guarded by m_lock std::vector> m_published; - // unguarded, buffers messages until newline is reached + // buffers messages until newline is reached + // guarded by m_print_directly_lock std::vector> m_unpublished; std::mutex m_print_directly_lock; diff --git a/include/vcpkg/base/messages.h b/include/vcpkg/base/messages.h index dde08eb6f9..f2c83f9f7b 100644 --- a/include/vcpkg/base/messages.h +++ b/include/vcpkg/base/messages.h @@ -8,7 +8,6 @@ #include #include -#include #include #include #include diff --git a/locales/messages.json b/locales/messages.json index 4145376508..8425653363 100644 --- a/locales/messages.json +++ b/locales/messages.json @@ -107,7 +107,6 @@ "AllFormatArgsUnbalancedBraces": "unbalanced brace in format string \"{value}\"", "_AllFormatArgsUnbalancedBraces.comment": "example of {value} is 'foo bar {'", "AllPackagesAreUpdated": "All installed packages are up-to-date with the local portfile.", - "AllPackagesUploaded": "All packages uploaded", "AlreadyInstalled": "{spec} is already installed", "_AlreadyInstalled.comment": "An example of {spec} is zlib:x64-windows.", "AlreadyInstalledNotHead": "{spec} is already installed -- not building from HEAD", diff --git a/src/vcpkg/base/message_sinks.cpp b/src/vcpkg/base/message_sinks.cpp index 769c8b416d..8690435330 100644 --- a/src/vcpkg/base/message_sinks.cpp +++ b/src/vcpkg/base/message_sinks.cpp @@ -70,10 +70,13 @@ namespace vcpkg auto pos = s.find_last_of('\n'); if (pos != std::string::npos) { - m_published.insert(m_published.end(), - std::make_move_iterator(m_unpublished.begin()), - std::make_move_iterator(m_unpublished.end())); - m_published.emplace_back(c, s.substr(0, pos + 1)); + { + std::lock_guard lk(m_lock); + m_published.insert(m_published.end(), + std::make_move_iterator(m_unpublished.begin()), + std::make_move_iterator(m_unpublished.end())); + m_published.emplace_back(c, s.substr(0, pos + 1)); + } m_unpublished.clear(); if (s.size() > pos + 1) { diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 242a9fba62..abc80b0abb 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -390,6 +390,10 @@ namespace .append_raw('\n') .append_raw(ec.message())); } + else + { + msg_sink.println(msgStoredBinaryCache, msg::path = archive_path); + } } // In the case of 1 write dir, the file will be moved instead of copied if (m_write_dirs.size() != 1) @@ -598,7 +602,7 @@ namespace Strings::case_insensitive_ascii_equals(use_nuget_cache, "true") || use_nuget_cache == "1"; } - ExpectedL run_nuget_commandline(const Command& cmdline) const + ExpectedL run_nuget_commandline(const Command& cmdline, MessageSink& msg_sink) const { if (m_interactive) { @@ -615,12 +619,12 @@ namespace return cmd_execute_and_capture_output(cmdline).then([&](ExitCodeAndOutput&& res) -> ExpectedL { if (Debug::g_debugging) { - msg::write_unlocalized_text_to_stdout(Color::error, res.output); + msg_sink.print(Color::error, res.output); } if (res.output.find("Authentication may require manual action.") != std::string::npos) { - msg::println(Color::warning, msgAuthenticationMayRequireManualAction, msg::vendor = "Nuget"); + msg_sink.println(Color::warning, msgAuthenticationMayRequireManualAction, msg::vendor = "Nuget"); } if (res.exit_code == 0) @@ -631,20 +635,20 @@ namespace if (res.output.find("Response status code does not indicate success: 401 (Unauthorized)") != std::string::npos) { - msg::println(Color::warning, - msgFailedVendorAuthentication, - msg::vendor = "NuGet", - msg::url = docs::binarycaching_url); + msg_sink.println(Color::warning, + msgFailedVendorAuthentication, + msg::vendor = "NuGet", + msg::url = docs::binarycaching_url); } else if (res.output.find("for example \"-ApiKey AzureDevOps\"") != std::string::npos) { auto real_cmdline = cmdline; real_cmdline.string_arg("-ApiKey").string_arg("AzureDevOps"); return cmd_execute_and_capture_output(real_cmdline) - .then([](ExitCodeAndOutput&& res) -> ExpectedL { + .then([&](ExitCodeAndOutput&& res) -> ExpectedL { if (Debug::g_debugging) { - msg::write_unlocalized_text_to_stdout(Color::error, res.output); + msg_sink.print(Color::error, res.output); } if (res.exit_code == 0) @@ -799,7 +803,7 @@ namespace } generate_packages_config(fs, packages_config, attempts); - run_nuget_commandline(cmdline); + run_nuget_commandline(cmdline, stdout_sink); Util::erase_remove_if(attempts, [&](const NuGetPrefetchAttempt& nuget_ref) -> bool { // note that we would like the nupkg downloaded to buildtrees, but nuget.exe downloads it to the // output directory @@ -871,7 +875,7 @@ namespace cmdline.string_arg("-NonInteractive"); } - if (!run_nuget_commandline(cmdline)) + if (!run_nuget_commandline(cmdline, msg_sink)) { msg_sink.println(Color::error, msgPackingVendorFailed, msg::vendor = "NuGet"); return; @@ -899,7 +903,7 @@ namespace } msg_sink.println( msgUploadingBinariesToVendor, msg::spec = spec, msg::vendor = "NuGet", msg::path = write_src); - if (!run_nuget_commandline(cmd)) + if (!run_nuget_commandline(cmd, msg_sink)) { msg_sink.println( Color::error, msgPushingVendorFailed, msg::vendor = "NuGet", msg::path = write_src); @@ -909,7 +913,7 @@ namespace { Command cmd; #ifndef _WIN32 - cmd.string_arg(paths.get_tool_exe(Tools::MONO, stdout_sink)); + cmd.string_arg(paths.get_tool_exe(Tools::MONO, msg_sink)); #endif cmd.string_arg(nuget_exe) .string_arg("push") @@ -928,7 +932,7 @@ namespace msg::spec = spec, msg::vendor = "NuGet config", msg::path = write_cfg); - if (!run_nuget_commandline(cmd)) + if (!run_nuget_commandline(cmd, msg_sink)) { msg_sink.println( Color::error, msgPushingVendorFailed, msg::vendor = "NuGet", msg::path = write_cfg); @@ -1095,7 +1099,7 @@ namespace auto& spec = request.info.spec; const auto tmp_archive_path = make_temp_archive_path(paths.buildtrees(), spec); auto compression_result = compress_directory_to_zip( - paths.get_filesystem(), paths.get_tool_cache(), stdout_sink, paths.package_dir(spec), tmp_archive_path); + paths.get_filesystem(), paths.get_tool_cache(), msg_sink, paths.package_dir(spec), tmp_archive_path); if (!compression_result) { msg_sink.println(Color::warning, @@ -1579,10 +1583,6 @@ namespace vcpkg end_push_thread = true; actions_to_push_notifier.notify_all(); push_thread.join(); - if (have_remaining_packages) - { - msg::println(msgAllPackagesUploaded); - } } BinaryCache::BinaryCache(Filesystem& filesystem) @@ -1590,7 +1590,6 @@ namespace vcpkg , push_thread([this]() { push_thread_main(); }) , end_push_thread{false} , filesystem(filesystem) - { } From 5e5171856cc7556b418ec53315000e2a6903306d Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 22 Mar 2023 14:10:25 +0100 Subject: [PATCH 16/40] Trigger Build From a9ac558b2a425c6c2263c2c00e31a059e24aa44f Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 22 Mar 2023 16:45:28 +0100 Subject: [PATCH 17/40] No rename dance --- src/vcpkg/binarycaching.cpp | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index abc80b0abb..23e1804a1e 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -1685,16 +1685,7 @@ namespace vcpkg const auto abi = action.package_abi().get(); if (abi) { - const auto clean_packages = action.build_options.clean_packages == CleanPackages::YES; - if (clean_packages) - { - static int counter = 0; - Path new_packaged_dir = package_dir + "_push_" + std::to_string(++counter); - filesystem.remove_all(new_packaged_dir, VCPKG_LINE_INFO); - filesystem.rename(package_dir, new_packaged_dir, VCPKG_LINE_INFO); - package_dir = new_packaged_dir; - } - + const auto clean_packages = action.build_options.clean_packages == CleanPackages::YES; std::string nuspec; if (needs_nuspec_data) { From 4faf6745aeb98ad81ccf29c5cb95c304f67812aa Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 22 Mar 2023 17:57:56 +0100 Subject: [PATCH 18/40] Print upload to provider only once and not once per provider --- include/vcpkg/base/message-data.inc.h | 7 +-- include/vcpkg/base/message_sinks.h | 1 + include/vcpkg/binarycaching.h | 3 +- locales/messages.json | 8 +-- src/vcpkg/binarycaching.cpp | 78 ++++++++++++++------------- 5 files changed, 47 insertions(+), 50 deletions(-) diff --git a/include/vcpkg/base/message-data.inc.h b/include/vcpkg/base/message-data.inc.h index 2c27989187..607db3d4c4 100644 --- a/include/vcpkg/base/message-data.inc.h +++ b/include/vcpkg/base/message-data.inc.h @@ -2276,6 +2276,7 @@ DECLARE_MESSAGE(SpecifyTargetArch, "Specify the target architecture triplet. See 'vcpkg help triplet'.\n(default: '{env_var}')") DECLARE_MESSAGE(StartCodeUnitInContinue, (), "", "found start code unit in continue position") DECLARE_MESSAGE(StoredBinaryCache, (msg::path), "", "Stored binary cache: \"{path}\"") +DECLARE_MESSAGE(StoredBinariesToDestinations, (msg::count), "", "Stored binaries in {count} destinations.") DECLARE_MESSAGE(StoreOptionMissingSha, (), "", "--store option is invalid without a sha512") DECLARE_MESSAGE(SuccessfulyExported, (msg::package_name, msg::path), "", "Exported {package_name} to {path}") DECLARE_MESSAGE(SuggestGitPull, (), "", "The result may be outdated. Run `git pull` to get the latest results.") @@ -2527,11 +2528,6 @@ DECLARE_MESSAGE( (), "", "If you are sure you want to rebuild the above packages, run this command with the --no-dry-run option.") -DECLARE_MESSAGE(UploadedBinaries, (msg::count, msg::vendor), "", "Uploaded binaries to {count} {vendor}.") -DECLARE_MESSAGE(UploadedPackagesToVendor, - (msg::count, msg::elapsed, msg::vendor), - "", - "Uploaded {count} package(s) to {vendor} in {elapsed}") DECLARE_MESSAGE(UploadingBinariesToVendor, (msg::spec, msg::vendor, msg::path), "", @@ -2540,7 +2536,6 @@ DECLARE_MESSAGE(UploadingBinariesUsingVendor, (msg::spec, msg::vendor, msg::path), "", "Uploading binaries for '{spec}' using '{vendor}' \"{path}\".") -DECLARE_MESSAGE(UploadRemainingPackages, (msg::count), "", "Upload remaining {count} package(s)") DECLARE_MESSAGE(UseEnvVar, (msg::env_var), "An example of env_var is \"HTTP(S)_PROXY\"" diff --git a/include/vcpkg/base/message_sinks.h b/include/vcpkg/base/message_sinks.h index db68bfae5a..4138a77022 100644 --- a/include/vcpkg/base/message_sinks.h +++ b/include/vcpkg/base/message_sinks.h @@ -96,6 +96,7 @@ namespace vcpkg ~BGMessageSink() { publish_directly_to_out_sink(); } // must be called from producer void print(Color c, StringView sv) override; + using MessageSink::print; // must be called from consumer (synchronizer of out) void print_published(); diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index 25815bf372..eaae887263 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -76,7 +76,8 @@ namespace vcpkg /// Called upon a successful build of `action` to store those contents in the binary cache. /// Prerequisite: action has a package_abi() - virtual void push_success(const BinaryProviderPushRequest& request, MessageSink& msg_sink) = 0; + /// returns the number of successful uploads + virtual int push_success(const BinaryProviderPushRequest& request, MessageSink& msg_sink) = 0; /// Gives the IBinaryProvider an opportunity to batch any downloading or server communication for /// executing `actions`. diff --git a/locales/messages.json b/locales/messages.json index 8425653363..8c371f7691 100644 --- a/locales/messages.json +++ b/locales/messages.json @@ -1266,6 +1266,8 @@ "_SpecifyTargetArch.comment": "An example of {env_var} is VCPKG_DEFAULT_TRIPLET.", "StartCodeUnitInContinue": "found start code unit in continue position", "StoreOptionMissingSha": "--store option is invalid without a sha512", + "StoredBinariesToDestinations": "Stored binaries in {count} destinations.", + "_StoredBinariesToDestinations.comment": "An example of {count} is 42.", "StoredBinaryCache": "Stored binary cache: \"{path}\"", "_StoredBinaryCache.comment": "An example of {path} is /foo/bar.", "SuccessfulyExported": "Exported {package_name} to {path}", @@ -1410,12 +1412,6 @@ "_UpdateBaselineUpdatedBaseline.comment": "example of {old_value}, {new_value} is '5507daa796359fe8d45418e694328e878ac2b82f' An example of {url} is https://github.com/microsoft/vcpkg.", "UpgradeInManifest": "The upgrade command does not currently support manifest mode. Instead, modify your vcpkg.json and run install.", "UpgradeRunWithNoDryRun": "If you are sure you want to rebuild the above packages, run this command with the --no-dry-run option.", - "UploadRemainingPackages": "Upload remaining {count} package(s)", - "_UploadRemainingPackages.comment": "An example of {count} is 42.", - "UploadedBinaries": "Uploaded binaries to {count} {vendor}.", - "_UploadedBinaries.comment": "An example of {count} is 42. An example of {vendor} is Azure.", - "UploadedPackagesToVendor": "Uploaded {count} package(s) to {vendor} in {elapsed}", - "_UploadedPackagesToVendor.comment": "An example of {count} is 42. An example of {elapsed} is 3.532 min. An example of {vendor} is Azure.", "UploadingBinariesToVendor": "Uploading binaries for '{spec}' to '{vendor}' source \"{path}\".", "_UploadingBinariesToVendor.comment": "An example of {spec} is zlib:x64-windows. An example of {vendor} is Azure. An example of {path} is /foo/bar.", "UploadingBinariesUsingVendor": "Uploading binaries for '{spec}' using '{vendor}' \"{path}\".", diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 23e1804a1e..5c9d0f6dfb 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -333,11 +333,11 @@ namespace return RestoreResult::unavailable; } - void push_success(const BinaryProviderPushRequest& request, MessageSink& msg_sink) override + int push_success(const BinaryProviderPushRequest& request, MessageSink& msg_sink) override { if (m_write_dirs.empty() && m_put_url_templates.empty()) { - return; + return 0; } const auto& abi_tag = request.info.package_abi; @@ -353,16 +353,16 @@ namespace msg::format_warning(msgCompressFolderFailed, msg::path = request.package_dir) .append_raw(' ') .append_raw(compress_result.error())); - return; + return 0; } - size_t http_remotes_pushed = 0; + int count_stored = 0; for (auto&& put_url_template : m_put_url_templates) { auto url = put_url_template.instantiate_variables(request.info); auto maybe_success = put_file(fs, url, m_secrets, put_url_template.headers_for_put, tmp_archive_path); if (maybe_success) { - http_remotes_pushed++; + count_stored++; continue; } @@ -392,7 +392,7 @@ namespace } else { - msg_sink.println(msgStoredBinaryCache, msg::path = archive_path); + count_stored++; } } // In the case of 1 write dir, the file will be moved instead of copied @@ -400,10 +400,7 @@ namespace { fs.remove(tmp_archive_path, IgnoreErrors{}); } - if (!m_put_url_templates.empty()) - { - msg_sink.println(msgUploadedBinaries, msg::count = http_remotes_pushed, msg::vendor = "HTTP remotes"); - } + return count_stored; } void precheck(View actions, View cache_status) const override @@ -458,7 +455,7 @@ namespace RestoreResult try_restore(const InstallPlanAction&) const override { return RestoreResult::unavailable; } - void push_success(const BinaryProviderPushRequest&, MessageSink&) override { } + int push_success(const BinaryProviderPushRequest&, MessageSink&) override { return 0; } void prefetch(View actions, View cache_status) const override { @@ -837,11 +834,11 @@ namespace bool needs_nuspec_data() const override { return !m_write_sources.empty() || !m_write_configs.empty(); } - void push_success(const BinaryProviderPushRequest& request, MessageSink& msg_sink) override + int push_success(const BinaryProviderPushRequest& request, MessageSink& msg_sink) override { if (m_write_sources.empty() && m_write_configs.empty()) { - return; + return 0; } if (request.info.nuspec.empty()) { @@ -878,9 +875,9 @@ namespace if (!run_nuget_commandline(cmdline, msg_sink)) { msg_sink.println(Color::error, msgPackingVendorFailed, msg::vendor = "NuGet"); - return; + return 0; } - + int count_stored = 0; auto nupkg_path = paths.buildtrees() / nuget_ref.nupkg_filename(); for (auto&& write_src : m_write_sources) { @@ -908,6 +905,10 @@ namespace msg_sink.println( Color::error, msgPushingVendorFailed, msg::vendor = "NuGet", msg::path = write_src); } + else + { + count_stored++; + } } for (auto&& write_cfg : m_write_configs) { @@ -937,9 +938,14 @@ namespace msg_sink.println( Color::error, msgPushingVendorFailed, msg::vendor = "NuGet", msg::path = write_cfg); } + else + { + count_stored++; + } } fs.remove(nupkg_path, IgnoreErrors{}); + return count_stored; } void precheck(View, View) const override { } @@ -1090,9 +1096,9 @@ namespace RestoreResult try_restore(const InstallPlanAction&) const override { return RestoreResult::unavailable; } - void push_success(const BinaryProviderPushRequest& request, MessageSink& msg_sink) override + int push_success(const BinaryProviderPushRequest& request, MessageSink& msg_sink) override { - if (m_write_url.empty()) return; + if (m_write_url.empty()) return 0; const ElapsedTimer timer; auto& fs = paths.get_filesystem(); const auto& abi = request.info.package_abi; @@ -1106,7 +1112,7 @@ namespace msg::format_warning(msgCompressFolderFailed, msg::path = paths.package_dir(spec)) .append_raw(' ') .append_raw(compression_result.error())); - return; + return 0; } int64_t cache_size; @@ -1142,11 +1148,7 @@ namespace } } } - - msg_sink.println(msgUploadedPackagesToVendor, - msg::count = upload_count, - msg::elapsed = timer.elapsed(), - msg::vendor = "GHA"); + return upload_count; } void precheck(View actions, View cache_status) const override @@ -1275,9 +1277,9 @@ namespace RestoreResult try_restore(const InstallPlanAction&) const override { return RestoreResult::unavailable; } - void push_success(const BinaryProviderPushRequest& request, MessageSink& msg_sink) override + int push_success(const BinaryProviderPushRequest& request, MessageSink& msg_sink) override { - if (m_write_prefixes.empty()) return; + if (m_write_prefixes.empty()) return 0; const ElapsedTimer timer; const auto& abi = request.info.package_abi; auto& spec = request.info.spec; @@ -1290,7 +1292,7 @@ namespace msg::format_warning(msgCompressFolderFailed, msg::path = request.package_dir) .append_raw(' ') .append_raw(compression_result.error())); - return; + return 0; } size_t upload_count = 0; @@ -1301,11 +1303,7 @@ namespace ++upload_count; } } - - msg_sink.println(msgUploadedPackagesToVendor, - msg::count = upload_count, - msg::elapsed = timer.elapsed(), - msg::vendor = vendor()); + return upload_count; } void precheck(View actions, View cache_status) const override @@ -1685,7 +1683,7 @@ namespace vcpkg const auto abi = action.package_abi().get(); if (abi) { - const auto clean_packages = action.build_options.clean_packages == CleanPackages::YES; + const auto clean_packages = action.build_options.clean_packages == CleanPackages::YES; std::string nuspec; if (needs_nuspec_data) { @@ -1761,6 +1759,7 @@ namespace vcpkg void BinaryCache::push_thread_main() { decltype(actions_to_push) my_tasks; + int count_pushed = 0; while (true) { { @@ -1777,19 +1776,24 @@ namespace vcpkg // Now, consume all of `my_tasks` before taking the lock again. for (auto& action_to_push : my_tasks) { - if (end_push_thread) + int num_destinations = 0; + for (auto&& provider : m_providers) { - msg::println(msgUploadRemainingPackages, msg::count = remaining_packages_to_push); + num_destinations += provider->push_success(action_to_push.request, bg_msg_sink); } - for (auto&& provider : m_providers) + remaining_packages_to_push--; + bg_msg_sink.print(msgStoredBinariesToDestinations, msg::count = num_destinations); + if (end_push_thread) { - provider->push_success(action_to_push.request, bg_msg_sink); + count_pushed++; + bg_msg_sink.print(LocalizedString::from_raw( + fmt::format(" ({}/{})", count_pushed, count_pushed + remaining_packages_to_push))); } + bg_msg_sink.println(); if (action_to_push.clean_after_push) { filesystem.remove_all(action_to_push.request.package_dir, VCPKG_LINE_INFO); } - remaining_packages_to_push--; } my_tasks.clear(); } From b9be8c63711c666253c48e3ebd08691679e95027 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 22 Mar 2023 18:05:31 +0100 Subject: [PATCH 19/40] Fix tests --- src/vcpkg-test/binarycaching.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/vcpkg-test/binarycaching.cpp b/src/vcpkg-test/binarycaching.cpp index 3b509bdf07..9e46b1b549 100644 --- a/src/vcpkg-test/binarycaching.cpp +++ b/src/vcpkg-test/binarycaching.cpp @@ -24,9 +24,10 @@ struct KnowNothingBinaryProvider : IBinaryProvider return RestoreResult::unavailable; } - void push_success(const BinaryProviderPushRequest& request, MessageSink&) override + int push_success(const BinaryProviderPushRequest& request, MessageSink&) override { CHECK_FALSE(request.info.package_abi.empty()); + return 0; } void prefetch(View actions, View cache_status) const override From 78ca08104a253c7646d94f6928984bc0b5baa1a3 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Fri, 31 Mar 2023 18:04:21 +0200 Subject: [PATCH 20/40] Don't create unnecessary strings --- include/vcpkg/base/strings.h | 2 ++ src/vcpkg-test/strings.cpp | 8 ++++++++ src/vcpkg/base/message_sinks.cpp | 12 ++++++------ src/vcpkg/base/strings.cpp | 6 ++++++ 4 files changed, 22 insertions(+), 6 deletions(-) diff --git a/include/vcpkg/base/strings.h b/include/vcpkg/base/strings.h index 4afbe63aed..1d62a8b8a4 100644 --- a/include/vcpkg/base/strings.h +++ b/include/vcpkg/base/strings.h @@ -177,6 +177,8 @@ namespace vcpkg::Strings const char* find_first_of(StringView searched, StringView candidates); + std::string::size_type find_last(StringView searched, char c); + std::vector find_all_enclosed(StringView input, StringView left_delim, StringView right_delim); StringView find_exactly_one_enclosed(StringView input, StringView left_tag, StringView right_tag); diff --git a/src/vcpkg-test/strings.cpp b/src/vcpkg-test/strings.cpp index dc631c260a..262a7e2183 100644 --- a/src/vcpkg-test/strings.cpp +++ b/src/vcpkg-test/strings.cpp @@ -68,6 +68,14 @@ TEST_CASE ("find_first_of", "[strings]") REQUIRE(find_first_of("abcdefg", "gb") == std::string("bcdefg")); } +TEST_CASE ("find_last", "[strings]") +{ + using vcpkg::Strings::find_last; + REQUIRE(find_last("abcdefg", 'a') == 0); + REQUIRE(find_last("abcdefg", 'g') == 6); + REQUIRE(find_last("abcdefg", 'z') == std::string::npos); +} + TEST_CASE ("contains_any_ignoring_c_comments", "[strings]") { using vcpkg::Strings::contains_any_ignoring_c_comments; diff --git a/src/vcpkg/base/message_sinks.cpp b/src/vcpkg/base/message_sinks.cpp index 8690435330..afd1c555b4 100644 --- a/src/vcpkg/base/message_sinks.cpp +++ b/src/vcpkg/base/message_sinks.cpp @@ -1,4 +1,5 @@ #include +#include namespace { @@ -66,8 +67,7 @@ namespace vcpkg return; } - std::string s = sv.to_string(); - auto pos = s.find_last_of('\n'); + auto pos = Strings::find_last(sv, '\n'); if (pos != std::string::npos) { { @@ -75,17 +75,17 @@ namespace vcpkg m_published.insert(m_published.end(), std::make_move_iterator(m_unpublished.begin()), std::make_move_iterator(m_unpublished.end())); - m_published.emplace_back(c, s.substr(0, pos + 1)); + m_published.emplace_back(c, sv.substr(0, pos + 1)); } m_unpublished.clear(); - if (s.size() > pos + 1) + if (sv.size() > pos + 1) { - m_unpublished.emplace_back(c, s.substr(pos + 1)); + m_unpublished.emplace_back(c, sv.substr(pos + 1)); } } else { - m_unpublished.emplace_back(c, std::move(s)); + m_unpublished.emplace_back(c, sv); } } diff --git a/src/vcpkg/base/strings.cpp b/src/vcpkg/base/strings.cpp index 7c294e3461..bee6cab0f1 100644 --- a/src/vcpkg/base/strings.cpp +++ b/src/vcpkg/base/strings.cpp @@ -309,6 +309,12 @@ const char* Strings::find_first_of(StringView input, StringView chars) return std::find_first_of(input.begin(), input.end(), chars.begin(), chars.end()); } +std::string::size_type Strings::find_last(StringView searched, char c) +{ + auto iter = std::find(searched.rbegin(), searched.rend(), c); + return iter == searched.rend() ? std::string::npos : (&*iter - searched.begin()); +} + std::vector Strings::find_all_enclosed(StringView input, StringView left_delim, StringView right_delim) { auto it_left = input.begin(); From 579bfa904f9e4c7641e232f15fe4f702b83b29d5 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Fri, 31 Mar 2023 18:53:26 +0200 Subject: [PATCH 21/40] Rename to m_published_lock --- include/vcpkg/base/message_sinks.h | 5 ++--- src/vcpkg/base/message_sinks.cpp | 6 +++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/include/vcpkg/base/message_sinks.h b/include/vcpkg/base/message_sinks.h index 4138a77022..13caf233e3 100644 --- a/include/vcpkg/base/message_sinks.h +++ b/include/vcpkg/base/message_sinks.h @@ -106,13 +106,12 @@ namespace vcpkg private: MessageSink& out_sink; - std::mutex m_lock; - // guarded by m_lock + std::mutex m_published_lock; std::vector> m_published; + // buffers messages until newline is reached // guarded by m_print_directly_lock std::vector> m_unpublished; - std::mutex m_print_directly_lock; bool m_print_directly_to_out_sink = false; }; diff --git a/src/vcpkg/base/message_sinks.cpp b/src/vcpkg/base/message_sinks.cpp index afd1c555b4..4897d6ea92 100644 --- a/src/vcpkg/base/message_sinks.cpp +++ b/src/vcpkg/base/message_sinks.cpp @@ -71,7 +71,7 @@ namespace vcpkg if (pos != std::string::npos) { { - std::lock_guard lk(m_lock); + std::lock_guard lk(m_published_lock); m_published.insert(m_published.end(), std::make_move_iterator(m_unpublished.begin()), std::make_move_iterator(m_unpublished.end())); @@ -91,7 +91,7 @@ namespace vcpkg void BGMessageSink::print_published() { - std::lock_guard lk(m_lock); + std::lock_guard lk(m_published_lock); for (auto&& m : m_published) { out_sink.print(m.first, m.second); @@ -102,7 +102,7 @@ namespace vcpkg void BGMessageSink::publish_directly_to_out_sink() { std::lock_guard print_lk(m_print_directly_lock); - std::lock_guard lk(m_lock); + std::lock_guard lk(m_published_lock); m_print_directly_to_out_sink = true; for (auto& messages : {&m_published, &m_unpublished}) From 103968ef0650b730b59b8651925239c7d9064fbb Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Fri, 31 Mar 2023 18:54:37 +0200 Subject: [PATCH 22/40] BinaryPackageInformation use Optional and make BinaryProviderPushRequest a aggregate --- include/vcpkg/binarycaching.h | 10 ++++------ src/vcpkg/binarycaching.cpp | 13 +++++++++---- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index eaae887263..2295e585b3 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -49,19 +49,17 @@ namespace vcpkg struct BinaryPackageInformation { - explicit BinaryPackageInformation(const InstallPlanAction& action, std::string&& nuspec = ""); + explicit BinaryPackageInformation(const InstallPlanAction& action); + explicit BinaryPackageInformation(const InstallPlanAction& action, std::string&& nuspec); std::string package_abi; PackageSpec spec; std::string raw_version; - std::string nuspec; // only filled if BinaryCache has a provider that returns true for needs_nuspec_data() + Optional + nuspec; // only filled if BinaryCache has a provider that returns true for needs_nuspec_data() }; struct BinaryProviderPushRequest { - BinaryProviderPushRequest(BinaryPackageInformation&& info, Path package_dir) - : info(std::move(info)), package_dir(std::move(package_dir)) - { - } BinaryPackageInformation info; Path package_dir; }; diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 5c9d0f6dfb..0906a5d4be 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -840,7 +840,7 @@ namespace { return 0; } - if (request.info.nuspec.empty()) + if (!request.info.nuspec.has_value()) { Checks::unreachable( VCPKG_LINE_INFO, @@ -852,7 +852,7 @@ namespace NugetReference nuget_ref = make_nugetref(request.info, get_nuget_prefix()); auto nuspec_path = paths.buildtrees() / spec.name() / (spec.triplet().to_string() + ".nuspec"); auto& fs = paths.get_filesystem(); - fs.write_contents(nuspec_path, request.info.nuspec, VCPKG_LINE_INFO); + fs.write_contents(nuspec_path, request.info.nuspec.value_or_exit(VCPKG_LINE_INFO), VCPKG_LINE_INFO); const auto& nuget_exe = paths.get_tool_exe("nuget", stdout_sink); Command cmdline; @@ -1912,14 +1912,19 @@ namespace vcpkg secrets.clear(); } - BinaryPackageInformation::BinaryPackageInformation(const InstallPlanAction& action, std::string&& nuspec) + BinaryPackageInformation::BinaryPackageInformation(const InstallPlanAction& action) : package_abi(action.package_abi().value_or_exit(VCPKG_LINE_INFO)) , spec(action.spec) , raw_version(action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO) .source_control_file->core_paragraph->raw_version) - , nuspec(std::move(nuspec)) { } + + BinaryPackageInformation::BinaryPackageInformation(const InstallPlanAction& action, std::string&& nuspec) + : BinaryPackageInformation(action) + { + this->nuspec = std::move(nuspec); + } } namespace From b666f94161a908237063ad2a2bb8c6cd3b573856 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 31 May 2023 19:50:22 +0200 Subject: [PATCH 23/40] Add missing files --- include/vcpkg/base/batch-quere.h | 57 ++++++++++++++++++++++++++++ include/vcpkg/base/fwd/batch-quere.h | 3 ++ src/vcpkg/base/batch-quere.cpp | 1 + 3 files changed, 61 insertions(+) create mode 100644 include/vcpkg/base/batch-quere.h create mode 100644 include/vcpkg/base/fwd/batch-quere.h create mode 100644 src/vcpkg/base/batch-quere.cpp diff --git a/include/vcpkg/base/batch-quere.h b/include/vcpkg/base/batch-quere.h new file mode 100644 index 0000000000..d1bc3c06b6 --- /dev/null +++ b/include/vcpkg/base/batch-quere.h @@ -0,0 +1,57 @@ +#pragma once + +template +class BatchQuere +{ +public: + template + void push(Args&&... args) + { + forward.emplace_back(std::forward(args)...); + } + + bool empty() const { return forward.empty(); } + + void pop(std::vector& out) + { + out.clear(); + swap(out, forward); + } + +private: + std::vector forward; +}; + +template +struct BGThreadBatchQueue +{ + template + void push(Args&&... args) + { + std::lock_guard lock(m_mtx); + m_tasks.push(std::forward(args)...); + m_cv.notify_all(); + } + + void wait_for_items(std::vector& out) + { + std::unique_lock lock(m_mtx); + m_cv.wait(lock, [this]() { return !m_tasks.empty() || !m_running; }); + m_tasks.pop(out); + } + + void stop() + { + std::lock_guard lock(m_mtx); + m_running = false; + m_cv.notify_all(); + } + + bool stopped() const { return m_running; } + +private: + std::mutex m_mtx; + std::condition_variable m_cv; + BatchQuere m_tasks; + bool m_running = true; +}; diff --git a/include/vcpkg/base/fwd/batch-quere.h b/include/vcpkg/base/fwd/batch-quere.h new file mode 100644 index 0000000000..3b364ef190 --- /dev/null +++ b/include/vcpkg/base/fwd/batch-quere.h @@ -0,0 +1,3 @@ +#pragma once + +class BatchQuere; diff --git a/src/vcpkg/base/batch-quere.cpp b/src/vcpkg/base/batch-quere.cpp new file mode 100644 index 0000000000..805021b2ff --- /dev/null +++ b/src/vcpkg/base/batch-quere.cpp @@ -0,0 +1 @@ +#include From 15bb5036c886179e228e6f012d7379e88daec488 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 31 May 2023 20:11:49 +0200 Subject: [PATCH 24/40] Add missing includes --- include/vcpkg/base/batch-quere.h | 4 ++++ include/vcpkg/base/fwd/batch-quere.h | 3 --- src/vcpkg/base/batch-quere.cpp | 1 - 3 files changed, 4 insertions(+), 4 deletions(-) delete mode 100644 include/vcpkg/base/fwd/batch-quere.h delete mode 100644 src/vcpkg/base/batch-quere.cpp diff --git a/include/vcpkg/base/batch-quere.h b/include/vcpkg/base/batch-quere.h index d1bc3c06b6..c9377a8510 100644 --- a/include/vcpkg/base/batch-quere.h +++ b/include/vcpkg/base/batch-quere.h @@ -1,5 +1,9 @@ #pragma once +#include +#include +#include + template class BatchQuere { diff --git a/include/vcpkg/base/fwd/batch-quere.h b/include/vcpkg/base/fwd/batch-quere.h deleted file mode 100644 index 3b364ef190..0000000000 --- a/include/vcpkg/base/fwd/batch-quere.h +++ /dev/null @@ -1,3 +0,0 @@ -#pragma once - -class BatchQuere; diff --git a/src/vcpkg/base/batch-quere.cpp b/src/vcpkg/base/batch-quere.cpp deleted file mode 100644 index 805021b2ff..0000000000 --- a/src/vcpkg/base/batch-quere.cpp +++ /dev/null @@ -1 +0,0 @@ -#include From d995bfd972bc035fb751025fd6da9ab1143565b4 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 31 May 2023 20:48:36 +0200 Subject: [PATCH 25/40] Make BianryCache a unique_ptr --- include/vcpkg/binarycaching.h | 12 +++-- src/vcpkg/binarycaching.cpp | 80 ++++++++++++++-------------- src/vcpkg/commands.build.cpp | 2 +- src/vcpkg/commands.ci.cpp | 8 +-- src/vcpkg/commands.install.cpp | 8 +-- src/vcpkg/commands.set-installed.cpp | 10 ++-- src/vcpkg/commands.upgrade.cpp | 6 +-- 7 files changed, 65 insertions(+), 61 deletions(-) diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index 6dd0e89a87..ee696cc49e 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -202,11 +202,13 @@ namespace vcpkg struct BinaryCache : ReadOnlyBinaryCache { - static ExpectedL make(const VcpkgCmdArguments& args, const VcpkgPaths& paths, MessageSink& sink); + static ExpectedL> make(const VcpkgCmdArguments& args, + const VcpkgPaths& paths, + MessageSink& sink); BinaryCache(Filesystem& fs); BinaryCache(const BinaryCache&) = delete; - BinaryCache(BinaryCache&&) = default; + BinaryCache(BinaryCache&&) = delete; ~BinaryCache(); /// Called upon a successful build of `action` to store those contents in the binary cache. @@ -231,9 +233,9 @@ namespace vcpkg void push_thread_main(); - std::unique_ptr m_bg_msg_sink; - std::unique_ptr> m_actions_to_push; - std::unique_ptr m_remaining_packages_to_push = 0; + BGMessageSink m_bg_msg_sink; + BGThreadBatchQueue m_actions_to_push; + std::atomic_int m_remaining_packages_to_push = 0; std::thread m_push_thread; }; diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 642429fc63..dce3cda207 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -2122,36 +2122,38 @@ namespace vcpkg }); } - BinaryCache::BinaryCache(Filesystem& fs) : m_fs(fs) { } - - ExpectedL BinaryCache::make(const VcpkgCmdArguments& args, const VcpkgPaths& paths, MessageSink& sink) - { - return make_binary_providers(args, paths).then([&](BinaryProviders&& p) -> ExpectedL { - BinaryCache b(std::move(p), paths.get_filesystem()); - b.m_needs_nuspec_data = Util::any_of(b.m_config.write, [](auto&& p) { return p->needs_nuspec_data(); }); - b.m_needs_zip_file = Util::any_of(b.m_config.write, [](auto&& p) { return p->needs_zip_file(); }); - if (b.m_needs_zip_file) - { - auto maybe_zt = ZipTool::make(paths.get_tool_cache(), sink); - if (auto z = maybe_zt.get()) - { - b.m_zip_tool.emplace(std::move(*z)); - } - else - { - return std::move(maybe_zt).error(); + BinaryCache::BinaryCache(Filesystem& fs) : m_fs(fs), m_bg_msg_sink(stdout_sink) { } + + ExpectedL> BinaryCache::make(const VcpkgCmdArguments& args, + const VcpkgPaths& paths, + MessageSink& sink) + { + return make_binary_providers(args, paths) + .then([&](BinaryProviders&& p) -> ExpectedL> { + std::unique_ptr b(new BinaryCache(std::move(p), paths.get_filesystem())); + b->m_needs_nuspec_data = + Util::any_of(b->m_config.write, [](auto&& p) { return p->needs_nuspec_data(); }); + b->m_needs_zip_file = Util::any_of(b->m_config.write, [](auto&& p) { return p->needs_zip_file(); }); + if (b->m_needs_zip_file) + { + auto maybe_zt = ZipTool::make(paths.get_tool_cache(), sink); + if (auto z = maybe_zt.get()) + { + b->m_zip_tool.emplace(std::move(*z)); + } + else + { + return std::move(maybe_zt).error(); + } } - } - return std::move(b); - }); + return std::move(b); + }); } BinaryCache::BinaryCache(BinaryProviders&& providers, Filesystem& fs) : ReadOnlyBinaryCache(std::move(providers)) , m_fs(fs) - , m_bg_msg_sink(std::make_unique(stdout_sink)) - , m_actions_to_push(std::make_unique>()) - , m_remaining_packages_to_push(std::make_unique()) + , m_bg_msg_sink(stdout_sink) , m_push_thread([this]() { push_thread_main(); }) { @@ -2180,8 +2182,8 @@ namespace vcpkg const auto clean_packages = action.build_options.clean_packages == CleanPackages::YES; - m_remaining_packages_to_push->fetch_add(1); - m_actions_to_push->push(ActionToPush{std::move(request), clean_packages}); + m_remaining_packages_to_push++; + m_actions_to_push.push(ActionToPush{std::move(request), clean_packages}); return; } } @@ -2191,18 +2193,18 @@ namespace vcpkg } } - void BinaryCache::print_push_success_messages() { m_bg_msg_sink->print_published(); } + void BinaryCache::print_push_success_messages() { m_bg_msg_sink.print_published(); } void BinaryCache::wait_for_async_complete() { bool have_remaining_packages = m_remaining_packages_to_push > 0; if (have_remaining_packages) { - m_bg_msg_sink->print_published(); - msg::println(msgWaitUntilPackagesUploaded, msg::count = m_remaining_packages_to_push->load()); + m_bg_msg_sink.print_published(); + msg::println(msgWaitUntilPackagesUploaded, msg::count = m_remaining_packages_to_push.load()); } - m_bg_msg_sink->publish_directly_to_out_sink(); - m_actions_to_push->stop(); + m_bg_msg_sink.publish_directly_to_out_sink(); + m_actions_to_push.stop(); m_push_thread.join(); } @@ -2212,7 +2214,7 @@ namespace vcpkg int count_pushed = 0; while (true) { - m_actions_to_push->wait_for_items(my_tasks); + m_actions_to_push.wait_for_items(my_tasks); if (my_tasks.empty()) { break; @@ -2232,7 +2234,7 @@ namespace vcpkg } else { - m_bg_msg_sink->println( + m_bg_msg_sink.println( Color::warning, msg::format_warning(msgCompressFolderFailed, msg::path = action_to_push.request.package_dir) .append_raw(' ') @@ -2252,18 +2254,18 @@ namespace vcpkg { m_fs.remove(*action_to_push.request.zip_path.get(), IgnoreErrors{}); } - m_bg_msg_sink->println( + m_bg_msg_sink.println( msgStoredBinariesToDestinations, msg::count = num_destinations, msg::elapsed = timer.elapsed()); // END - m_remaining_packages_to_push->fetch_sub(1); - if (m_actions_to_push->stopped()) + m_remaining_packages_to_push.fetch_sub(1); + if (m_actions_to_push.stopped()) { count_pushed++; - m_bg_msg_sink->print(LocalizedString::from_raw( - fmt::format(" ({}/{})", count_pushed, count_pushed + m_remaining_packages_to_push->load()))); + m_bg_msg_sink.print(LocalizedString::from_raw( + fmt::format(" ({}/{})", count_pushed, count_pushed + m_remaining_packages_to_push.load()))); } - m_bg_msg_sink->println(); + m_bg_msg_sink.println(); if (action_to_push.clean_after_push) { m_fs.remove_all(action_to_push.request.package_dir, VCPKG_LINE_INFO); diff --git a/src/vcpkg/commands.build.cpp b/src/vcpkg/commands.build.cpp index 402b4fab5c..71e0c85453 100644 --- a/src/vcpkg/commands.build.cpp +++ b/src/vcpkg/commands.build.cpp @@ -170,7 +170,7 @@ namespace vcpkg::Build msg::print(create_user_troubleshooting_message(*action, paths, nullopt)); return 1; } - binary_cache.push_success(*action); + binary_cache->push_success(*action); return 0; } diff --git a/src/vcpkg/commands.ci.cpp b/src/vcpkg/commands.ci.cpp index aeddba7770..bb099adfea 100644 --- a/src/vcpkg/commands.ci.cpp +++ b/src/vcpkg/commands.ci.cpp @@ -433,7 +433,7 @@ namespace vcpkg::Commands::CI auto action_plan = compute_full_plan(paths, provider, var_provider, all_default_full_specs, serialize_options); auto binary_cache = BinaryCache::make(args, paths, stdout_sink).value_or_exit(VCPKG_LINE_INFO); - const auto precheck_results = binary_cache.precheck(action_plan.install_actions); + const auto precheck_results = binary_cache->precheck(action_plan.install_actions); auto split_specs = compute_action_statuses(ExclusionPredicate{&exclusions_map}, var_provider, precheck_results, action_plan); @@ -515,9 +515,9 @@ namespace vcpkg::Commands::CI msg::println_warning(msgCISkipInstallation, msg::list = Strings::join(", ", already_installed)); } Install::preclear_packages(paths, action_plan); - binary_cache.fetch(action_plan.install_actions); + binary_cache->fetch(action_plan.install_actions); auto summary = Install::execute_plan( - args, action_plan, KeepGoing::YES, paths, status_db, binary_cache, build_logs_recorder); + args, action_plan, KeepGoing::YES, paths, status_db, *binary_cache, build_logs_recorder); for (auto&& result : summary.results) { @@ -564,7 +564,7 @@ namespace vcpkg::Commands::CI it_xunit->second, xunitTestResults.build_xml(target_triplet), VCPKG_LINE_INFO); } } - binary_cache.wait_for_async_complete(); + binary_cache->wait_for_async_complete(); Checks::exit_success(VCPKG_LINE_INFO); } } diff --git a/src/vcpkg/commands.install.cpp b/src/vcpkg/commands.install.cpp index cbc3eb781b..67d41f6140 100644 --- a/src/vcpkg/commands.install.cpp +++ b/src/vcpkg/commands.install.cpp @@ -1287,11 +1287,11 @@ namespace vcpkg track_install_plan(action_plan); Install::preclear_packages(paths, action_plan); - auto binary_cache = only_downloads ? BinaryCache(paths.get_filesystem()) + auto binary_cache = only_downloads ? std::make_unique(paths.get_filesystem()) : BinaryCache::make(args, paths, stdout_sink).value_or_exit(VCPKG_LINE_INFO); - binary_cache.fetch(action_plan.install_actions); + binary_cache->fetch(action_plan.install_actions); const InstallSummary summary = Install::execute_plan( - args, action_plan, keep_going, paths, status_db, binary_cache, null_build_logs_recorder()); + args, action_plan, keep_going, paths, status_db, *binary_cache, null_build_logs_recorder()); if (keep_going == KeepGoing::YES) { @@ -1329,7 +1329,7 @@ namespace vcpkg Install::print_usage_information(*bpgh, printed_usages, fs, paths.installed()); } } - binary_cache.wait_for_async_complete(); + binary_cache->wait_for_async_complete(); Checks::exit_with_code(VCPKG_LINE_INFO, summary.failed()); } diff --git a/src/vcpkg/commands.set-installed.cpp b/src/vcpkg/commands.set-installed.cpp index 97b29b0d7b..5a42fc703e 100644 --- a/src/vcpkg/commands.set-installed.cpp +++ b/src/vcpkg/commands.set-installed.cpp @@ -133,18 +133,18 @@ namespace vcpkg::Commands::SetInstalled track_install_plan(action_plan); Install::preclear_packages(paths, action_plan); - auto binary_cache = only_downloads ? BinaryCache(paths.get_filesystem()) + auto binary_cache = only_downloads ? std::make_unique(paths.get_filesystem()) : BinaryCache::make(args, paths, stdout_sink).value_or_exit(VCPKG_LINE_INFO); - binary_cache.fetch(action_plan.install_actions); + binary_cache->fetch(action_plan.install_actions); const auto summary = Install::execute_plan( - args, action_plan, keep_going, paths, status_db, binary_cache, null_build_logs_recorder()); + args, action_plan, keep_going, paths, status_db, *binary_cache, null_build_logs_recorder()); if (keep_going == KeepGoing::YES && summary.failed()) { summary.print_failed(); if (!only_downloads) { - binary_cache.wait_for_async_complete(); + binary_cache->wait_for_async_complete(); Checks::exit_fail(VCPKG_LINE_INFO); } } @@ -161,7 +161,7 @@ namespace vcpkg::Commands::SetInstalled } } } - binary_cache.wait_for_async_complete(); + binary_cache->wait_for_async_complete(); Checks::exit_success(VCPKG_LINE_INFO); } diff --git a/src/vcpkg/commands.upgrade.cpp b/src/vcpkg/commands.upgrade.cpp index 930defd1e2..c6765f81c1 100644 --- a/src/vcpkg/commands.upgrade.cpp +++ b/src/vcpkg/commands.upgrade.cpp @@ -206,16 +206,16 @@ namespace vcpkg::Commands::Upgrade auto binary_cache = BinaryCache::make(args, paths, stdout_sink).value_or_exit(VCPKG_LINE_INFO); compute_all_abis(paths, action_plan, var_provider, status_db); - binary_cache.fetch(action_plan.install_actions); + binary_cache->fetch(action_plan.install_actions); const InstallSummary summary = Install::execute_plan( - args, action_plan, keep_going, paths, status_db, binary_cache, null_build_logs_recorder()); + args, action_plan, keep_going, paths, status_db, *binary_cache, null_build_logs_recorder()); if (keep_going == KeepGoing::YES) { summary.print(); } - binary_cache.wait_for_async_complete(); + binary_cache->wait_for_async_complete(); Checks::exit_success(VCPKG_LINE_INFO); } } From 24cd026e92c79f10a34e597174352ab405a9bd09 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 31 May 2023 20:51:31 +0200 Subject: [PATCH 26/40] Reduce changes --- include/vcpkg/base/strings.h | 4 +++- include/vcpkg/binarycaching.h | 3 --- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/include/vcpkg/base/strings.h b/include/vcpkg/base/strings.h index 6375177936..500971d333 100644 --- a/include/vcpkg/base/strings.h +++ b/include/vcpkg/base/strings.h @@ -161,7 +161,9 @@ namespace vcpkg::Strings [[nodiscard]] std::string::size_type find_last(StringView searched, char c); - std::vector find_all_enclosed(StringView input, StringView left_delim, StringView right_delim); + [[nodiscard]] std::vector find_all_enclosed(StringView input, + StringView left_delim, + StringView right_delim); [[nodiscard]] StringView find_exactly_one_enclosed(StringView input, StringView left_tag, StringView right_tag); diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index ee696cc49e..406a3ef1ec 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -15,12 +15,9 @@ #include #include -#include #include -#include #include -#include #include #include #include From 92fc76b26ee821072ab0723deb8c1a2cea401a87 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 31 May 2023 20:59:55 +0200 Subject: [PATCH 27/40] Fix output --- include/vcpkg/base/batch-quere.h | 2 +- src/vcpkg/binarycaching.cpp | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/include/vcpkg/base/batch-quere.h b/include/vcpkg/base/batch-quere.h index c9377a8510..1595dbf1fb 100644 --- a/include/vcpkg/base/batch-quere.h +++ b/include/vcpkg/base/batch-quere.h @@ -51,7 +51,7 @@ struct BGThreadBatchQueue m_cv.notify_all(); } - bool stopped() const { return m_running; } + bool stopped() const { return !m_running; } private: std::mutex m_mtx; diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index dce3cda207..a622071415 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -2254,10 +2254,9 @@ namespace vcpkg { m_fs.remove(*action_to_push.request.zip_path.get(), IgnoreErrors{}); } - m_bg_msg_sink.println( + m_bg_msg_sink.print( msgStoredBinariesToDestinations, msg::count = num_destinations, msg::elapsed = timer.elapsed()); - // END m_remaining_packages_to_push.fetch_sub(1); if (m_actions_to_push.stopped()) { From 3527227878898f3e14f08761bda84d0937ede95e Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 31 May 2023 21:00:39 +0200 Subject: [PATCH 28/40] Fix bug --- src/vcpkg/binarycaching.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index a622071415..2e75005db1 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -2205,7 +2205,9 @@ namespace vcpkg } m_bg_msg_sink.publish_directly_to_out_sink(); m_actions_to_push.stop(); - m_push_thread.join(); + if (m_push_thread.joinable()) { + m_push_thread.join(); + } } void BinaryCache::push_thread_main() From 48305b327af17df43c831591ea641261537d703e Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 31 May 2023 21:01:04 +0200 Subject: [PATCH 29/40] Format --- src/vcpkg/binarycaching.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 2e75005db1..a2e5b01ef0 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -2205,7 +2205,8 @@ namespace vcpkg } m_bg_msg_sink.publish_directly_to_out_sink(); m_actions_to_push.stop(); - if (m_push_thread.joinable()) { + if (m_push_thread.joinable()) + { m_push_thread.join(); } } From 27fa0767d35c6fd89507ba67d357fa626033dc2d Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 31 May 2023 21:53:31 +0200 Subject: [PATCH 30/40] Use lock_guard --- include/vcpkg/base/batch-quere.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/vcpkg/base/batch-quere.h b/include/vcpkg/base/batch-quere.h index 1595dbf1fb..48f984d93a 100644 --- a/include/vcpkg/base/batch-quere.h +++ b/include/vcpkg/base/batch-quere.h @@ -39,7 +39,7 @@ struct BGThreadBatchQueue void wait_for_items(std::vector& out) { - std::unique_lock lock(m_mtx); + std::lock_guard lock(m_mtx); m_cv.wait(lock, [this]() { return !m_tasks.empty() || !m_running; }); m_tasks.pop(out); } From bcd459a80529a2dcbdcb4cde12d2492ff0b39520 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 31 May 2023 21:55:54 +0200 Subject: [PATCH 31/40] Revert "Use lock_guard" This reverts commit 27fa0767d35c6fd89507ba67d357fa626033dc2d. --- include/vcpkg/base/batch-quere.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/vcpkg/base/batch-quere.h b/include/vcpkg/base/batch-quere.h index 48f984d93a..1595dbf1fb 100644 --- a/include/vcpkg/base/batch-quere.h +++ b/include/vcpkg/base/batch-quere.h @@ -39,7 +39,7 @@ struct BGThreadBatchQueue void wait_for_items(std::vector& out) { - std::lock_guard lock(m_mtx); + std::unique_lock lock(m_mtx); m_cv.wait(lock, [this]() { return !m_tasks.empty() || !m_running; }); m_tasks.pop(out); } From f958d3626cdb03759e4962a5b76b8cf45dbf648e Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 31 May 2023 21:56:35 +0200 Subject: [PATCH 32/40] Use enum --- include/vcpkg/binarycaching.h | 3 ++- src/vcpkg/binarycaching.cpp | 6 ++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index 406a3ef1ec..b03170a29b 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -225,7 +226,7 @@ namespace vcpkg struct ActionToPush { BinaryPackageWriteInfo request; - bool clean_after_push = false; + CleanPackages clean_after_push; }; void push_thread_main(); diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index a2e5b01ef0..cdfa2dcd11 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -2180,10 +2180,8 @@ namespace vcpkg generate_nuspec(request.package_dir, action, m_config.nuget_prefix, m_config.nuget_repo); } - const auto clean_packages = action.build_options.clean_packages == CleanPackages::YES; - m_remaining_packages_to_push++; - m_actions_to_push.push(ActionToPush{std::move(request), clean_packages}); + m_actions_to_push.push(ActionToPush{std::move(request), action.build_options.clean_packages}); return; } } @@ -2268,7 +2266,7 @@ namespace vcpkg fmt::format(" ({}/{})", count_pushed, count_pushed + m_remaining_packages_to_push.load()))); } m_bg_msg_sink.println(); - if (action_to_push.clean_after_push) + if (action_to_push.clean_after_push == CleanPackages::YES) { m_fs.remove_all(action_to_push.request.package_dir, VCPKG_LINE_INFO); } From 7a24007b47afd9bec060d3756e4545b5d60f4758 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 31 May 2023 22:14:48 +0200 Subject: [PATCH 33/40] BGMessageSink::print_published apply code review --- src/vcpkg/base/message_sinks.cpp | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/vcpkg/base/message_sinks.cpp b/src/vcpkg/base/message_sinks.cpp index 8082d3b868..b51ea91474 100644 --- a/src/vcpkg/base/message_sinks.cpp +++ b/src/vcpkg/base/message_sinks.cpp @@ -92,12 +92,26 @@ namespace vcpkg void BGMessageSink::print_published() { - std::lock_guard lk(m_published_lock); - for (auto&& m : m_published) + std::vector> tmp; + for (;;) { - out_sink.print(m.first, m.second); + { + std::lock_guard lk(m_published_lock); + swap(tmp, m_published); + } + + if (tmp.empty()) + { + return; + } + + for (auto&& m : tmp) + { + out_sink.print(m.first, m.second); + } + + tmp.clear(); } - m_published.clear(); } void BGMessageSink::publish_directly_to_out_sink() From eccd9ee317604214195b37481c4e17007f4eaed2 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Thu, 24 Aug 2023 16:37:58 +0200 Subject: [PATCH 34/40] Fix typo --- include/vcpkg/base/batch-quere.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/vcpkg/base/batch-quere.h b/include/vcpkg/base/batch-quere.h index 1595dbf1fb..6026ad91f6 100644 --- a/include/vcpkg/base/batch-quere.h +++ b/include/vcpkg/base/batch-quere.h @@ -5,7 +5,7 @@ #include template -class BatchQuere +class BatchQueue { public: template @@ -46,7 +46,7 @@ struct BGThreadBatchQueue void stop() { - std::lock_guard lock(m_mtx); + std::lock_guard lock(m_mtx); m_running = false; m_cv.notify_all(); } @@ -56,6 +56,6 @@ struct BGThreadBatchQueue private: std::mutex m_mtx; std::condition_variable m_cv; - BatchQuere m_tasks; + BatchQueue m_tasks; bool m_running = true; }; From e7837e07bc5bff75b15e2ea2caf1872c334d5ff7 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Thu, 24 Aug 2023 16:39:31 +0200 Subject: [PATCH 35/40] Fix typo in file name --- include/vcpkg/base/{batch-quere.h => batch-queue.h} | 0 include/vcpkg/binarycaching.h | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename include/vcpkg/base/{batch-quere.h => batch-queue.h} (100%) diff --git a/include/vcpkg/base/batch-quere.h b/include/vcpkg/base/batch-queue.h similarity index 100% rename from include/vcpkg/base/batch-quere.h rename to include/vcpkg/base/batch-queue.h diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index 67ae37487c..6c301f1e7f 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -8,7 +8,7 @@ #include #include -#include +#include #include #include #include From 809d0b6b49b8b60eb0d0ffbf45f62d5863dfc698 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Wed, 11 Oct 2023 16:20:31 +0200 Subject: [PATCH 36/40] Renamings --- include/vcpkg/binarycaching.h | 2 +- src/vcpkg/base/message_sinks.cpp | 8 ++++---- src/vcpkg/binarycaching.cpp | 29 ++++++++++++++-------------- src/vcpkg/commands.ci.cpp | 2 +- src/vcpkg/commands.install.cpp | 4 ++-- src/vcpkg/commands.set-installed.cpp | 4 ++-- src/vcpkg/commands.upgrade.cpp | 4 ++-- 7 files changed, 27 insertions(+), 26 deletions(-) diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index 6c301f1e7f..2695c21d5d 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -213,7 +213,7 @@ namespace vcpkg void push_success(const InstallPlanAction& action); void print_push_success_messages(); - void wait_for_async_complete(); + void wait_for_async_complete_and_join(); private: BinaryCache(BinaryProviders&& providers, const Filesystem& fs); diff --git a/src/vcpkg/base/message_sinks.cpp b/src/vcpkg/base/message_sinks.cpp index 5f8a457356..bf79fca492 100644 --- a/src/vcpkg/base/message_sinks.cpp +++ b/src/vcpkg/base/message_sinks.cpp @@ -105,9 +105,9 @@ namespace vcpkg return; } - for (auto&& m : tmp) + for (auto&& msg : tmp) { - out_sink.print(m.first, m.second); + out_sink.print(msg.first, msg.second); } tmp.clear(); @@ -122,9 +122,9 @@ namespace vcpkg m_print_directly_to_out_sink = true; for (auto& messages : {&m_published, &m_unpublished}) { - for (auto&& m : *messages) + for (auto&& msg : *messages) { - out_sink.print(m.first, m.second); + out_sink.print(msg.first, msg.second); } messages->clear(); } diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index ed53a46abd..fcbdb4f2f3 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -2122,24 +2122,25 @@ namespace vcpkg MessageSink& sink) { return make_binary_providers(args, paths) - .then([&](BinaryProviders&& p) -> ExpectedL> { - std::unique_ptr b(new BinaryCache(std::move(p), paths.get_filesystem())); - b->m_needs_nuspec_data = - Util::any_of(b->m_config.write, [](auto&& p) { return p->needs_nuspec_data(); }); - b->m_needs_zip_file = Util::any_of(b->m_config.write, [](auto&& p) { return p->needs_zip_file(); }); - if (b->m_needs_zip_file) - { - auto maybe_zt = ZipTool::make(paths.get_tool_cache(), sink); - if (auto z = maybe_zt.get()) + .then([&](BinaryProviders&& providers) -> ExpectedL> { + std::unique_ptr cache(new BinaryCache(std::move(providers), paths.get_filesystem())); + cache->m_needs_nuspec_data = + Util::any_of(cache->m_config.write, [](auto&& p) { return p->needs_nuspec_data(); }); + cache->m_needs_zip_file = + Util::any_of(cache->m_config.write, [](auto&& p) { return p->needs_zip_file(); }); + if (cache->m_needs_zip_file) + { + auto maybe_zip_tool = ZipTool::make(paths.get_tool_cache(), sink); + if (auto zip_tool = maybe_zip_tool.get()) { - b->m_zip_tool.emplace(std::move(*z)); + cache->m_zip_tool.emplace(std::move(*zip_tool)); } else { - return std::move(maybe_zt).error(); + return std::move(maybe_zip_tool).error(); } } - return std::move(b); + return std::move(cache); }); } @@ -2151,7 +2152,7 @@ namespace vcpkg { } - BinaryCache::~BinaryCache() { wait_for_async_complete(); } + BinaryCache::~BinaryCache() { wait_for_async_complete_and_join(); } void BinaryCache::push_success(const InstallPlanAction& action) { @@ -2186,7 +2187,7 @@ namespace vcpkg void BinaryCache::print_push_success_messages() { m_bg_msg_sink.print_published(); } - void BinaryCache::wait_for_async_complete() + void BinaryCache::wait_for_async_complete_and_join() { bool have_remaining_packages = m_remaining_packages_to_push > 0; if (have_remaining_packages) diff --git a/src/vcpkg/commands.ci.cpp b/src/vcpkg/commands.ci.cpp index 76f1693409..2fbc964931 100644 --- a/src/vcpkg/commands.ci.cpp +++ b/src/vcpkg/commands.ci.cpp @@ -579,7 +579,7 @@ namespace vcpkg it_xunit->second, xunitTestResults.build_xml(target_triplet), VCPKG_LINE_INFO); } } - binary_cache->wait_for_async_complete(); + binary_cache->wait_for_async_complete_and_join(); Checks::exit_success(VCPKG_LINE_INFO); } } // namespace vcpkg diff --git a/src/vcpkg/commands.install.cpp b/src/vcpkg/commands.install.cpp index c7e5e869c5..bd96ac31ed 100644 --- a/src/vcpkg/commands.install.cpp +++ b/src/vcpkg/commands.install.cpp @@ -588,7 +588,7 @@ namespace vcpkg issue_body_path, create_github_issue(args, result, paths, action), VCPKG_LINE_INFO); return issue_body_path; })); - binary_cache.wait_for_async_complete(); + binary_cache.wait_for_async_complete_and_join(); Checks::exit_fail(VCPKG_LINE_INFO); } @@ -1380,7 +1380,7 @@ namespace vcpkg install_print_usage_information(*bpgh, printed_usages, fs, paths.installed()); } } - binary_cache->wait_for_async_complete(); + binary_cache->wait_for_async_complete_and_join(); Checks::exit_with_code(VCPKG_LINE_INFO, summary.failed()); } diff --git a/src/vcpkg/commands.set-installed.cpp b/src/vcpkg/commands.set-installed.cpp index fe651128be..d52d02bf31 100644 --- a/src/vcpkg/commands.set-installed.cpp +++ b/src/vcpkg/commands.set-installed.cpp @@ -251,7 +251,7 @@ namespace vcpkg summary.print_failed(); if (!only_downloads) { - binary_cache->wait_for_async_complete(); + binary_cache->wait_for_async_complete_and_join(); Checks::exit_fail(VCPKG_LINE_INFO); } } @@ -268,7 +268,7 @@ namespace vcpkg } } } - binary_cache->wait_for_async_complete(); + binary_cache->wait_for_async_complete_and_join(); Checks::exit_success(VCPKG_LINE_INFO); } diff --git a/src/vcpkg/commands.upgrade.cpp b/src/vcpkg/commands.upgrade.cpp index 8b51e7c733..dff660ba70 100644 --- a/src/vcpkg/commands.upgrade.cpp +++ b/src/vcpkg/commands.upgrade.cpp @@ -221,8 +221,8 @@ namespace vcpkg { summary.print(); } - - binary_cache->wait_for_async_complete(); + + binary_cache->wait_for_async_complete_and_join(); Checks::exit_success(VCPKG_LINE_INFO); } } // namespace vcpkg From 455e29b1a2c9c54a3fdcf8ffd0c5dcb9d3d0eb61 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Thu, 12 Oct 2023 12:48:55 +0200 Subject: [PATCH 37/40] format --- src/vcpkg/commands.upgrade.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vcpkg/commands.upgrade.cpp b/src/vcpkg/commands.upgrade.cpp index dff660ba70..8e4bf24e23 100644 --- a/src/vcpkg/commands.upgrade.cpp +++ b/src/vcpkg/commands.upgrade.cpp @@ -221,7 +221,7 @@ namespace vcpkg { summary.print(); } - + binary_cache->wait_for_async_complete_and_join(); Checks::exit_success(VCPKG_LINE_INFO); } From f4bad8cd0b7b2c6b3c788384f0cac48a26f4cd15 Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Sat, 4 Nov 2023 16:02:08 +0100 Subject: [PATCH 38/40] BinaryCache and std::unique_ptr --- include/vcpkg/binarycaching.h | 12 ++-- src/vcpkg/binarycaching.cpp | 88 +++++++++++++++------------- src/vcpkg/commands.build.cpp | 2 +- src/vcpkg/commands.ci.cpp | 8 +-- src/vcpkg/commands.install.cpp | 8 +-- src/vcpkg/commands.set-installed.cpp | 10 ++-- src/vcpkg/commands.upgrade.cpp | 6 +- 7 files changed, 68 insertions(+), 66 deletions(-) diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index 2695c21d5d..f502740e25 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -200,13 +200,11 @@ namespace vcpkg struct BinaryCache : ReadOnlyBinaryCache { - static ExpectedL> make(const VcpkgCmdArguments& args, - const VcpkgPaths& paths, - MessageSink& sink); + static ExpectedL make(const VcpkgCmdArguments& args, const VcpkgPaths& paths, MessageSink& sink); BinaryCache(const Filesystem& fs); BinaryCache(const BinaryCache&) = delete; - BinaryCache(BinaryCache&&) = delete; + BinaryCache(BinaryCache&&) = default; ~BinaryCache(); /// Called upon a successful build of `action` to store those contents in the binary cache. @@ -231,9 +229,9 @@ namespace vcpkg void push_thread_main(); - BGMessageSink m_bg_msg_sink; - BGThreadBatchQueue m_actions_to_push; - std::atomic_int m_remaining_packages_to_push = 0; + std::unique_ptr m_bg_msg_sink; + std::unique_ptr> m_actions_to_push; + std::unique_ptr m_remaining_packages_to_push = 0; std::thread m_push_thread; }; diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index a4b1dccb15..566481654d 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -2115,39 +2115,43 @@ namespace vcpkg }); } - BinaryCache::BinaryCache(const Filesystem& fs) : m_fs(fs), m_bg_msg_sink(stdout_sink) { } - - ExpectedL> BinaryCache::make(const VcpkgCmdArguments& args, - const VcpkgPaths& paths, - MessageSink& sink) - { - return make_binary_providers(args, paths) - .then([&](BinaryProviders&& providers) -> ExpectedL> { - std::unique_ptr cache(new BinaryCache(std::move(providers), paths.get_filesystem())); - cache->m_needs_nuspec_data = - Util::any_of(cache->m_config.write, [](auto&& p) { return p->needs_nuspec_data(); }); - cache->m_needs_zip_file = - Util::any_of(cache->m_config.write, [](auto&& p) { return p->needs_zip_file(); }); - if (cache->m_needs_zip_file) - { - auto maybe_zip_tool = ZipTool::make(paths.get_tool_cache(), sink); - if (auto zip_tool = maybe_zip_tool.get()) - { - cache->m_zip_tool.emplace(std::move(*zip_tool)); - } - else - { - return std::move(maybe_zip_tool).error(); - } + BinaryCache::BinaryCache(const Filesystem& fs) + : m_fs(fs) + , m_bg_msg_sink(std::make_unique(stdout_sink)) + , m_actions_to_push(std::make_unique>()) + , m_remaining_packages_to_push(std::make_unique()) + { + } + + ExpectedL BinaryCache::make(const VcpkgCmdArguments& args, const VcpkgPaths& paths, MessageSink& sink) + { + return make_binary_providers(args, paths).then([&](BinaryProviders&& providers) -> ExpectedL { + BinaryCache cache(std::move(providers), paths.get_filesystem()); + cache.m_needs_nuspec_data = + Util::any_of(cache.m_config.write, [](auto&& p) { return p->needs_nuspec_data(); }); + cache.m_needs_zip_file = Util::any_of(cache.m_config.write, [](auto&& p) { return p->needs_zip_file(); }); + if (cache.m_needs_zip_file) + { + auto maybe_zip_tool = ZipTool::make(paths.get_tool_cache(), sink); + if (auto zip_tool = maybe_zip_tool.get()) + { + cache.m_zip_tool.emplace(std::move(*zip_tool)); } - return std::move(cache); - }); + else + { + return std::move(maybe_zip_tool).error(); + } + } + return std::move(cache); + }); } BinaryCache::BinaryCache(BinaryProviders&& providers, const Filesystem& fs) : ReadOnlyBinaryCache(std::move(providers)) , m_fs(fs) - , m_bg_msg_sink(stdout_sink) + , m_bg_msg_sink(std::make_unique(stdout_sink)) + , m_actions_to_push(std::make_unique>()) + , m_remaining_packages_to_push(std::make_unique()) , m_push_thread([this]() { push_thread_main(); }) { @@ -2174,8 +2178,8 @@ namespace vcpkg generate_nuspec(request.package_dir, action, m_config.nuget_prefix, m_config.nuget_repo); } - m_remaining_packages_to_push++; - m_actions_to_push.push(ActionToPush{std::move(request), action.build_options.clean_packages}); + (*m_remaining_packages_to_push)++; + m_actions_to_push->push(ActionToPush{std::move(request), action.build_options.clean_packages}); return; } } @@ -2185,18 +2189,18 @@ namespace vcpkg } } - void BinaryCache::print_push_success_messages() { m_bg_msg_sink.print_published(); } + void BinaryCache::print_push_success_messages() { m_bg_msg_sink->print_published(); } void BinaryCache::wait_for_async_complete_and_join() { bool have_remaining_packages = m_remaining_packages_to_push > 0; if (have_remaining_packages) { - m_bg_msg_sink.print_published(); - msg::println(msgWaitUntilPackagesUploaded, msg::count = m_remaining_packages_to_push.load()); + m_bg_msg_sink->print_published(); + msg::println(msgWaitUntilPackagesUploaded, msg::count = m_remaining_packages_to_push->load()); } - m_bg_msg_sink.publish_directly_to_out_sink(); - m_actions_to_push.stop(); + m_bg_msg_sink->publish_directly_to_out_sink(); + m_actions_to_push->stop(); if (m_push_thread.joinable()) { m_push_thread.join(); @@ -2209,7 +2213,7 @@ namespace vcpkg int count_pushed = 0; while (true) { - m_actions_to_push.wait_for_items(my_tasks); + m_actions_to_push->wait_for_items(my_tasks); if (my_tasks.empty()) { break; @@ -2229,7 +2233,7 @@ namespace vcpkg } else { - m_bg_msg_sink.println( + m_bg_msg_sink->println( Color::warning, msg::format_warning(msgCompressFolderFailed, msg::path = action_to_push.request.package_dir) .append_raw(' ') @@ -2249,17 +2253,17 @@ namespace vcpkg { m_fs.remove(*action_to_push.request.zip_path.get(), IgnoreErrors{}); } - m_bg_msg_sink.print( + m_bg_msg_sink->print( msgStoredBinariesToDestinations, msg::count = num_destinations, msg::elapsed = timer.elapsed()); - m_remaining_packages_to_push.fetch_sub(1); - if (m_actions_to_push.stopped()) + m_remaining_packages_to_push->fetch_sub(1); + if (m_actions_to_push->stopped()) { count_pushed++; - m_bg_msg_sink.print(LocalizedString::from_raw( - fmt::format(" ({}/{})", count_pushed, count_pushed + m_remaining_packages_to_push.load()))); + m_bg_msg_sink->print(LocalizedString::from_raw( + fmt::format(" ({}/{})", count_pushed, count_pushed + m_remaining_packages_to_push->load()))); } - m_bg_msg_sink.println(); + m_bg_msg_sink->println(); if (action_to_push.clean_after_push == CleanPackages::YES) { m_fs.remove_all(action_to_push.request.package_dir, VCPKG_LINE_INFO); diff --git a/src/vcpkg/commands.build.cpp b/src/vcpkg/commands.build.cpp index 13702228e0..e96b1c0f4e 100644 --- a/src/vcpkg/commands.build.cpp +++ b/src/vcpkg/commands.build.cpp @@ -192,7 +192,7 @@ namespace vcpkg msg::print(create_user_troubleshooting_message(*action, paths, nullopt)); return 1; } - binary_cache->push_success(*action); + binary_cache.push_success(*action); return 0; } diff --git a/src/vcpkg/commands.ci.cpp b/src/vcpkg/commands.ci.cpp index 006d92d6e5..9d439885ed 100644 --- a/src/vcpkg/commands.ci.cpp +++ b/src/vcpkg/commands.ci.cpp @@ -421,7 +421,7 @@ namespace vcpkg auto action_plan = compute_full_plan(paths, provider, var_provider, all_default_full_specs, serialize_options); auto binary_cache = BinaryCache::make(args, paths, stdout_sink).value_or_exit(VCPKG_LINE_INFO); - const auto precheck_results = binary_cache->precheck(action_plan.install_actions); + const auto precheck_results = binary_cache.precheck(action_plan.install_actions); auto split_specs = compute_action_statuses(ExclusionPredicate{&exclusions_map}, precheck_results, action_plan); LocalizedString regressions; { @@ -522,9 +522,9 @@ namespace vcpkg } install_preclear_packages(paths, action_plan); - binary_cache->fetch(action_plan.install_actions); + binary_cache.fetch(action_plan.install_actions); auto summary = install_execute_plan( - args, action_plan, KeepGoing::YES, paths, status_db, *binary_cache, build_logs_recorder); + args, action_plan, KeepGoing::YES, paths, status_db, binary_cache, build_logs_recorder); for (auto&& result : summary.results) { @@ -579,7 +579,7 @@ namespace vcpkg it_xunit->second, xunitTestResults.build_xml(target_triplet), VCPKG_LINE_INFO); } } - binary_cache->wait_for_async_complete_and_join(); + binary_cache.wait_for_async_complete_and_join(); Checks::exit_success(VCPKG_LINE_INFO); } } // namespace vcpkg diff --git a/src/vcpkg/commands.install.cpp b/src/vcpkg/commands.install.cpp index 30576b1b0c..ed97220559 100644 --- a/src/vcpkg/commands.install.cpp +++ b/src/vcpkg/commands.install.cpp @@ -1339,11 +1339,11 @@ namespace vcpkg track_install_plan(action_plan); install_preclear_packages(paths, action_plan); - auto binary_cache = only_downloads ? std::make_unique(paths.get_filesystem()) + auto binary_cache = only_downloads ? BinaryCache(paths.get_filesystem()) : BinaryCache::make(args, paths, stdout_sink).value_or_exit(VCPKG_LINE_INFO); - binary_cache->fetch(action_plan.install_actions); + binary_cache.fetch(action_plan.install_actions); const InstallSummary summary = install_execute_plan( - args, action_plan, keep_going, paths, status_db, *binary_cache, null_build_logs_recorder()); + args, action_plan, keep_going, paths, status_db, binary_cache, null_build_logs_recorder()); if (keep_going == KeepGoing::YES) { @@ -1381,7 +1381,7 @@ namespace vcpkg install_print_usage_information(*bpgh, printed_usages, fs, paths.installed()); } } - binary_cache->wait_for_async_complete_and_join(); + binary_cache.wait_for_async_complete_and_join(); Checks::exit_with_code(VCPKG_LINE_INFO, summary.failed()); } diff --git a/src/vcpkg/commands.set-installed.cpp b/src/vcpkg/commands.set-installed.cpp index d52d02bf31..fe004f1abc 100644 --- a/src/vcpkg/commands.set-installed.cpp +++ b/src/vcpkg/commands.set-installed.cpp @@ -240,18 +240,18 @@ namespace vcpkg track_install_plan(action_plan); install_preclear_packages(paths, action_plan); - auto binary_cache = only_downloads ? std::make_unique(paths.get_filesystem()) + auto binary_cache = only_downloads ? BinaryCache(paths.get_filesystem()) : BinaryCache::make(args, paths, stdout_sink).value_or_exit(VCPKG_LINE_INFO); - binary_cache->fetch(action_plan.install_actions); + binary_cache.fetch(action_plan.install_actions); const auto summary = install_execute_plan( - args, action_plan, keep_going, paths, status_db, *binary_cache, null_build_logs_recorder()); + args, action_plan, keep_going, paths, status_db, binary_cache, null_build_logs_recorder()); if (keep_going == KeepGoing::YES && summary.failed()) { summary.print_failed(); if (!only_downloads) { - binary_cache->wait_for_async_complete_and_join(); + binary_cache.wait_for_async_complete_and_join(); Checks::exit_fail(VCPKG_LINE_INFO); } } @@ -268,7 +268,7 @@ namespace vcpkg } } } - binary_cache->wait_for_async_complete_and_join(); + binary_cache.wait_for_async_complete_and_join(); Checks::exit_success(VCPKG_LINE_INFO); } diff --git a/src/vcpkg/commands.upgrade.cpp b/src/vcpkg/commands.upgrade.cpp index 8e4bf24e23..b617e21cf4 100644 --- a/src/vcpkg/commands.upgrade.cpp +++ b/src/vcpkg/commands.upgrade.cpp @@ -213,16 +213,16 @@ namespace vcpkg auto binary_cache = BinaryCache::make(args, paths, stdout_sink).value_or_exit(VCPKG_LINE_INFO); compute_all_abis(paths, action_plan, var_provider, status_db); - binary_cache->fetch(action_plan.install_actions); + binary_cache.fetch(action_plan.install_actions); const InstallSummary summary = install_execute_plan( - args, action_plan, keep_going, paths, status_db, *binary_cache, null_build_logs_recorder()); + args, action_plan, keep_going, paths, status_db, binary_cache, null_build_logs_recorder()); if (keep_going == KeepGoing::YES) { summary.print(); } - binary_cache->wait_for_async_complete_and_join(); + binary_cache.wait_for_async_complete_and_join(); Checks::exit_success(VCPKG_LINE_INFO); } } // namespace vcpkg From 814e434903b93a752dc03cd99f9d9c51a73c115d Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Tue, 14 Nov 2023 16:19:55 +0100 Subject: [PATCH 39/40] BinaryCache: save data in std::unique_ptr so that the object can be moved while the data is accessed from a thread --- include/vcpkg/base/batch-queue.h | 6 +- include/vcpkg/binarycaching.h | 38 ++++++----- src/vcpkg/binarycaching.cpp | 109 ++++++++++++++++--------------- 3 files changed, 84 insertions(+), 69 deletions(-) diff --git a/include/vcpkg/base/batch-queue.h b/include/vcpkg/base/batch-queue.h index 6026ad91f6..ef3e993ed8 100644 --- a/include/vcpkg/base/batch-queue.h +++ b/include/vcpkg/base/batch-queue.h @@ -51,7 +51,11 @@ struct BGThreadBatchQueue m_cv.notify_all(); } - bool stopped() const { return !m_running; } + bool stopped() + { + std::lock_guard lock(m_mtx); + return !m_running; + } private: std::mutex m_mtx; diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index 2f591f51bf..23a18b105a 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -194,9 +194,13 @@ namespace vcpkg std::vector precheck(View actions); protected: - BinaryProviders m_config; + struct ReadOnlyBinaryCacheData + { + BinaryProviders m_config; - std::unordered_map m_status; + std::unordered_map m_status; + }; + std::unique_ptr data; }; struct BinaryCache : ReadOnlyBinaryCache @@ -216,24 +220,28 @@ namespace vcpkg private: BinaryCache(BinaryProviders&& providers, const Filesystem& fs); - - const Filesystem& m_fs; - Optional m_zip_tool; - bool m_needs_nuspec_data = false; - bool m_needs_zip_file = false; - struct ActionToPush { BinaryPackageWriteInfo request; CleanPackages clean_after_push; }; - - void push_thread_main(); - - std::unique_ptr m_bg_msg_sink; - std::unique_ptr> m_actions_to_push; - std::unique_ptr m_remaining_packages_to_push = 0; - std::thread m_push_thread; + struct BinaryCacheData + { + BinaryCacheData(const Filesystem& fs, ReadOnlyBinaryCacheData* data); + const Filesystem& m_fs; + Optional m_zip_tool; + bool m_needs_nuspec_data = false; + bool m_needs_zip_file = false; + + BGMessageSink m_bg_msg_sink; + BGThreadBatchQueue m_actions_to_push; + std::atomic_int m_remaining_packages_to_push = 0; + std::thread m_push_thread; + + void push_thread_main(ReadOnlyBinaryCacheData* ro_data); + void wait_for_async_complete_and_join(); + }; + std::unique_ptr bc_data; }; ExpectedL parse_download_configuration(const Optional& arg); diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 1d6be63b8a..cc93185cbd 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -2009,14 +2009,17 @@ namespace vcpkg return std::move(ret); } - ReadOnlyBinaryCache::ReadOnlyBinaryCache(BinaryProviders&& providers) : m_config(std::move(providers)) { } + ReadOnlyBinaryCache::ReadOnlyBinaryCache(BinaryProviders&& providers) + : data(new ReadOnlyBinaryCacheData{std::move(providers)}) + { + } void ReadOnlyBinaryCache::fetch(View actions) { std::vector action_ptrs; std::vector restores; std::vector statuses; - for (auto&& provider : m_config.read) + for (auto&& provider : data->m_config.read) { action_ptrs.clear(); restores.clear(); @@ -2025,7 +2028,7 @@ namespace vcpkg { if (actions[i].package_abi()) { - CacheStatus& status = m_status[*actions[i].package_abi().get()]; + CacheStatus& status = data->m_status[*actions[i].package_abi().get()]; if (status.should_attempt_restore(provider.get())) { action_ptrs.push_back(&actions[i]); @@ -2060,8 +2063,8 @@ namespace vcpkg { if (auto abi = action.package_abi().get()) { - auto it = m_status.find(*abi); - if (it != m_status.end()) return it->second.is_restored(); + auto it = data->m_status.find(*abi); + if (it != data->m_status.end()) return it->second.is_restored(); } return false; } @@ -2070,13 +2073,13 @@ namespace vcpkg { std::vector statuses = Util::fmap(actions, [this](const auto& action) { if (!action.package_abi()) Checks::unreachable(VCPKG_LINE_INFO); - return &m_status[*action.package_abi().get()]; + return &data->m_status[*action.package_abi().get()]; }); std::vector action_ptrs; std::vector cache_result; std::vector indexes; - for (auto&& provider : m_config.read) + for (auto&& provider : data->m_config.read) { action_ptrs.clear(); cache_result.clear(); @@ -2096,7 +2099,7 @@ namespace vcpkg for (size_t i = 0; i < action_ptrs.size(); ++i) { - auto&& this_status = m_status[*action_ptrs[i]->package_abi().get()]; + auto&& this_status = data->m_status[*action_ptrs[i]->package_abi().get()]; if (cache_result[i] == CacheAvailability::available) { this_status.mark_available(provider.get()); @@ -2113,27 +2116,22 @@ namespace vcpkg }); } - BinaryCache::BinaryCache(const Filesystem& fs) - : m_fs(fs) - , m_bg_msg_sink(std::make_unique(stdout_sink)) - , m_actions_to_push(std::make_unique>()) - , m_remaining_packages_to_push(std::make_unique()) - { - } + BinaryCache::BinaryCache(const Filesystem& fs) : bc_data(std::make_unique(fs, data.get())) { } ExpectedL BinaryCache::make(const VcpkgCmdArguments& args, const VcpkgPaths& paths, MessageSink& sink) { return make_binary_providers(args, paths).then([&](BinaryProviders&& providers) -> ExpectedL { BinaryCache cache(std::move(providers), paths.get_filesystem()); - cache.m_needs_nuspec_data = - Util::any_of(cache.m_config.write, [](auto&& p) { return p->needs_nuspec_data(); }); - cache.m_needs_zip_file = Util::any_of(cache.m_config.write, [](auto&& p) { return p->needs_zip_file(); }); - if (cache.m_needs_zip_file) + cache.bc_data->m_needs_nuspec_data = + Util::any_of(cache.data->m_config.write, [](auto&& p) { return p->needs_nuspec_data(); }); + cache.bc_data->m_needs_zip_file = + Util::any_of(cache.data->m_config.write, [](auto&& p) { return p->needs_zip_file(); }); + if (cache.bc_data->m_needs_zip_file) { auto maybe_zip_tool = ZipTool::make(paths.get_tool_cache(), sink); if (auto zip_tool = maybe_zip_tool.get()) { - cache.m_zip_tool.emplace(std::move(*zip_tool)); + cache.bc_data->m_zip_tool.emplace(std::move(*zip_tool)); } else { @@ -2145,73 +2143,78 @@ namespace vcpkg } BinaryCache::BinaryCache(BinaryProviders&& providers, const Filesystem& fs) - : ReadOnlyBinaryCache(std::move(providers)) - , m_fs(fs) - , m_bg_msg_sink(std::make_unique(stdout_sink)) - , m_actions_to_push(std::make_unique>()) - , m_remaining_packages_to_push(std::make_unique()) - , m_push_thread([this]() { push_thread_main(); }) - + : ReadOnlyBinaryCache(std::move(providers)), bc_data(std::make_unique(fs, data.get())) { } - BinaryCache::~BinaryCache() { wait_for_async_complete_and_join(); } + BinaryCache::BinaryCacheData::BinaryCacheData(const Filesystem& fs, ReadOnlyBinaryCacheData* data) + : m_fs(fs), m_bg_msg_sink(stdout_sink), m_push_thread([this, data]() { push_thread_main(data); }) + { + } + BinaryCache::~BinaryCache() + { + if (bc_data) + { + wait_for_async_complete_and_join(); + } + } void BinaryCache::push_success(const InstallPlanAction& action) { if (auto abi = action.package_abi().get()) { - bool restored = m_status[*abi].is_restored(); + bool restored = data->m_status[*abi].is_restored(); // Purge all status information on push_success (cache invalidation) // - push_success may delete packages/ (invalidate restore) // - push_success may make the package available from providers (invalidate unavailable) - m_status.erase(*abi); - if (!restored && !m_config.write.empty()) + data->m_status.erase(*abi); + if (!restored && !data->m_config.write.empty()) { ElapsedTimer timer; BinaryPackageWriteInfo request{action}; - if (m_needs_nuspec_data) + if (bc_data->m_needs_nuspec_data) { - request.nuspec = - generate_nuspec(request.package_dir, action, m_config.nuget_prefix, m_config.nuget_repo); + request.nuspec = generate_nuspec( + request.package_dir, action, data->m_config.nuget_prefix, data->m_config.nuget_repo); } - (*m_remaining_packages_to_push)++; - m_actions_to_push->push(ActionToPush{std::move(request), action.build_options.clean_packages}); + bc_data->m_remaining_packages_to_push++; + bc_data->m_actions_to_push.push(ActionToPush{std::move(request), action.build_options.clean_packages}); return; } } if (action.build_options.clean_packages == CleanPackages::YES) { - m_fs.remove_all(action.package_dir.value_or_exit(VCPKG_LINE_INFO), VCPKG_LINE_INFO); + bc_data->m_fs.remove_all(action.package_dir.value_or_exit(VCPKG_LINE_INFO), VCPKG_LINE_INFO); } } - void BinaryCache::print_push_success_messages() { m_bg_msg_sink->print_published(); } + void BinaryCache::print_push_success_messages() { bc_data->m_bg_msg_sink.print_published(); } + void BinaryCache::wait_for_async_complete_and_join() { bc_data->wait_for_async_complete_and_join(); } - void BinaryCache::wait_for_async_complete_and_join() + void BinaryCache::BinaryCacheData::wait_for_async_complete_and_join() { bool have_remaining_packages = m_remaining_packages_to_push > 0; if (have_remaining_packages) { - m_bg_msg_sink->print_published(); - msg::println(msgWaitUntilPackagesUploaded, msg::count = m_remaining_packages_to_push->load()); + m_bg_msg_sink.print_published(); + msg::println(msgWaitUntilPackagesUploaded, msg::count = m_remaining_packages_to_push.load()); } - m_bg_msg_sink->publish_directly_to_out_sink(); - m_actions_to_push->stop(); + m_bg_msg_sink.publish_directly_to_out_sink(); + m_actions_to_push.stop(); if (m_push_thread.joinable()) { m_push_thread.join(); } } - void BinaryCache::push_thread_main() + void BinaryCache::BinaryCacheData::push_thread_main(ReadOnlyBinaryCacheData* ro_data) { std::vector my_tasks; int count_pushed = 0; while (true) { - m_actions_to_push->wait_for_items(my_tasks); + m_actions_to_push.wait_for_items(my_tasks); if (my_tasks.empty()) { break; @@ -2231,7 +2234,7 @@ namespace vcpkg } else { - m_bg_msg_sink->println( + m_bg_msg_sink.println( Color::warning, msg::format_warning(msgCompressFolderFailed, msg::path = action_to_push.request.package_dir) .append_raw(' ') @@ -2240,7 +2243,7 @@ namespace vcpkg } size_t num_destinations = 0; - for (auto&& provider : m_config.write) + for (auto&& provider : ro_data->m_config.write) { if (!provider->needs_zip_file() || action_to_push.request.zip_path.has_value()) { @@ -2251,17 +2254,17 @@ namespace vcpkg { m_fs.remove(*action_to_push.request.zip_path.get(), IgnoreErrors{}); } - m_bg_msg_sink->print( + m_bg_msg_sink.print( msgStoredBinariesToDestinations, msg::count = num_destinations, msg::elapsed = timer.elapsed()); - m_remaining_packages_to_push->fetch_sub(1); - if (m_actions_to_push->stopped()) + m_remaining_packages_to_push.fetch_sub(1); + if (m_actions_to_push.stopped()) { count_pushed++; - m_bg_msg_sink->print(LocalizedString::from_raw( - fmt::format(" ({}/{})", count_pushed, count_pushed + m_remaining_packages_to_push->load()))); + m_bg_msg_sink.print(LocalizedString::from_raw( + fmt::format(" ({}/{})", count_pushed, count_pushed + m_remaining_packages_to_push.load()))); } - m_bg_msg_sink->println(); + m_bg_msg_sink.println(); if (action_to_push.clean_after_push == CleanPackages::YES) { m_fs.remove_all(action_to_push.request.package_dir, VCPKG_LINE_INFO); From 290e58615de89eccc4a753908f9b519f66e5b03f Mon Sep 17 00:00:00 2001 From: Leander Schulten Date: Tue, 14 Nov 2023 16:51:55 +0100 Subject: [PATCH 40/40] fix --- include/vcpkg/binarycaching.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/include/vcpkg/binarycaching.h b/include/vcpkg/binarycaching.h index 23a18b105a..f5ec028bb9 100644 --- a/include/vcpkg/binarycaching.h +++ b/include/vcpkg/binarycaching.h @@ -200,9 +200,18 @@ namespace vcpkg std::unordered_map m_status; }; - std::unique_ptr data; + std::unique_ptr data = std::make_unique(); }; + // compression and upload of binary cache entries happens on a single 'background' thread, `m_push_thread` + // Thread safety is achieved within the binary cache providers by: + // 1. Only using one thread in the background for this work. + // 2. Forming a queue of work for that thread to consume in `m_actions_to_push`, which maintains its own thread + // safety + // 3. Sending any replies from the background thread through `m_bg_msg_sink` + // 4. Ensuring any supporting data, such as tool exes, is provided before the background thread is started. + // 5. Ensuring that work is not submitted to the background thread until the corresponding `packages` directory to + // upload is no longer being actively touched by the foreground thread. struct BinaryCache : ReadOnlyBinaryCache { static ExpectedL make(const VcpkgCmdArguments& args, const VcpkgPaths& paths, MessageSink& sink);