diff options
author | Paul Cristian Sarbu <paul.cristian.sarbu@huawei.com> | 2024-09-12 17:45:14 +0200 |
---|---|---|
committer | Paul Cristian Sarbu <paul.cristian.sarbu@huawei.com> | 2024-09-16 10:01:34 +0200 |
commit | 49c2382ea5ea45966ef15140fce6ad89672e956c (patch) | |
tree | da781f16f3315f662492af00648fc1469efa55a0 | |
parent | 7d925d8d6c5fed37bb74506c5b7589b99f5d2885 (diff) | |
download | justbuild-49c2382ea5ea45966ef15140fce6ad89672e956c.tar.gz |
execution_response: Allow failures to be reported while populating
As populating the containers from remote response only takes place
once, no assumptions should be made that this cannot fail (for
example if wrong or invalid entries were produced). Instead, return
error messages on failure to callers that can log accordingly.
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: |