diff options
23 files changed, 227 insertions, 189 deletions
diff --git a/src/buildtool/execution_api/bazel_msg/bazel_msg_factory.cpp b/src/buildtool/execution_api/bazel_msg/bazel_msg_factory.cpp index 6eb35e4c..ccc0f2d7 100644 --- a/src/buildtool/execution_api/bazel_msg/bazel_msg_factory.cpp +++ b/src/buildtool/execution_api/bazel_msg/bazel_msg_factory.cpp @@ -164,13 +164,14 @@ struct DirectoryNodeBundle final { // SHA256 is used since bazel types are processed here. HashFunction const hash_function{HashFunction::Type::PlainSHA256}; - auto digest = ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, *content); - - return DirectoryNodeBundle{ - .message = CreateDirectoryNode(dir_name, digest), - .blob = ArtifactBlob{ - std::move(digest), std::move(*content), /*is_exec=*/false}}; + auto blob = ArtifactBlob::FromMemory( + hash_function, ObjectType::File, *std::move(content)); + if (not blob.has_value()) { + return std::nullopt; + } + auto const digest = blob->GetDigest(); + return DirectoryNodeBundle{.message = CreateDirectoryNode(dir_name, digest), + .blob = *std::move(blob)}; } /// \brief Create bundle for protobuf message Command from args strings. @@ -201,11 +202,12 @@ struct DirectoryNodeBundle final { if (not content) { return std::nullopt; } - auto digest = ArtifactDigestFactory::HashDataAs<ObjectType::File>( - request.hash_function, *content); - return ArtifactBlob{std::move(digest), - std::move(*content), - /*is_exec=*/false}; + auto blob = ArtifactBlob::FromMemory( + request.hash_function, ObjectType::File, *std::move(content)); + if (not blob.has_value()) { + return std::nullopt; + } + return *std::move(blob); } /// \brief Create bundle for protobuf message Action from Command. @@ -240,11 +242,12 @@ struct DirectoryNodeBundle final { if (not content) { return std::nullopt; } - auto digest = ArtifactDigestFactory::HashDataAs<ObjectType::File>( - request.hash_function, *content); - return ArtifactBlob{std::move(digest), - std::move(*content), - /*is_exec=*/false}; + auto blob = ArtifactBlob::FromMemory( + request.hash_function, ObjectType::File, *std::move(content)); + if (not blob.has_value()) { + return std::nullopt; + } + return *std::move(blob); } /// \brief Convert `DirectoryTree` to `DirectoryNodeBundle`. diff --git a/src/buildtool/execution_api/common/TARGETS b/src/buildtool/execution_api/common/TARGETS index 6eeb3931..39ccc042 100644 --- a/src/buildtool/execution_api/common/TARGETS +++ b/src/buildtool/execution_api/common/TARGETS @@ -133,8 +133,7 @@ , ["src/buildtool/execution_api/bazel_msg", "directory_tree"] ] , "private-deps": - [ ["src/buildtool/common", "artifact_digest_factory"] - , ["src/buildtool/common", "common"] + [ ["src/buildtool/common", "common"] , ["src/buildtool/crypto", "hash_function"] , ["src/buildtool/file_system", "git_repo"] , ["src/buildtool/file_system", "object_type"] diff --git a/src/buildtool/execution_api/common/blob_tree.cpp b/src/buildtool/execution_api/common/blob_tree.cpp index 7224406a..e6d8fc2c 100644 --- a/src/buildtool/execution_api/common/blob_tree.cpp +++ b/src/buildtool/execution_api/common/blob_tree.cpp @@ -22,7 +22,6 @@ #include "src/buildtool/common/artifact.hpp" #include "src/buildtool/common/artifact_digest.hpp" -#include "src/buildtool/common/artifact_digest_factory.hpp" #include "src/buildtool/crypto/hash_function.hpp" #include "src/buildtool/file_system/git_repo.hpp" #include "src/buildtool/file_system/object_type.hpp" @@ -68,17 +67,13 @@ auto BlobTree::FromDirectoryTree(DirectoryTreePtr const& tree, } } if (auto git_tree = GitRepo::CreateShallowTree(entries)) { - auto digest = - ArtifactDigestFactory::Create(HashFunction::Type::GitSHA1, - ToHexString(git_tree->first), - git_tree->second.size(), - /*is_tree=*/true); - if (digest) { - return std::make_shared<BlobTree>( - ArtifactBlob{*std::move(digest), - git_tree->second, - /*is_exec=*/false}, - nodes); + auto blob = ArtifactBlob::FromMemory( + HashFunction{HashFunction::Type::GitSHA1}, + ObjectType::Tree, + std::move(git_tree)->second); + if (blob.has_value()) { + return std::make_shared<BlobTree>(*std::move(blob), + std::move(nodes)); } } } catch (...) { diff --git a/src/buildtool/execution_api/git/git_api.cpp b/src/buildtool/execution_api/git/git_api.cpp index 6021a913..231e717a 100644 --- a/src/buildtool/execution_api/git/git_api.cpp +++ b/src/buildtool/execution_api/git/git_api.cpp @@ -223,19 +223,16 @@ auto GitApi::RetrieveToCas( return false; } - ArtifactDigest digest = - IsTreeObject(info->type) - ? ArtifactDigestFactory::HashDataAs<ObjectType::Tree>( - hash_function, *content) - : ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, *content); + auto blob = ArtifactBlob::FromMemory( + hash_function, info->type, *std::move(content)); + if (not blob.has_value()) { + return false; + } // Collect blob and upload to remote CAS if transfer size reached. if (not UpdateContainerAndUpload( &container, - ArtifactBlob{std::move(digest), - std::move(*content), - IsExecutableObject(info->type)}, + *std::move(blob), /*exception_is_fatal=*/true, [&api](std::unordered_set<ArtifactBlob>&& blobs) { return api.Upload(std::move(blobs), diff --git a/src/buildtool/execution_api/local/local_api.cpp b/src/buildtool/execution_api/local/local_api.cpp index 040b4be6..92953190 100644 --- a/src/buildtool/execution_api/local/local_api.cpp +++ b/src/buildtool/execution_api/local/local_api.cpp @@ -25,7 +25,6 @@ #include <grpcpp/support/status.h> -#include "src/buildtool/common/artifact_digest_factory.hpp" #include "src/buildtool/common/protocol_traits.hpp" #include "src/buildtool/execution_api/bazel_msg/directory_tree.hpp" #include "src/buildtool/execution_api/common/common_api.hpp" @@ -177,26 +176,23 @@ auto LocalApi::RetrieveToCas( } // Read artifact content (file or symlink). - auto const& content = FileSystemManager::ReadFile(*path); + auto content = FileSystemManager::ReadFile(*path); if (not content) { return false; } - // Regenerate digest since object infos read by - // storage_.ReadTreeInfos() will contain 0 as size. - ArtifactDigest digest = - IsTreeObject(info->type) - ? ArtifactDigestFactory::HashDataAs<ObjectType::Tree>( - local_context_.storage_config->hash_function, *content) - : ArtifactDigestFactory::HashDataAs<ObjectType::File>( - local_context_.storage_config->hash_function, *content); + auto blob = ArtifactBlob::FromMemory( + local_context_.storage_config->hash_function, + info->type, + *std::move(content)); + if (not blob.has_value()) { + return false; + } // Collect blob and upload to remote CAS if transfer size reached. if (not UpdateContainerAndUpload( &container, - ArtifactBlob{std::move(digest), - *content, - IsExecutableObject(info->type)}, + *std::move(blob), /*exception_is_fatal=*/true, [&api](std::unordered_set<ArtifactBlob>&& blobs) { return api.Upload(std::move(blobs), diff --git a/src/buildtool/execution_api/remote/TARGETS b/src/buildtool/execution_api/remote/TARGETS index 0dd5ccb0..a575edb0 100644 --- a/src/buildtool/execution_api/remote/TARGETS +++ b/src/buildtool/execution_api/remote/TARGETS @@ -33,6 +33,7 @@ , ["src/buildtool/execution_api/common", "ids"] , ["src/buildtool/execution_api/common", "message_limits"] , ["src/buildtool/file_system", "git_repo"] + , ["src/buildtool/file_system", "object_type"] , ["src/buildtool/logging", "log_level"] , ["src/buildtool/logging", "logging"] , ["src/utils/cpp", "expected"] @@ -54,7 +55,6 @@ , ["src/buildtool/common", "protocol_traits"] , ["src/buildtool/common/remote", "retry"] , ["src/buildtool/execution_api/bazel_msg", "bazel_msg_factory"] - , ["src/buildtool/file_system", "object_type"] , ["src/utils/cpp", "back_map"] , ["src/utils/cpp", "gsl"] , ["src/utils/cpp", "path"] diff --git a/src/buildtool/execution_api/remote/bazel/bazel_cas_client.cpp b/src/buildtool/execution_api/remote/bazel/bazel_cas_client.cpp index dd4ec919..a02b8d4c 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_cas_client.cpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_cas_client.cpp @@ -36,6 +36,7 @@ #include "src/buildtool/file_system/object_type.hpp" #include "src/buildtool/logging/log_level.hpp" #include "src/utils/cpp/back_map.hpp" +#include "src/utils/cpp/expected.hpp" namespace { @@ -302,12 +303,19 @@ auto BazelCasClient::BatchReadBlobs( std::vector<ArtifactBlob>* v, bazel_re::BatchReadBlobsResponse_Response const& r) { - if (auto value = - back_map->GetReference(r.digest())) { - v->emplace_back(*value.value(), - r.data(), - /*is_exec=*/false); + auto ref = back_map->GetReference(r.digest()); + if (not ref.has_value()) { + return; } + auto blob = ArtifactBlob::FromMemory( + HashFunction{ref.value()->GetHashType()}, + ref.value()->IsTree() ? ObjectType::Tree + : ObjectType::File, + r.data()); + if (not blob.has_value()) { + return; + } + v->emplace_back(*std::move(blob)); }); if (batch_response.ok) { std::move(batch_response.result.begin(), diff --git a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp index f06a4275..d0429cd7 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp @@ -50,12 +50,8 @@ auto ProcessDirectoryMessage(HashFunction hash_function, fmt::format("found invalid symlink at {}", link.name())}; } } - auto data = dir.SerializeAsString(); - auto digest = ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, data); - return ArtifactBlob{std::move(digest), - std::move(data), - /*is_exec=*/false}; + return ArtifactBlob::FromMemory( + hash_function, ObjectType::File, dir.SerializeAsString()); } } // namespace diff --git a/src/buildtool/execution_api/remote/bazel/bytestream_client.hpp b/src/buildtool/execution_api/remote/bazel/bytestream_client.hpp index 23a296c9..cf85b769 100644 --- a/src/buildtool/execution_api/remote/bazel/bytestream_client.hpp +++ b/src/buildtool/execution_api/remote/bazel/bytestream_client.hpp @@ -33,8 +33,10 @@ #include "src/buildtool/common/artifact_digest.hpp" #include "src/buildtool/common/remote/client_common.hpp" #include "src/buildtool/common/remote/port.hpp" +#include "src/buildtool/crypto/hash_function.hpp" #include "src/buildtool/execution_api/common/bytestream_utils.hpp" #include "src/buildtool/execution_api/common/ids.hpp" +#include "src/buildtool/file_system/object_type.hpp" #include "src/buildtool/logging/log_level.hpp" #include "src/buildtool/logging/logger.hpp" #include "src/utils/cpp/expected.hpp" @@ -115,7 +117,15 @@ class ByteStreamClient { if (not data) { return std::nullopt; } - return ArtifactBlob{digest, std::move(output), /*is_exec=*/false}; + + auto blob = ArtifactBlob::FromMemory( + HashFunction{digest.GetHashType()}, + digest.IsTree() ? ObjectType::Tree : ObjectType::File, + std::move(output)); + if (not blob.has_value() or blob->GetDigest() != digest) { + return std::nullopt; + } + return *std::move(blob); } [[nodiscard]] auto Write(std::string const& instance_name, diff --git a/src/buildtool/execution_engine/executor/executor.hpp b/src/buildtool/execution_engine/executor/executor.hpp index eea0336d..93cb9655 100644 --- a/src/buildtool/execution_engine/executor/executor.hpp +++ b/src/buildtool/execution_engine/executor/executor.hpp @@ -331,6 +331,7 @@ class ExecutorImpl { } // upload missing entries (blobs or trees) + HashFunction const hash_function{api.GetHashType()}; std::unordered_set<ArtifactBlob> container; for (auto const& [digest, value] : missing_entries) { auto const entry = value->second; @@ -338,13 +339,16 @@ class ExecutorImpl { if (not content.has_value()) { return false; } + auto blob = ArtifactBlob::FromMemory( + hash_function, entry->Type(), *std::move(content)); + if (not blob.has_value()) { + return false; + } // store and/or upload blob, taking into account the maximum // transfer size if (not UpdateContainerAndUpload( &container, - ArtifactBlob{*digest, - std::move(*content), - IsExecutableObject(entry->Type())}, + *std::move(blob), /*exception_is_fatal=*/true, [&api](std::unordered_set<ArtifactBlob>&& blobs) { return api.Upload(std::move(blobs), @@ -396,9 +400,14 @@ class ExecutorImpl { return false; } - return api.Upload({ArtifactBlob{info.digest, - std::move(*content), - IsExecutableObject(info.type)}}, + auto blob = ArtifactBlob::FromMemory( + HashFunction{api.GetHashType()}, info.type, *std::move(content)); + if (not blob.has_value()) { + Logger::Log(LogLevel::Error, "failed to create ArtifactBlob"); + return false; + } + + return api.Upload({*std::move(blob)}, /*skip_find_missing=*/true); } @@ -483,12 +492,14 @@ class ExecutorImpl { if (not content.has_value()) { return std::nullopt; } - HashFunction const hash_function{api.GetHashType()}; - auto digest = ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, *content); - if (not api.Upload({ArtifactBlob{digest, - std::move(*content), - IsExecutableObject(*object_type)}})) { + + auto blob = ArtifactBlob::FromMemory( + HashFunction{api.GetHashType()}, *object_type, *std::move(content)); + if (not blob.has_value()) { + return std::nullopt; + } + auto digest = blob->GetDigest(); + if (not api.Upload({*std::move(blob)})) { return std::nullopt; } return Artifact::ObjectInfo{.digest = std::move(digest), diff --git a/src/buildtool/graph_traverser/TARGETS b/src/buildtool/graph_traverser/TARGETS index ab085350..e4aeeaa3 100644 --- a/src/buildtool/graph_traverser/TARGETS +++ b/src/buildtool/graph_traverser/TARGETS @@ -9,7 +9,6 @@ , ["src/buildtool/common", "action_description"] , ["src/buildtool/common", "artifact_blob"] , ["src/buildtool/common", "artifact_description"] - , ["src/buildtool/common", "artifact_digest_factory"] , ["src/buildtool/common", "cli"] , ["src/buildtool/common", "common"] , ["src/buildtool/common", "tree"] @@ -28,6 +27,7 @@ , ["src/buildtool/logging", "log_level"] , ["src/buildtool/logging", "logging"] , ["src/buildtool/progress_reporting", "base_progress_reporter"] + , ["src/utils/cpp", "expected"] , ["src/utils/cpp", "json"] , ["src/utils/cpp", "path"] ] diff --git a/src/buildtool/graph_traverser/graph_traverser.hpp b/src/buildtool/graph_traverser/graph_traverser.hpp index ac3067e8..5d240cd6 100644 --- a/src/buildtool/graph_traverser/graph_traverser.hpp +++ b/src/buildtool/graph_traverser/graph_traverser.hpp @@ -53,7 +53,6 @@ #include "src/buildtool/common/artifact_blob.hpp" #include "src/buildtool/common/artifact_description.hpp" #include "src/buildtool/common/artifact_digest.hpp" -#include "src/buildtool/common/artifact_digest_factory.hpp" #include "src/buildtool/common/cli.hpp" #include "src/buildtool/common/identifier.hpp" #include "src/buildtool/common/statistics.hpp" @@ -73,6 +72,7 @@ #include "src/buildtool/logging/log_level.hpp" #include "src/buildtool/logging/logger.hpp" #include "src/buildtool/progress_reporting/base_progress_reporter.hpp" +#include "src/utils/cpp/expected.hpp" #include "src/utils/cpp/json.hpp" #include "src/utils/cpp/path.hpp" @@ -313,21 +313,27 @@ class GraphTraverser { std::vector<std::string> const& blobs) const noexcept -> bool { std::unordered_set<ArtifactBlob> container; HashFunction const hash_function{context_.apis->remote->GetHashType()}; - for (auto const& blob : blobs) { - auto digest = ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, blob); + for (auto const& content : blobs) { + auto blob = ArtifactBlob::FromMemory( + hash_function, ObjectType::File, content); + if (not blob.has_value()) { + logger_->Emit(LogLevel::Trace, + "Failed to create ArtifactBlob for:\n{}", + nlohmann::json(content).dump()); + return false; + } Logger::Log(logger_, LogLevel::Trace, [&]() { return fmt::format( "Will upload blob {}, its digest has id {} and size {}.", - nlohmann::json(blob).dump(), - digest.hash(), - digest.size()); + nlohmann::json(content).dump(), + blob->GetDigest().hash(), + blob->GetDigest().size()); }); // Store and/or upload blob, taking into account the maximum // transfer size. if (not UpdateContainerAndUpload( &container, - ArtifactBlob{std::move(digest), blob, /*is_exec=*/false}, + *std::move(blob), /*exception_is_fatal=*/true, [&api = context_.apis->remote]( std::unordered_set<ArtifactBlob>&& blobs) { diff --git a/test/buildtool/execution_api/bazel/TARGETS b/test/buildtool/execution_api/bazel/TARGETS index ce5c8077..25fbda81 100644 --- a/test/buildtool/execution_api/bazel/TARGETS +++ b/test/buildtool/execution_api/bazel/TARGETS @@ -6,7 +6,6 @@ [ ["@", "catch2", "", "catch2"] , ["@", "gsl", "", "gsl"] , ["@", "src", "src/buildtool/common", "artifact_blob"] - , ["@", "src", "src/buildtool/common", "artifact_digest_factory"] , ["@", "src", "src/buildtool/common", "common"] , ["@", "src", "src/buildtool/common/remote", "remote_common"] , ["@", "src", "src/buildtool/common/remote", "retry_config"] @@ -14,6 +13,7 @@ , ["@", "src", "src/buildtool/execution_api/remote", "bazel_network"] , ["@", "src", "src/buildtool/execution_api/remote", "config"] , ["@", "src", "src/buildtool/file_system", "object_type"] + , ["@", "src", "src/utils/cpp", "expected"] , ["utils", "catch-main-remote-execution"] , ["utils", "test_auth_config"] , ["utils", "test_hash_function_type"] @@ -63,6 +63,7 @@ , ["@", "src", "src/buildtool/execution_api/remote", "bazel_network"] , ["@", "src", "src/buildtool/execution_api/remote", "config"] , ["@", "src", "src/buildtool/file_system", "object_type"] + , ["@", "src", "src/utils/cpp", "expected"] , ["utils", "catch-main-remote-execution"] , ["utils", "execution_bazel"] , ["utils", "test_auth_config"] @@ -80,7 +81,6 @@ , ["@", "grpc", "", "grpc"] , ["@", "gsl", "", "gsl"] , ["@", "src", "src/buildtool/common", "artifact_blob"] - , ["@", "src", "src/buildtool/common", "artifact_digest_factory"] , ["@", "src", "src/buildtool/common", "common"] , ["@", "src", "src/buildtool/common", "protocol_traits"] , ["@", "src", "src/buildtool/common/remote", "remote_common"] @@ -90,6 +90,7 @@ , ["@", "src", "src/buildtool/execution_api/remote", "bazel_network"] , ["@", "src", "src/buildtool/execution_api/remote", "config"] , ["@", "src", "src/buildtool/file_system", "object_type"] + , ["@", "src", "src/utils/cpp", "expected"] , ["utils", "catch-main-remote-execution"] , ["utils", "execution_bazel"] , ["utils", "test_auth_config"] @@ -107,7 +108,6 @@ [ ["@", "catch2", "", "catch2"] , ["@", "src", "src/buildtool/common", "artifact_blob"] , ["@", "src", "src/buildtool/common", "artifact_description"] - , ["@", "src", "src/buildtool/common", "artifact_digest_factory"] , ["@", "src", "src/buildtool/common", "common"] , ["@", "src", "src/buildtool/crypto", "hash_function"] , [ "@" @@ -120,6 +120,7 @@ , ["@", "src", "src/buildtool/execution_engine/dag", "dag"] , ["@", "src", "src/buildtool/file_system", "file_system_manager"] , ["@", "src", "src/buildtool/file_system", "object_type"] + , ["@", "src", "src/utils/cpp", "expected"] , ["", "catch-main"] , ["utils", "test_hash_function_type"] ] diff --git a/test/buildtool/execution_api/bazel/bazel_cas_client.test.cpp b/test/buildtool/execution_api/bazel/bazel_cas_client.test.cpp index 031da8f0..814de567 100644 --- a/test/buildtool/execution_api/bazel/bazel_cas_client.test.cpp +++ b/test/buildtool/execution_api/bazel/bazel_cas_client.test.cpp @@ -24,13 +24,13 @@ #include "gsl/gsl" #include "src/buildtool/common/artifact_blob.hpp" #include "src/buildtool/common/artifact_digest.hpp" -#include "src/buildtool/common/artifact_digest_factory.hpp" #include "src/buildtool/common/remote/remote_common.hpp" #include "src/buildtool/common/remote/retry_config.hpp" #include "src/buildtool/crypto/hash_function.hpp" #include "src/buildtool/execution_api/remote/bazel/bazel_capabilities_client.hpp" #include "src/buildtool/execution_api/remote/config.hpp" #include "src/buildtool/file_system/object_type.hpp" +#include "src/utils/cpp/expected.hpp" #include "test/utils/hermeticity/test_hash_function_type.hpp" #include "test/utils/remote_execution/test_auth_config.hpp" #include "test/utils/remote_execution/test_remote_config.hpp" @@ -60,25 +60,26 @@ TEST_CASE("Bazel internals: CAS Client", "[execution_api]") { SECTION("Valid digest and blob") { // digest of "test" HashFunction const hash_function{TestHashType::ReadFromEnvironment()}; - auto const digest = ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, content); - // Valid blob - ArtifactBlob blob{digest, content, /*is_exec=*/false}; + auto const blob = + ArtifactBlob::FromMemory(hash_function, ObjectType::File, content); + REQUIRE(blob.has_value()); // Search blob via digest - auto digests = cas_client.FindMissingBlobs(instance_name, {digest}); + auto digests = + cas_client.FindMissingBlobs(instance_name, {blob->GetDigest()}); CHECK(digests.size() <= 1); if (not digests.empty()) { // Upload blob, if not found - CHECK(cas_client.BatchUpdateBlobs(instance_name, {blob}) == 1U); + CHECK(cas_client.BatchUpdateBlobs(instance_name, {*blob}) == 1U); } // Read blob - auto blobs = cas_client.BatchReadBlobs(instance_name, {digest}); + auto blobs = + cas_client.BatchReadBlobs(instance_name, {blob->GetDigest()}); REQUIRE(blobs.size() == 1); - CHECK(blobs.begin()->GetDigest() == digest); + CHECK(blobs.begin()->GetDigest() == blob->GetDigest()); auto const read_content = blobs.begin()->ReadContent(); CHECK(read_content != nullptr); CHECK(*read_content == content); diff --git a/test/buildtool/execution_api/bazel/bazel_msg_factory.test.cpp b/test/buildtool/execution_api/bazel/bazel_msg_factory.test.cpp index 6b106bc7..e4422884 100644 --- a/test/buildtool/execution_api/bazel/bazel_msg_factory.test.cpp +++ b/test/buildtool/execution_api/bazel/bazel_msg_factory.test.cpp @@ -28,12 +28,12 @@ #include "src/buildtool/common/artifact_blob.hpp" #include "src/buildtool/common/artifact_description.hpp" #include "src/buildtool/common/artifact_digest.hpp" -#include "src/buildtool/common/artifact_digest_factory.hpp" #include "src/buildtool/crypto/hash_function.hpp" #include "src/buildtool/execution_api/bazel_msg/directory_tree.hpp" #include "src/buildtool/execution_engine/dag/dag.hpp" #include "src/buildtool/file_system/file_system_manager.hpp" #include "src/buildtool/file_system/object_type.hpp" +#include "src/utils/cpp/expected.hpp" #include "test/utils/hermeticity/test_hash_function_type.hpp" namespace { @@ -46,14 +46,16 @@ namespace { if (not type) { return std::nullopt; } - auto const content = FileSystemManager::ReadContentAtPath(fpath, *type); + auto content = FileSystemManager::ReadContentAtPath(fpath, *type); if (not content.has_value()) { return std::nullopt; } - return ArtifactBlob{ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, *content), - *content, - IsExecutableObject(*type)}; + auto blob = ArtifactBlob::FromMemory( + hash_function, ObjectType::File, *std::move(content)); + if (not blob.has_value()) { + return std::nullopt; + } + return *std::move(blob); } } // namespace diff --git a/test/buildtool/execution_api/bazel/bazel_network.test.cpp b/test/buildtool/execution_api/bazel/bazel_network.test.cpp index 3cb0ef35..fd812287 100644 --- a/test/buildtool/execution_api/bazel/bazel_network.test.cpp +++ b/test/buildtool/execution_api/bazel/bazel_network.test.cpp @@ -28,7 +28,6 @@ #include "gsl/gsl" #include "src/buildtool/common/artifact_blob.hpp" #include "src/buildtool/common/artifact_digest.hpp" -#include "src/buildtool/common/artifact_digest_factory.hpp" #include "src/buildtool/common/protocol_traits.hpp" #include "src/buildtool/common/remote/remote_common.hpp" #include "src/buildtool/common/remote/retry_config.hpp" @@ -37,6 +36,7 @@ #include "src/buildtool/execution_api/remote/bazel/bazel_network_reader.hpp" #include "src/buildtool/execution_api/remote/config.hpp" #include "src/buildtool/file_system/object_type.hpp" +#include "src/utils/cpp/expected.hpp" #include "test/utils/hermeticity/test_hash_function_type.hpp" #include "test/utils/remote_execution/test_auth_config.hpp" #include "test/utils/remote_execution/test_remote_config.hpp" @@ -69,29 +69,26 @@ TEST_CASE("Bazel network: write/read blobs", "[execution_api]") { std::string content_bar("bar"); std::string content_baz(kLargeSize, 'x'); // single larger blob - ArtifactBlob foo{ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, content_foo), - content_foo, - /*is_exec=*/false}; - ArtifactBlob bar{ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, content_bar), - content_bar, - /*is_exec=*/false}; - ArtifactBlob baz{ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, content_baz), - content_baz, - /*is_exec=*/false}; + auto const foo = + ArtifactBlob::FromMemory(hash_function, ObjectType::File, content_foo); + REQUIRE(foo.has_value()); + auto const bar = + ArtifactBlob::FromMemory(hash_function, ObjectType::File, content_bar); + REQUIRE(bar.has_value()); + auto const baz = + ArtifactBlob::FromMemory(hash_function, ObjectType::File, content_baz); + REQUIRE(baz.has_value()); // Search blobs via digest - REQUIRE(network.UploadBlobs({foo, bar, baz})); + REQUIRE(network.UploadBlobs({*foo, *bar, *baz})); // Read blobs in order auto reader = network.CreateReader(); - std::vector<ArtifactDigest> to_read{foo.GetDigest(), - bar.GetDigest(), - baz.GetDigest(), - bar.GetDigest(), - foo.GetDigest()}; + std::vector<ArtifactDigest> to_read{foo->GetDigest(), + bar->GetDigest(), + baz->GetDigest(), + bar->GetDigest(), + foo->GetDigest()}; std::vector<ArtifactBlob> blobs{}; for (auto next : reader.ReadIncrementally(&to_read)) { blobs.insert(blobs.end(), next.begin(), next.end()); diff --git a/test/buildtool/execution_api/bazel/bytestream_client.test.cpp b/test/buildtool/execution_api/bazel/bytestream_client.test.cpp index 991dcf72..823c317c 100644 --- a/test/buildtool/execution_api/bazel/bytestream_client.test.cpp +++ b/test/buildtool/execution_api/bazel/bytestream_client.test.cpp @@ -30,6 +30,7 @@ #include "src/buildtool/crypto/hash_function.hpp" #include "src/buildtool/execution_api/remote/config.hpp" #include "src/buildtool/file_system/object_type.hpp" +#include "src/utils/cpp/expected.hpp" #include "test/utils/hermeticity/test_hash_function_type.hpp" #include "test/utils/remote_execution/test_auth_config.hpp" #include "test/utils/remote_execution/test_remote_config.hpp" @@ -53,13 +54,14 @@ TEST_CASE("ByteStream Client: Transfer single blob", "[execution_api]") { std::string content("foobar"); // digest of "foobar" - auto digest = ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, content); + auto const blob = + ArtifactBlob::FromMemory(hash_function, ObjectType::File, content); + REQUIRE(blob.has_value()); - ArtifactBlob const blob{digest, content, /*is_exec=*/false}; - CHECK(stream.Write(instance_name, blob)); + CHECK(stream.Write(instance_name, *blob)); - auto const downloaded_blob = stream.Read(instance_name, digest); + auto const downloaded_blob = + stream.Read(instance_name, blob->GetDigest()); REQUIRE(downloaded_blob.has_value()); auto const downloaded_content = downloaded_blob->ReadContent(); @@ -91,14 +93,15 @@ TEST_CASE("ByteStream Client: Transfer single blob", "[execution_api]") { } // digest of "instance_nameinstance_nameinstance_..." - auto digest = ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, content); - ArtifactBlob const blob{digest, content, /*is_exec=*/false}; + auto const blob = + ArtifactBlob::FromMemory(hash_function, ObjectType::File, content); + REQUIRE(blob.has_value()); - CHECK(stream.Write(instance_name, blob)); + CHECK(stream.Write(instance_name, *blob)); SECTION("Download large blob") { - auto const downloaded_blob = stream.Read(instance_name, digest); + auto const downloaded_blob = + stream.Read(instance_name, blob->GetDigest()); REQUIRE(downloaded_blob.has_value()); auto const downloaded_content = downloaded_blob->ReadContent(); @@ -107,7 +110,8 @@ TEST_CASE("ByteStream Client: Transfer single blob", "[execution_api]") { } SECTION("Incrementally download large blob") { - auto reader = stream.IncrementalRead(instance_name, digest); + auto reader = + stream.IncrementalRead(instance_name, blob->GetDigest()); std::string data{}; auto chunk = reader.Next(); diff --git a/test/buildtool/execution_api/common/api_test.hpp b/test/buildtool/execution_api/common/api_test.hpp index 8836181d..f8981434 100644 --- a/test/buildtool/execution_api/common/api_test.hpp +++ b/test/buildtool/execution_api/common/api_test.hpp @@ -241,12 +241,13 @@ using ExecProps = std::map<std::string, std::string>; HashFunction const hash_function{TestHashType::ReadFromEnvironment()}; - auto test_digest = ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, test_content); + auto const test_blob = + ArtifactBlob::FromMemory(hash_function, ObjectType::File, test_content); + REQUIRE(test_blob.has_value()); - auto input_artifact_opt = - ArtifactDescription::CreateKnown(test_digest, ObjectType::File) - .ToArtifact(); + auto input_artifact_opt = ArtifactDescription::CreateKnown( + test_blob->GetDigest(), ObjectType::File) + .ToArtifact(); auto input_artifact = DependencyGraph::ArtifactNode{std::move(input_artifact_opt)}; @@ -254,8 +255,7 @@ using ExecProps = std::map<std::string, std::string>; std::string output_path{"output_file"}; auto api = api_factory(); - CHECK(api->Upload( - {ArtifactBlob{test_digest, test_content, /*is_exec=*/false}}, false)); + CHECK(api->Upload({*test_blob}, false)); auto action = api->CreateAction(*api->UploadTree({{input_path, &input_artifact}}), @@ -277,7 +277,8 @@ using ExecProps = std::map<std::string, std::string>; 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(artifacts.value()->at(output_path).digest == + test_blob->GetDigest()); if (is_hermetic) { CHECK(not response->IsCached()); @@ -291,7 +292,8 @@ using ExecProps = std::map<std::string, std::string>; 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(artifacts.value()->at(output_path).digest == + test_blob->GetDigest()); CHECK(response->IsCached()); } } @@ -308,7 +310,8 @@ using ExecProps = std::map<std::string, std::string>; 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(artifacts.value()->at(output_path).digest == + test_blob->GetDigest()); CHECK(not response->IsCached()); SECTION("Rerun execution to verify caching") { @@ -320,7 +323,8 @@ using ExecProps = std::map<std::string, std::string>; 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(artifacts.value()->at(output_path).digest == + test_blob->GetDigest()); 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 a0220761..e1fbec99 100644 --- a/test/buildtool/execution_api/local/local_execution.test.cpp +++ b/test/buildtool/execution_api/local/local_execution.test.cpp @@ -273,19 +273,19 @@ TEST_CASE("LocalExecution: One input copied to output", "[execution_api]") { auto api = LocalApi(&local_context, &repo_config); std::string test_content("test"); - auto test_digest = ArtifactDigestFactory::HashDataAs<ObjectType::File>( - storage_config.Get().hash_function, test_content); - REQUIRE(api.Upload( - {ArtifactBlob{test_digest, test_content, /*is_exec=*/false}}, false)); + auto const test_blob = ArtifactBlob::FromMemory( + storage_config.Get().hash_function, ObjectType::File, test_content); + REQUIRE(test_blob); + REQUIRE(api.Upload({*test_blob}, false)); std::string input_path{"dir/subdir/input"}; std::string output_path{"output_file"}; std::vector<std::string> const cmdline = {"cp", input_path, output_path}; - auto local_artifact_opt = - ArtifactDescription::CreateKnown(test_digest, ObjectType::File) - .ToArtifact(); + auto local_artifact_opt = ArtifactDescription::CreateKnown( + test_blob->GetDigest(), ObjectType::File) + .ToArtifact(); auto local_artifact = DependencyGraph::ArtifactNode{std::move(local_artifact_opt)}; @@ -310,7 +310,8 @@ TEST_CASE("LocalExecution: One input copied to output", "[execution_api]") { auto const artifacts = output->Artifacts(); REQUIRE(artifacts.has_value()); REQUIRE(artifacts.value()->contains(output_path)); - CHECK(artifacts.value()->at(output_path).digest == test_digest); + CHECK(artifacts.value()->at(output_path).digest == + test_blob->GetDigest()); // ensure result IS in cache output = action->Execute(nullptr); @@ -329,7 +330,8 @@ TEST_CASE("LocalExecution: One input copied to output", "[execution_api]") { auto const artifacts = output->Artifacts(); REQUIRE(artifacts.has_value()); REQUIRE(artifacts.value()->contains(output_path)); - CHECK(artifacts.value()->at(output_path).digest == test_digest); + CHECK(artifacts.value()->at(output_path).digest == + test_blob->GetDigest()); // 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 bb7c43da..c0dcc56f 100644 --- a/test/buildtool/execution_engine/executor/TARGETS +++ b/test/buildtool/execution_engine/executor/TARGETS @@ -9,7 +9,6 @@ , ["@", "src", "src/buildtool/common", "action_description"] , ["@", "src", "src/buildtool/common", "artifact_blob"] , ["@", "src", "src/buildtool/common", "artifact_description"] - , ["@", "src", "src/buildtool/common", "artifact_digest_factory"] , ["@", "src", "src/buildtool/common", "common"] , ["@", "src", "src/buildtool/common", "config"] , ["@", "src", "src/buildtool/common", "tree"] @@ -24,6 +23,7 @@ , ["@", "src", "src/buildtool/file_system", "file_system_manager"] , ["@", "src", "src/buildtool/file_system", "object_type"] , ["@", "src", "src/buildtool/progress_reporting", "progress"] + , ["@", "src", "src/utils/cpp", "expected"] , ["utils", "test_api_bundle"] , ["utils", "test_hash_function_type"] , ["utils", "test_remote_config"] diff --git a/test/buildtool/execution_engine/executor/executor_api.test.hpp b/test/buildtool/execution_engine/executor/executor_api.test.hpp index 20b4a091..6a8f409e 100644 --- a/test/buildtool/execution_engine/executor/executor_api.test.hpp +++ b/test/buildtool/execution_engine/executor/executor_api.test.hpp @@ -36,7 +36,6 @@ #include "src/buildtool/common/artifact_blob.hpp" #include "src/buildtool/common/artifact_description.hpp" #include "src/buildtool/common/artifact_digest.hpp" -#include "src/buildtool/common/artifact_digest_factory.hpp" #include "src/buildtool/common/remote/retry_config.hpp" #include "src/buildtool/common/repository_config.hpp" #include "src/buildtool/common/statistics.hpp" @@ -51,6 +50,7 @@ #include "src/buildtool/file_system/file_system_manager.hpp" #include "src/buildtool/file_system/object_type.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" #include "test/utils/remote_execution/test_remote_config.hpp" @@ -70,12 +70,10 @@ static inline void RunBlobUpload(RepositoryConfig* repo_config, auto api = factory(); HashFunction const hash_function{TestHashType::ReadFromEnvironment()}; - std::string const blob = "test"; - CHECK(api->Upload( - {ArtifactBlob{ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, blob), - blob, - /*is_exec=*/false}})); + auto const blob = + ArtifactBlob::FromMemory(hash_function, ObjectType::File, "test"); + REQUIRE(blob.has_value()); + CHECK(api->Upload({*blob})); } [[nodiscard]] static inline auto GetTestDir() -> std::filesystem::path { @@ -424,23 +422,22 @@ static inline void TestUploadAndDownloadTrees( HashFunction const hash_function{TestHashType::ReadFromEnvironment()}; - auto foo = std::string{"foo"}; - auto bar = std::string{"bar"}; - auto const foo_digest = - ArtifactDigestFactory::HashDataAs<ObjectType::File>(hash_function, foo); - auto const bar_digest = - ArtifactDigestFactory::HashDataAs<ObjectType::File>(hash_function, bar); + auto const foo = + ArtifactBlob::FromMemory(hash_function, ObjectType::File, "foo"); + REQUIRE(foo.has_value()); + auto const bar = + ArtifactBlob::FromMemory(hash_function, ObjectType::File, "bar"); + REQUIRE(bar.has_value()); // upload blobs auto api = factory(); - REQUIRE(api->Upload({ArtifactBlob{foo_digest, foo, /*is_exec=*/false}, - ArtifactBlob{bar_digest, bar, /*is_exec=*/false}})); + REQUIRE(api->Upload({*foo, *bar})); // define known artifacts auto foo_desc = - ArtifactDescription::CreateKnown(foo_digest, ObjectType::File); + ArtifactDescription::CreateKnown(foo->GetDigest(), ObjectType::File); auto bar_desc = - ArtifactDescription::CreateKnown(bar_digest, ObjectType::Symlink); + ArtifactDescription::CreateKnown(bar->GetDigest(), ObjectType::Symlink); DependencyGraph g{}; auto foo_id = g.AddArtifact(foo_desc); diff --git a/test/utils/TARGETS b/test/utils/TARGETS index 10245604..183c5009 100644 --- a/test/utils/TARGETS +++ b/test/utils/TARGETS @@ -24,6 +24,7 @@ , ["@", "src", "src/buildtool/execution_api/remote", "bazel_network"] , ["@", "src", "src/buildtool/execution_api/remote", "config"] , ["@", "src", "src/buildtool/file_system", "object_type"] + , ["@", "src", "src/utils/cpp", "expected"] ] , "stage": ["test", "utils"] } diff --git a/test/utils/remote_execution/bazel_action_creator.hpp b/test/utils/remote_execution/bazel_action_creator.hpp index 30dcc20a..69a5b172 100644 --- a/test/utils/remote_execution/bazel_action_creator.hpp +++ b/test/utils/remote_execution/bazel_action_creator.hpp @@ -37,6 +37,7 @@ #include "src/buildtool/execution_api/remote/bazel/bazel_cas_client.hpp" #include "src/buildtool/execution_api/remote/config.hpp" #include "src/buildtool/file_system/object_type.hpp" +#include "src/utils/cpp/expected.hpp" #include "test/utils/remote_execution/test_auth_config.hpp" #include "test/utils/remote_execution/test_remote_config.hpp" @@ -71,27 +72,34 @@ return env_var_message; }); - auto cmd_data = cmd.SerializeAsString(); - auto cmd_id = ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, cmd_data); - blobs.emplace(cmd_id, cmd_data, /*is_exec=*/false); + auto const cmd_blob = ArtifactBlob::FromMemory( + hash_function, ObjectType::File, cmd.SerializeAsString()); + if (not cmd_blob.has_value()) { + return nullptr; + } + blobs.emplace(*cmd_blob); - bazel_re::Directory empty_dir; - auto dir_data = empty_dir.SerializeAsString(); - auto dir_id = ArtifactDigestFactory::HashDataAs<ObjectType::Tree>( - hash_function, dir_data); - blobs.emplace(dir_id, dir_data, /*is_exec=*/false); + bazel_re::Directory const empty_dir; + auto const dir_blob = ArtifactBlob::FromMemory( + hash_function, ObjectType::Tree, empty_dir.SerializeAsString()); + if (not dir_blob.has_value()) { + return nullptr; + } + blobs.emplace(*dir_blob); bazel_re::Action action; - (*action.mutable_command_digest()) = ArtifactDigestFactory::ToBazel(cmd_id); + (*action.mutable_command_digest()) = + ArtifactDigestFactory::ToBazel(cmd_blob->GetDigest()); action.set_do_not_cache(false); (*action.mutable_input_root_digest()) = - ArtifactDigestFactory::ToBazel(dir_id); + ArtifactDigestFactory::ToBazel(dir_blob->GetDigest()); - auto action_data = action.SerializeAsString(); - auto action_id = ArtifactDigestFactory::HashDataAs<ObjectType::File>( - hash_function, action_data); - blobs.emplace(action_id, action_data, /*is_exec=*/false); + auto const action_blob = ArtifactBlob::FromMemory( + hash_function, ObjectType::File, action.SerializeAsString()); + if (not action_blob.has_value()) { + return nullptr; + } + blobs.emplace(*action_blob); auto auth_config = TestAuthConfig::ReadFromEnvironment(); if (not auth_config) { @@ -117,7 +125,7 @@ if (cas_client.BatchUpdateBlobs(instance_name, blobs) == blobs.size()) { return std::make_unique<bazel_re::Digest>( - ArtifactDigestFactory::ToBazel(action_id)); + ArtifactDigestFactory::ToBazel(action_blob->GetDigest())); } return nullptr; } |