From 6f3b762ceec9e37a1afc77c8289b0cf5ce31e995 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tam=C3=A1s=20PEREGI?= Date: Thu, 9 May 2024 17:20:00 +0200 Subject: [PATCH 1/3] Enable differentiating cache misses from errors This commit does not change any behavior, just prepares the way for the next commit. --- src/vcpkg/binarycaching.cpp | 87 +++++++++++++++++++++++++++---------- 1 file changed, 64 insertions(+), 23 deletions(-) diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index ab1b2b2420..c7164567eb 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -951,14 +951,37 @@ namespace static constexpr StringLiteral m_accept_header = "Accept: application/json;api-version=6.0-preview.1"; }; + template + static ExpectedL flatten_generic(const ExpectedL& maybe_exit, + StringView tool_name, + ResultOnSuccessType result_on_success) + { + if (auto exit = maybe_exit.get()) + { + if (exit->exit_code == 0) + { + return {result_on_success}; + } + + return {msg::format_error( + msgProgramReturnedNonzeroExitCode, msg::tool_name = tool_name, msg::exit_code = exit->exit_code) + .append_raw('\n') + .append_raw(exit->output)}; + } + + return {msg::format_error(msgLaunchingProgramFailed, msg::tool_name = tool_name) + .append_raw(' ') + .append_raw(maybe_exit.error().to_string())}; + } + struct IObjectStorageTool { virtual ~IObjectStorageTool() = default; virtual LocalizedString restored_message(size_t count, std::chrono::high_resolution_clock::duration elapsed) const = 0; - virtual ExpectedL stat(StringView url) const = 0; - virtual ExpectedL download_file(StringView object, const Path& archive) const = 0; + virtual ExpectedL stat(StringView url) const = 0; + virtual ExpectedL download_file(StringView object, const Path& archive) const = 0; virtual ExpectedL upload_file(StringView object, const Path& archive) const = 0; }; @@ -990,9 +1013,12 @@ namespace const auto& abi = action.package_abi().value_or_exit(VCPKG_LINE_INFO); auto tmp = make_temp_archive_path(m_buildtrees, action.spec); auto res = m_tool->download_file(make_object_path(m_prefix, abi), tmp); - if (res) + if (auto cache_result = res.get()) { - out_zip_paths[idx].emplace(std::move(tmp), RemoveWhen::always); + if (*cache_result == RestoreResult::restored) + { + out_zip_paths[idx].emplace(std::move(tmp), RemoveWhen::always); + } } else { @@ -1007,9 +1033,10 @@ namespace { auto&& action = *actions[idx]; const auto& abi = action.package_abi().value_or_exit(VCPKG_LINE_INFO); - if (m_tool->stat(make_object_path(m_prefix, abi))) + auto maybe_res = m_tool->stat(make_object_path(m_prefix, abi)); + if (auto res = maybe_res.get()) { - cache_status[idx] = CacheAvailability::available; + cache_status[idx] = *res; } else { @@ -1077,19 +1104,21 @@ namespace return msg::format(msgRestoredPackagesFromGCS, msg::count = count, msg::elapsed = ElapsedTime(elapsed)); } - ExpectedL stat(StringView url) const override + ExpectedL stat(StringView url) const override { - return flatten( + return flatten_generic( cmd_execute_and_capture_output(Command{m_tool}.string_arg("-q").string_arg("stat").string_arg(url)), - Tools::GSUTIL); + Tools::GSUTIL, + CacheAvailability::available); } - ExpectedL download_file(StringView object, const Path& archive) const override + ExpectedL download_file(StringView object, const Path& archive) const override { - return flatten( + return flatten_generic( cmd_execute_and_capture_output( Command{m_tool}.string_arg("-q").string_arg("cp").string_arg(object).string_arg(archive)), - Tools::GSUTIL); + Tools::GSUTIL, + RestoreResult::restored); } ExpectedL upload_file(StringView object, const Path& archive) const override @@ -1116,7 +1145,7 @@ namespace return msg::format(msgRestoredPackagesFromAWS, msg::count = count, msg::elapsed = ElapsedTime(elapsed)); } - ExpectedL stat(StringView url) const override + ExpectedL stat(StringView url) const override { auto cmd = Command{m_tool}.string_arg("s3").string_arg("ls").string_arg(url); if (m_no_sign_request) @@ -1124,13 +1153,23 @@ namespace cmd.string_arg("--no-sign-request"); } - return flatten(cmd_execute_and_capture_output(cmd), Tools::AWSCLI); + return flatten_generic(cmd_execute_and_capture_output(cmd), Tools::AWSCLI, CacheAvailability::available); } - ExpectedL download_file(StringView object, const Path& archive) const override + ExpectedL download_file(StringView object, const Path& archive) const override { auto r = stat(object); - if (!r) return r; + if (auto stat_result = r.get()) + { + if (*stat_result != CacheAvailability::available) + { + return RestoreResult::unavailable; + } + } + else + { + return r.error(); + } auto cmd = Command{m_tool}.string_arg("s3").string_arg("cp").string_arg(object).string_arg(archive); if (m_no_sign_request) @@ -1138,7 +1177,7 @@ namespace cmd.string_arg("--no-sign-request"); } - return flatten(cmd_execute_and_capture_output(cmd), Tools::AWSCLI); + return flatten_generic(cmd_execute_and_capture_output(cmd), Tools::AWSCLI, RestoreResult::restored); } ExpectedL upload_file(StringView object, const Path& archive) const override @@ -1165,17 +1204,19 @@ namespace return msg::format(msgRestoredPackagesFromCOS, msg::count = count, msg::elapsed = ElapsedTime(elapsed)); } - ExpectedL stat(StringView url) const override + ExpectedL stat(StringView url) const override { - return flatten(cmd_execute_and_capture_output(Command{m_tool}.string_arg("ls").string_arg(url)), - Tools::COSCLI); + return flatten_generic(cmd_execute_and_capture_output(Command{m_tool}.string_arg("ls").string_arg(url)), + Tools::COSCLI, + CacheAvailability::available); } - ExpectedL download_file(StringView object, const Path& archive) const override + ExpectedL download_file(StringView object, const Path& archive) const override { - return flatten( + return flatten_generic( cmd_execute_and_capture_output(Command{m_tool}.string_arg("cp").string_arg(object).string_arg(archive)), - Tools::COSCLI); + Tools::COSCLI, + RestoreResult::restored); } ExpectedL upload_file(StringView object, const Path& archive) const override From 80dcf3f73a3348a541faef538e5f4a1a9748a700 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tam=C3=A1s=20PEREGI?= Date: Thu, 9 May 2024 17:20:42 +0200 Subject: [PATCH 2/3] AWS S3 binary cache miss should not issue warning --- src/vcpkg/binarycaching.cpp | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index c7164567eb..5a4f424108 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -1153,7 +1153,28 @@ namespace cmd.string_arg("--no-sign-request"); } - return flatten_generic(cmd_execute_and_capture_output(cmd), Tools::AWSCLI, CacheAvailability::available); + auto maybe_exit = cmd_execute_and_capture_output(cmd); + + // When the file is not found, "aws s3 ls" prints nothing, and returns exit code 1. + // flatten_generic() would treat this as an error, but we want to treat it as a (silent) cache miss instead, + // so we handle this special case before calling flatten_generic(). + // See https://github.com/aws/aws-cli/issues/5544 for the related aws-cli bug report. + if (auto exit = maybe_exit.get()) + { + // We want to return CacheAvailability::unavailable even if aws-cli starts to return exit code 0 with an + // empty output when the file is missing. This way, both the current and possible future behavior of + // aws-cli is covered. + if (exit->exit_code == 0 || exit->exit_code == 1) + { + if (Strings::trim(exit->output) == "") + { + return CacheAvailability::unavailable; + } + } + } + + // In the non-special case, simply let flatten_generic() do its job. + return flatten_generic(maybe_exit, Tools::AWSCLI, CacheAvailability::available); } ExpectedL download_file(StringView object, const Path& archive) const override From 9353f161a21f6fe5ca25170992d8f6a63878975c Mon Sep 17 00:00:00 2001 From: Billy Robert O'Neal III Date: Thu, 6 Jun 2024 17:53:56 -0700 Subject: [PATCH 3/3] empty nitpick --- src/vcpkg/binarycaching.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vcpkg/binarycaching.cpp b/src/vcpkg/binarycaching.cpp index 5a4f424108..aa83bc5bed 100644 --- a/src/vcpkg/binarycaching.cpp +++ b/src/vcpkg/binarycaching.cpp @@ -1166,7 +1166,7 @@ namespace // aws-cli is covered. if (exit->exit_code == 0 || exit->exit_code == 1) { - if (Strings::trim(exit->output) == "") + if (Strings::trim(exit->output).empty()) { return CacheAvailability::unavailable; }