diff options
13 files changed, 299 insertions, 162 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, diff --git a/test/buildtool/execution_api/common/api_test.hpp b/test/buildtool/execution_api/common/api_test.hpp index 4b84d66a..d00eec7b 100644 --- a/test/buildtool/execution_api/common/api_test.hpp +++ b/test/buildtool/execution_api/common/api_test.hpp @@ -164,26 +164,28 @@ using ExecProps = std::map<std::string, std::string>; action->SetCacheFlag(IExecutionAction::CacheFlag::CacheOutput); // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); if (is_hermetic) { CHECK(not response->IsCached()); SECTION("Rerun execution to verify caching") { // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); CHECK(response->IsCached()); } } @@ -193,24 +195,25 @@ using ExecProps = std::map<std::string, std::string>; action->SetCacheFlag(IExecutionAction::CacheFlag::DoNotCacheOutput); // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); CHECK(not response->IsCached()); SECTION("Rerun execution to verify caching") { // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); CHECK(not response->IsCached()); } } @@ -254,26 +257,28 @@ using ExecProps = std::map<std::string, std::string>; action->SetCacheFlag(IExecutionAction::CacheFlag::CacheOutput); // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); if (is_hermetic) { CHECK(not response->IsCached()); SECTION("Rerun execution to verify caching") { // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); CHECK(response->IsCached()); } } @@ -283,24 +288,26 @@ using ExecProps = std::map<std::string, std::string>; action->SetCacheFlag(IExecutionAction::CacheFlag::DoNotCacheOutput); // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); CHECK(not response->IsCached()); SECTION("Rerun execution to verify caching") { // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); CHECK(not response->IsCached()); } } @@ -335,27 +342,29 @@ using ExecProps = std::map<std::string, std::string>; action->SetCacheFlag(IExecutionAction::CacheFlag::CacheOutput); // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result CHECK(response->ExitCode() == 1); - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); CHECK(not response->IsCached()); SECTION("Rerun execution to verify that non-zero actions are rerun") { // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result CHECK(response->ExitCode() == 1); - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); CHECK(not response->IsCached()); } } @@ -364,14 +373,15 @@ using ExecProps = std::map<std::string, std::string>; action->SetCacheFlag(IExecutionAction::CacheFlag::DoNotCacheOutput); // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result CHECK(response->ExitCode() == 1); - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); CHECK(not response->IsCached()); SECTION("Rerun execution to verify non-zero actions are not cached") { @@ -381,9 +391,10 @@ using ExecProps = std::map<std::string, std::string>; // verify result CHECK(response->ExitCode() == 1); - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); CHECK(not response->IsCached()); } } @@ -427,7 +438,7 @@ using ExecProps = std::map<std::string, std::string>; action->SetCacheFlag(IExecutionAction::CacheFlag::CacheOutput); // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result @@ -437,10 +448,11 @@ using ExecProps = std::map<std::string, std::string>; CHECK_FALSE(response->IsCached()); } - auto artifacts = response->Artifacts(); - REQUIRE_FALSE(artifacts.empty()); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE_FALSE(artifacts.value()->empty()); - auto info = artifacts.begin()->second; + auto info = artifacts.value()->begin()->second; SECTION("retrieve via same API object") { auto out_path = GetTestDir(test_name) / "out1"; @@ -500,7 +512,7 @@ TestRetrieveFileAndSymlinkWithSameContentToPath(ApiFactory const& api_factory, action->SetCacheFlag(IExecutionAction::CacheFlag::CacheOutput); // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result @@ -510,10 +522,11 @@ TestRetrieveFileAndSymlinkWithSameContentToPath(ApiFactory const& api_factory, CHECK_FALSE(response->IsCached()); } - auto artifacts = response->Artifacts(); - REQUIRE_FALSE(artifacts.empty()); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE_FALSE(artifacts.value()->empty()); - auto info = artifacts.begin()->second; + auto info = artifacts.value()->begin()->second; SECTION("retrieve via same API object") { auto out_path = GetTestDir(test_name) / "out1"; @@ -569,7 +582,7 @@ TestRetrieveFileAndSymlinkWithSameContentToPath(ApiFactory const& api_factory, action->SetCacheFlag(IExecutionAction::CacheFlag::CacheOutput); // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result @@ -579,16 +592,17 @@ TestRetrieveFileAndSymlinkWithSameContentToPath(ApiFactory const& api_factory, CHECK_FALSE(response->IsCached()); } - auto artifacts = response->Artifacts(); - REQUIRE_FALSE(artifacts.empty()); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE_FALSE(artifacts.value()->empty()); std::vector<std::filesystem::path> paths{}; std::vector<Artifact::ObjectInfo> infos{}; SECTION("retrieve via same API object") { auto out_path = GetTestDir(test_name) / "out1"; - std::for_each(artifacts.begin(), - artifacts.end(), + std::for_each(artifacts.value()->begin(), + artifacts.value()->end(), [&out_path, &paths, &infos](auto const& entry) { paths.emplace_back(out_path / entry.first); infos.emplace_back(entry.second); @@ -602,8 +616,8 @@ TestRetrieveFileAndSymlinkWithSameContentToPath(ApiFactory const& api_factory, SECTION("retrieve from new API object but same endpoint") { auto second_api = api_factory(); auto out_path = GetTestDir(test_name) / "out2"; - std::for_each(artifacts.begin(), - artifacts.end(), + std::for_each(artifacts.value()->begin(), + artifacts.value()->end(), [&out_path, &paths, &infos](auto const& entry) { paths.emplace_back(out_path / entry.first); infos.emplace_back(entry.second); @@ -638,26 +652,28 @@ TestRetrieveFileAndSymlinkWithSameContentToPath(ApiFactory const& api_factory, action->SetCacheFlag(IExecutionAction::CacheFlag::CacheOutput); // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(IsTreeObject(artifacts.at(output_path).type)); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(IsTreeObject(artifacts.value()->at(output_path).type)); if (is_hermetic) { CHECK(not response->IsCached()); SECTION("Rerun execution to verify caching") { // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(IsTreeObject(artifacts.at(output_path).type)); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(IsTreeObject(artifacts.value()->at(output_path).type)); CHECK(response->IsCached()); } } @@ -667,24 +683,26 @@ TestRetrieveFileAndSymlinkWithSameContentToPath(ApiFactory const& api_factory, action->SetCacheFlag(IExecutionAction::CacheFlag::DoNotCacheOutput); // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(IsTreeObject(artifacts.at(output_path).type)); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(IsTreeObject(artifacts.value()->at(output_path).type)); CHECK(not response->IsCached()); SECTION("Rerun execution to verify caching") { // run execution - auto response = action->Execute(); + auto const response = action->Execute(); REQUIRE(response); // verify result - auto artifacts = response->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(IsTreeObject(artifacts.at(output_path).type)); + auto const artifacts = response->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(IsTreeObject(artifacts.value()->at(output_path).type)); CHECK(not response->IsCached()); } } diff --git a/test/buildtool/execution_api/local/local_execution.test.cpp b/test/buildtool/execution_api/local/local_execution.test.cpp index 120100de..28fd03c6 100644 --- a/test/buildtool/execution_api/local/local_execution.test.cpp +++ b/test/buildtool/execution_api/local/local_execution.test.cpp @@ -211,9 +211,10 @@ TEST_CASE("LocalExecution: No input, create output", "[execution_api]") { // verify result CHECK_FALSE(output->IsCached()); - auto artifacts = output->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = output->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); // ensure result IS in cache output = action->Execute(nullptr); @@ -229,9 +230,10 @@ TEST_CASE("LocalExecution: No input, create output", "[execution_api]") { // verify result CHECK_FALSE(output->IsCached()); - auto artifacts = output->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = output->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); // ensure result IS STILL NOT in cache output = action->Execute(nullptr); @@ -290,9 +292,10 @@ TEST_CASE("LocalExecution: One input copied to output", "[execution_api]") { // verify result CHECK_FALSE(output->IsCached()); - auto artifacts = output->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = output->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); // ensure result IS in cache output = action->Execute(nullptr); @@ -308,9 +311,10 @@ TEST_CASE("LocalExecution: One input copied to output", "[execution_api]") { // verify result CHECK_FALSE(output->IsCached()); - auto artifacts = output->Artifacts(); - REQUIRE(artifacts.contains(output_path)); - CHECK(artifacts.at(output_path).digest == test_digest); + auto const artifacts = output->Artifacts(); + REQUIRE(artifacts.has_value()); + REQUIRE(artifacts.value()->contains(output_path)); + CHECK(artifacts.value()->at(output_path).digest == test_digest); // ensure result IS STILL NOT in cache output = action->Execute(nullptr); diff --git a/test/buildtool/execution_engine/executor/TARGETS b/test/buildtool/execution_engine/executor/TARGETS index bb86c6d8..343b83eb 100644 --- a/test/buildtool/execution_engine/executor/TARGETS +++ b/test/buildtool/execution_engine/executor/TARGETS @@ -43,6 +43,7 @@ , ["@", "src", "src/buildtool/execution_engine/executor", "executor"] , ["@", "src", "src/buildtool/progress_reporting", "progress"] , ["@", "src", "src/buildtool/crypto", "hash_function"] + , ["@", "src", "src/utils/cpp", "expected"] , ["", "catch-main"] , ["@", "catch2", "", "catch2"] , ["@", "gsl", "", "gsl"] diff --git a/test/buildtool/execution_engine/executor/executor.test.cpp b/test/buildtool/execution_engine/executor/executor.test.cpp index 48bcf082..e2d62342 100644 --- a/test/buildtool/execution_engine/executor/executor.test.cpp +++ b/test/buildtool/execution_engine/executor/executor.test.cpp @@ -38,6 +38,7 @@ #include "src/buildtool/execution_engine/executor/context.hpp" #include "src/buildtool/file_system/file_system_manager.hpp" #include "src/buildtool/progress_reporting/progress.hpp" +#include "src/utils/cpp/expected.hpp" #include "test/utils/executor/test_api_bundle.hpp" #include "test/utils/hermeticity/test_hash_function_type.hpp" @@ -97,16 +98,17 @@ class TestResponse : public IExecutionResponse { static const std::string kEmptyHash; return kEmptyHash; } - [[nodiscard]] auto Artifacts() noexcept -> ArtifactInfos const& final { + [[nodiscard]] auto Artifacts() noexcept + -> expected<gsl::not_null<ArtifactInfos const*>, std::string> final { if (not populated_) { Populate(); } - return artifacts_; + return gsl::not_null<ArtifactInfos const*>(&artifacts_); } [[nodiscard]] auto DirectorySymlinks() noexcept - -> DirSymlinks const& final { + -> expected<gsl::not_null<DirSymlinks const*>, std::string> final { static const DirSymlinks kEmptySymlinks{}; - return kEmptySymlinks; + return gsl::not_null<DirSymlinks const*>(&kEmptySymlinks); } private: |