diff options
Diffstat (limited to 'src')
9 files changed, 180 insertions, 68 deletions
diff --git a/src/buildtool/execution_api/common/TARGETS b/src/buildtool/execution_api/common/TARGETS index b417e26a..0f3288e5 100644 --- a/src/buildtool/execution_api/common/TARGETS +++ b/src/buildtool/execution_api/common/TARGETS @@ -21,6 +21,7 @@ , ["src/buildtool/file_system", "file_system_manager"] , ["src/buildtool/logging", "log_level"] , ["src/buildtool/logging", "logging"] + , ["src/utils/cpp", "expected"] , ["src/utils/cpp", "gsl"] , ["src/utils/cpp", "hex_string"] , ["src/buildtool/file_system", "git_repo"] diff --git a/src/buildtool/execution_api/common/execution_response.hpp b/src/buildtool/execution_api/common/execution_response.hpp index ca1f34a1..c0aa16f8 100644 --- a/src/buildtool/execution_api/common/execution_response.hpp +++ b/src/buildtool/execution_api/common/execution_response.hpp @@ -23,6 +23,7 @@ #include "gsl/gsl" #include "src/buildtool/common/artifact.hpp" +#include "src/utils/cpp/expected.hpp" /// \brief Abstract response. /// Response of an action execution. Contains outputs from multiple commands and @@ -60,9 +61,10 @@ class IExecutionResponse { [[nodiscard]] virtual auto ActionDigest() const noexcept -> std::string const& = 0; - [[nodiscard]] virtual auto Artifacts() noexcept -> ArtifactInfos const& = 0; + [[nodiscard]] virtual auto Artifacts() noexcept + -> expected<gsl::not_null<ArtifactInfos const*>, std::string> = 0; [[nodiscard]] virtual auto DirectorySymlinks() noexcept - -> DirSymlinks const& = 0; + -> expected<gsl::not_null<DirSymlinks const*>, std::string> = 0; }; #endif // INCLUDED_SRC_BUILDTOOL_EXECUTION_API_COMMON_REMOTE_EXECUTION_RESPONSE_HPP diff --git a/src/buildtool/execution_api/execution_service/execution_server.cpp b/src/buildtool/execution_api/execution_service/execution_server.cpp index 61fd3434..d59eb91f 100644 --- a/src/buildtool/execution_api/execution_service/execution_server.cpp +++ b/src/buildtool/execution_api/execution_service/execution_server.cpp @@ -94,8 +94,16 @@ auto ExecutionServiceImpl::ToIExecutionAction( auto ExecutionServiceImpl::ToBazelExecuteResponse( IExecutionResponse::Ptr const& i_execution_response) const noexcept -> expected<::bazel_re::ExecuteResponse, std::string> { - auto result = ToBazelActionResult(i_execution_response->Artifacts(), - i_execution_response->DirectorySymlinks(), + auto artifacts = i_execution_response->Artifacts(); + if (not artifacts) { + return unexpected{std::move(artifacts).error()}; + } + auto dir_symlinks = i_execution_response->DirectorySymlinks(); + if (not dir_symlinks) { + return unexpected{std::move(dir_symlinks).error()}; + } + auto result = ToBazelActionResult(*std::move(artifacts).value(), + *std::move(dir_symlinks).value(), storage_); if (not result) { return unexpected{std::move(result).error()}; diff --git a/src/buildtool/execution_api/local/TARGETS b/src/buildtool/execution_api/local/TARGETS index 66a57f21..9bf9de97 100644 --- a/src/buildtool/execution_api/local/TARGETS +++ b/src/buildtool/execution_api/local/TARGETS @@ -43,6 +43,7 @@ , ["src/buildtool/logging", "logging"] , ["src/buildtool/execution_api/execution_service", "cas_utils"] , ["src/buildtool/file_system", "git_repo"] + , ["src/utils/cpp", "expected"] , ["src/utils/cpp", "tmp_dir"] , ["src/buildtool/crypto", "hash_function"] ] diff --git a/src/buildtool/execution_api/local/local_response.hpp b/src/buildtool/execution_api/local/local_response.hpp index 3cc04361..141ffbe3 100644 --- a/src/buildtool/execution_api/local/local_response.hpp +++ b/src/buildtool/execution_api/local/local_response.hpp @@ -16,9 +16,11 @@ #define INCLUDED_SRC_BUILDTOOL_EXECUTION_API_LOCAL_LOCAL_RESPONSE_HPP #include <cstddef> +#include <optional> #include <string> #include <utility> +#include "fmt/core.h" #include "gsl/gsl" #include "src/buildtool/common/artifact_digest_factory.hpp" #include "src/buildtool/crypto/hash_function.hpp" @@ -28,6 +30,7 @@ #include "src/buildtool/logging/log_level.hpp" #include "src/buildtool/logging/logger.hpp" #include "src/buildtool/storage/storage.hpp" +#include "src/utils/cpp/expected.hpp" /// \brief Response of a LocalAction. class LocalResponse final : public IExecutionResponse { @@ -66,14 +69,22 @@ class LocalResponse final : public IExecutionResponse { return action_id_; } - auto Artifacts() noexcept -> ArtifactInfos const& final { - Populate(); - return artifacts_; + auto Artifacts() noexcept + -> expected<gsl::not_null<ArtifactInfos const*>, std::string> final { + if (auto error_msg = Populate()) { + return unexpected{*std::move(error_msg)}; + } + return gsl::not_null<ArtifactInfos const*>( + &artifacts_); // explicit type needed for expected } - auto DirectorySymlinks() noexcept -> DirSymlinks const& final { - Populate(); - return dir_symlinks_; + auto DirectorySymlinks() noexcept + -> expected<gsl::not_null<DirSymlinks const*>, std::string> final { + if (auto error_msg = Populate()) { + return unexpected{*std::move(error_msg)}; + } + return gsl::not_null<DirSymlinks const*>( + &dir_symlinks_); // explicit type needed for expected } private: @@ -92,10 +103,12 @@ class LocalResponse final : public IExecutionResponse { output_{std::move(output)}, storage_{*storage} {} - void Populate() noexcept { + /// \brief Populates the stored data, once. + /// \returns Error message on failure, nullopt on success. + [[nodiscard]] auto Populate() noexcept -> std::optional<std::string> { // Initialized only once lazily if (populated_) { - return; + return std::nullopt; } populated_ = true; @@ -119,7 +132,9 @@ class LocalResponse final : public IExecutionResponse { auto digest = ArtifactDigestFactory::FromBazel(hash_type, file.digest()); if (not digest) { - return; + return fmt::format( + "LocalResponse: failed to create artifact digest for {}", + file.path()); } try { artifacts.emplace( @@ -128,8 +143,12 @@ class LocalResponse final : public IExecutionResponse { .type = file.is_executable() ? ObjectType::Executable : ObjectType::File}); - } catch (...) { - return; + } catch (std::exception const& ex) { + return fmt::format( + "LocalResponse: unexpected failure gathering digest for " + "{}:\n{}", + file.path(), + ex.what()); } } @@ -143,8 +162,12 @@ class LocalResponse final : public IExecutionResponse { ArtifactDigestFactory::HashDataAs<ObjectType::File>( storage_.GetHashFunction(), link.target()), .type = ObjectType::Symlink}); - } catch (...) { - return; + } catch (std::exception const& ex) { + return fmt::format( + "LocalResponse: unexpected failure gathering digest for " + "{}:\n{}", + link.path(), + ex.what()); } } for (auto const& link : action_result.output_directory_symlinks()) { @@ -157,8 +180,12 @@ class LocalResponse final : public IExecutionResponse { storage_.GetHashFunction(), link.target()), .type = ObjectType::Symlink}); dir_symlinks.emplace(link.path()); // add it to set - } catch (...) { - return; + } catch (std::exception const& ex) { + return fmt::format( + "LocalResponse: unexpected failure gathering digest for " + "{}:\n{}", + link.path(), + ex.what()); } } @@ -167,19 +194,26 @@ class LocalResponse final : public IExecutionResponse { auto digest = ArtifactDigestFactory::FromBazel(hash_type, dir.tree_digest()); if (not digest) { - return; + return fmt::format( + "LocalResponse: failed to create artifact digest for {}", + dir.path()); } try { artifacts.emplace( dir.path(), Artifact::ObjectInfo{.digest = *std::move(digest), .type = ObjectType::Tree}); - } catch (...) { - return; + } catch (std::exception const& ex) { + return fmt::format( + "LocalResponse: unexpected failure gathering digest for " + "{}:\n{}", + dir.path(), + ex.what()); } } artifacts_ = std::move(artifacts); dir_symlinks_ = std::move(dir_symlinks); + return std::nullopt; } [[nodiscard]] auto ReadContent(bazel_re::Digest const& digest) diff --git a/src/buildtool/execution_api/remote/TARGETS b/src/buildtool/execution_api/remote/TARGETS index ae67146d..ee1de5cb 100644 --- a/src/buildtool/execution_api/remote/TARGETS +++ b/src/buildtool/execution_api/remote/TARGETS @@ -34,6 +34,7 @@ , ["src/buildtool/execution_api/bazel_msg", "bazel_msg_factory"] , ["src/buildtool/auth", "auth"] , ["src/buildtool/execution_api/common", "bytestream-common"] + , ["src/utils/cpp", "expected"] , ["src/utils/cpp", "gsl"] , ["src/buildtool/common/remote", "client_common"] , ["src/buildtool/common/remote", "port"] @@ -48,7 +49,8 @@ ] , "stage": ["src", "buildtool", "execution_api", "remote"] , "private-deps": - [ ["src/buildtool/common", "artifact_digest_factory"] + [ ["@", "fmt", "", "fmt"] + , ["src/buildtool/common", "artifact_digest_factory"] , ["src/buildtool/common", "bazel_digest_factory"] , ["src/buildtool/common", "protocol_traits"] , ["src/buildtool/file_system", "file_system_manager"] diff --git a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp index c536f5dd..468a2531 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp @@ -17,7 +17,7 @@ #include <cstddef> #include <functional> -#include "gsl/gsl" +#include "fmt/core.h" #include "src/buildtool/common/artifact_digest.hpp" #include "src/buildtool/common/artifact_digest_factory.hpp" #include "src/buildtool/common/bazel_digest_factory.hpp" @@ -56,20 +56,28 @@ auto BazelResponse::ReadStringBlob(bazel_re::Digest const& id) noexcept return std::string{}; } -auto BazelResponse::Artifacts() noexcept -> ArtifactInfos const& { - Populate(); - return artifacts_; +auto BazelResponse::Artifacts() noexcept + -> expected<gsl::not_null<ArtifactInfos const*>, std::string> { + if (auto error_msg = Populate()) { + return unexpected{*std::move(error_msg)}; + } + return gsl::not_null<ArtifactInfos const*>( + &artifacts_); // explicit type needed for expected } -auto BazelResponse::DirectorySymlinks() noexcept -> DirSymlinks const& { - Populate(); - return dir_symlinks_; +auto BazelResponse::DirectorySymlinks() noexcept + -> expected<gsl::not_null<DirSymlinks const*>, std::string> { + if (auto error_msg = Populate()) { + return unexpected{*std::move(error_msg)}; + } + return gsl::not_null<DirSymlinks const*>( + &dir_symlinks_); // explicit type needed for expected } -void BazelResponse::Populate() noexcept { +auto BazelResponse::Populate() noexcept -> std::optional<std::string> { // Initialized only once lazily if (populated_) { - return; + return std::nullopt; } populated_ = true; @@ -92,7 +100,9 @@ void BazelResponse::Populate() noexcept { auto digest = ArtifactDigestFactory::FromBazel(hash_type, file.digest()); if (not digest) { - return; + return fmt::format( + "BazelResponse: failed to create artifact digest for {}", + file.path()); } try { artifacts.emplace( @@ -101,8 +111,12 @@ void BazelResponse::Populate() noexcept { .type = file.is_executable() ? ObjectType::Executable : ObjectType::File}); - } catch (...) { - return; + } catch (std::exception const& ex) { + return fmt::format( + "BazelResponse: unexpected failure gathering digest for " + "{}:\n{}", + file.path(), + ex.what()); } } @@ -116,8 +130,12 @@ void BazelResponse::Populate() noexcept { ArtifactDigestFactory::HashDataAs<ObjectType::File>( network_->GetHashFunction(), link.target()), .type = ObjectType::Symlink}); - } catch (...) { - return; + } catch (std::exception const& ex) { + return fmt::format( + "BazelResponse: unexpected failure gathering digest for " + "{}:\n{}", + link.path(), + ex.what()); } } for (auto const& link : action_result.output_directory_symlinks()) { @@ -130,8 +148,12 @@ void BazelResponse::Populate() noexcept { network_->GetHashFunction(), link.target()), .type = ObjectType::Symlink}); dir_symlinks.emplace(link.path()); // add it to set - } catch (...) { - return; + } catch (std::exception const& ex) { + return fmt::format( + "BazelResponse: unexpected failure gathering digest for " + "{}:\n{}", + link.path(), + ex.what()); } } @@ -141,7 +163,9 @@ void BazelResponse::Populate() noexcept { auto digest = ArtifactDigestFactory::FromBazel(hash_type, tree.tree_digest()); if (not digest) { - return; + return fmt::format( + "BazelResponse: failed to create artifact digest for {}", + tree.path()); } ExpectsAudit(digest->IsTree()); try { @@ -149,13 +173,17 @@ void BazelResponse::Populate() noexcept { tree.path(), Artifact::ObjectInfo{.digest = *std::move(digest), .type = ObjectType::Tree}); - } catch (...) { - return; + } catch (std::exception const& ex) { + return fmt::format( + "BazelResponse: unexpected failure gathering digest for " + "{}:\n{}", + tree.path(), + ex.what()); } } artifacts_ = std::move(artifacts); dir_symlinks_ = std::move(dir_symlinks); - return; + return std::nullopt; } // obtain tree digests for output directories @@ -176,7 +204,9 @@ void BazelResponse::Populate() noexcept { auto tree = BazelMsgFactory::MessageFromString<bazel_re::Tree>( *tree_blob.data); if (not tree) { - return; + return fmt::format( + "BazelResponse: failed to create Tree for {}", + tree_blob.digest.hash()); } // The server does not store the Directory messages it just @@ -185,22 +215,30 @@ void BazelResponse::Populate() noexcept { // have to upload them manually. auto root_digest = UploadTreeMessageDirectories(*tree); if (not root_digest) { - Logger::Log(LogLevel::Error, - "uploading Tree's Directory messages failed"); - return; + auto const error = fmt::format( + "BazelResponse: failure in uploading Tree Directory " + "message for {}", + tree_blob.digest.hash()); + Logger::Log(LogLevel::Trace, error); + return error; } artifacts.emplace( action_result.output_directories(pos).path(), Artifact::ObjectInfo{.digest = *root_digest, .type = ObjectType::Tree}); - } catch (...) { - return; + } catch (std::exception const& ex) { + return fmt::format( + "BazelResponse: unexpected failure gathering digest for " + "{}:\n{}", + tree_blob.digest.hash(), + ex.what()); } ++pos; } } artifacts_ = std::move(artifacts); dir_symlinks_ = std::move(dir_symlinks); + return std::nullopt; } auto BazelResponse::UploadTreeMessageDirectories( diff --git a/src/buildtool/execution_api/remote/bazel/bazel_response.hpp b/src/buildtool/execution_api/remote/bazel/bazel_response.hpp index 6e37d344..ec3e9748 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_response.hpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_response.hpp @@ -20,9 +20,11 @@ #include <utility> // std::move #include <vector> +#include "gsl/gsl" #include "src/buildtool/execution_api/common/execution_api.hpp" #include "src/buildtool/execution_api/remote/bazel/bazel_execution_client.hpp" #include "src/buildtool/execution_api/remote/bazel/bazel_network.hpp" +#include "src/utils/cpp/expected.hpp" class BazelAction; @@ -58,8 +60,10 @@ class BazelResponse final : public IExecutionResponse { return action_id_; } - auto Artifacts() noexcept -> ArtifactInfos const& final; - auto DirectorySymlinks() noexcept -> DirSymlinks const& final; + auto Artifacts() noexcept + -> expected<gsl::not_null<ArtifactInfos const*>, std::string> final; + auto DirectorySymlinks() noexcept + -> expected<gsl::not_null<DirSymlinks const*>, std::string> final; private: std::string action_id_{}; @@ -84,7 +88,9 @@ class BazelResponse final : public IExecutionResponse { return id.size_bytes() != 0; } - void Populate() noexcept; + /// \brief Populates the stored data, once. + /// \returns Error message on failure, nullopt on success. + [[nodiscard]] auto Populate() noexcept -> std::optional<std::string>; [[nodiscard]] auto UploadTreeMessageDirectories( bazel_re::Tree const& tree) const -> std::optional<ArtifactDigest>; diff --git a/src/buildtool/execution_engine/executor/executor.hpp b/src/buildtool/execution_engine/executor/executor.hpp index 4b2548da..d0bc7aee 100644 --- a/src/buildtool/execution_engine/executor/executor.hpp +++ b/src/buildtool/execution_engine/executor/executor.hpp @@ -154,10 +154,14 @@ class ExecutorImpl { auto result = remote_action->Execute(&logger); if (alternative_api) { if (result) { - auto const& artifacts = result->Artifacts(); + auto const artifacts = result->Artifacts(); + if (not artifacts) { + logger.Emit(LogLevel::Error, artifacts.error()); + return nullptr; + } std::vector<Artifact::ObjectInfo> object_infos{}; - object_infos.reserve(artifacts.size()); - for (auto const& [path, info] : artifacts) { + object_infos.reserve(artifacts.value()->size()); + for (auto const& [path, info] : *artifacts.value()) { object_infos.emplace_back(info); } if (not alternative_api->RetrieveToCas(object_infos, api)) { @@ -588,15 +592,19 @@ class ExecutorImpl { } } - auto artifacts = response->Artifacts(); + auto const artifacts = response->Artifacts(); + if (not artifacts) { + logger.Emit(LogLevel::Error, artifacts.error()); + return false; + } auto output_files = action->OutputFilePaths(); auto output_dirs = action->OutputDirPaths(); - if (artifacts.empty() or + if (artifacts.value()->empty() or not CheckOutputsExist( - artifacts, output_files, action->Content().Cwd()) or + *artifacts.value(), output_files, action->Content().Cwd()) or not CheckOutputsExist( - artifacts, output_dirs, action->Content().Cwd())) { + *artifacts.value(), output_dirs, action->Content().Cwd())) { logger.Emit(LogLevel::Error, [&] { std::string message{ "action executed with missing outputs.\n" @@ -613,7 +621,7 @@ class ExecutorImpl { return false; } - SaveObjectInfo(artifacts, action, should_fail_outputs); + SaveObjectInfo(*artifacts.value(), action, should_fail_outputs); return true; } @@ -901,7 +909,11 @@ class Rebuilder { return false; } - DetectFlakyAction(*response, *response_cached, action->Content()); + if (auto error = DetectFlakyAction( + *response, *response_cached, action->Content())) { + logger_cached.Emit(LogLevel::Error, *error); + return false; + } return Impl::ParseResponse(logger, *response, action, @@ -943,18 +955,25 @@ class Rebuilder { std::pair<Artifact::ObjectInfo, Artifact::ObjectInfo>>> flaky_actions_{}; - void DetectFlakyAction(IExecutionResponse::Ptr const& response, - IExecutionResponse::Ptr const& response_cached, - Action const& action) const noexcept { + [[nodiscard]] auto DetectFlakyAction( + IExecutionResponse::Ptr const& response, + IExecutionResponse::Ptr const& response_cached, + Action const& action) const noexcept -> std::optional<std::string> { auto& stats = *context_.statistics; if (response and response_cached and response_cached->ActionDigest() == response->ActionDigest()) { stats.IncrementRebuiltActionComparedCounter(); - auto artifacts = response->Artifacts(); - auto artifacts_cached = response_cached->Artifacts(); + auto const artifacts = response->Artifacts(); + if (not artifacts) { + return artifacts.error(); + } + auto const artifacts_cached = response_cached->Artifacts(); + if (not artifacts_cached) { + return artifacts_cached.error(); + } std::ostringstream msg{}; - for (auto const& [path, info] : artifacts) { - auto const& info_cached = artifacts_cached[path]; + for (auto const& [path, info] : *artifacts.value()) { + auto const& info_cached = artifacts_cached.value()->at(path); if (info != info_cached) { RecordFlakyAction(&msg, action, path, info, info_cached); } @@ -975,6 +994,7 @@ class Rebuilder { std::unique_lock lock{m_}; cache_misses_.emplace_back(action.Id()); } + return std::nullopt; // ok } void RecordFlakyAction(gsl::not_null<std::ostringstream*> const& msg, |