diff options
-rw-r--r-- | src/buildtool/main/main.cpp | 19 | ||||
-rw-r--r-- | src/buildtool/storage/config.hpp | 40 | ||||
-rw-r--r-- | src/other_tools/just_mr/TARGETS | 1 | ||||
-rw-r--r-- | src/other_tools/just_mr/main.cpp | 11 | ||||
-rw-r--r-- | test/buildtool/build_engine/target_map/target_map.test.cpp | 8 | ||||
-rw-r--r-- | test/buildtool/execution_api/execution_service/TARGETS | 1 | ||||
-rw-r--r-- | test/buildtool/execution_api/execution_service/cas_server.test.cpp | 5 | ||||
-rw-r--r-- | test/buildtool/execution_api/local/TARGETS | 1 | ||||
-rw-r--r-- | test/buildtool/execution_api/local/local_execution.test.cpp | 5 | ||||
-rw-r--r-- | test/buildtool/graph_traverser/TARGETS | 1 | ||||
-rw-r--r-- | test/buildtool/graph_traverser/graph_traverser_remote.test.cpp | 4 | ||||
-rw-r--r-- | test/buildtool/storage/TARGETS | 2 | ||||
-rw-r--r-- | test/buildtool/storage/local_ac.test.cpp | 13 | ||||
-rw-r--r-- | test/buildtool/storage/local_cas.test.cpp | 8 | ||||
-rw-r--r-- | test/utils/TARGETS | 1 | ||||
-rw-r--r-- | test/utils/hermeticity/test_storage_config.hpp | 7 |
16 files changed, 80 insertions, 47 deletions
diff --git a/src/buildtool/main/main.cpp b/src/buildtool/main/main.cpp index a3be587b..9f10b8bc 100644 --- a/src/buildtool/main/main.cpp +++ b/src/buildtool/main/main.cpp @@ -110,10 +110,10 @@ void SetupLogging(LogArguments const& clargs) { [[nodiscard]] auto CreateStorageConfig( EndpointArguments const& eargs, + bool is_compatible, std::optional<ServerAddress> const& remote_address = std::nullopt, - std::map<std::string, std::string> const& remote_platform_properties = {}, - std::vector<std::pair<std::map<std::string, std::string>, - ServerAddress>> const& remote_dispatch = {}) noexcept + ExecutionProperties const& remote_platform_properties = {}, + std::vector<DispatchEndpoint> const& remote_dispatch = {}) noexcept -> std::optional<StorageConfig> { StorageConfig::Builder builder; if (eargs.local_root.has_value()) { @@ -122,6 +122,8 @@ void SetupLogging(LogArguments const& clargs) { auto config = builder + .SetHashType(is_compatible ? HashFunction::JustHash::Compatible + : HashFunction::JustHash::Native) .SetRemoteExecutionArgs( remote_address, remote_platform_properties, remote_dispatch) .Build(); @@ -800,7 +802,8 @@ auto main(int argc, char* argv[]) -> int { if (arguments.cmd == SubCommand::kGc) { // Set up storage for GC, as we have all the config args we need. - auto const storage_config = CreateStorageConfig(arguments.endpoint); + auto const storage_config = CreateStorageConfig( + arguments.endpoint, Compatibility::IsCompatible()); if (not storage_config) { return kExitFailure; } @@ -828,7 +831,8 @@ auto main(int argc, char* argv[]) -> int { RemoteExecutionConfig remote_exec_config{}; // Set up storage for local execution. - auto const storage_config = CreateStorageConfig(arguments.endpoint); + auto const storage_config = CreateStorageConfig( + arguments.endpoint, Compatibility::IsCompatible()); if (not storage_config) { return kExitFailure; } @@ -885,6 +889,7 @@ auto main(int argc, char* argv[]) -> int { // Set up storage for serve operation. auto const storage_config = CreateStorageConfig(arguments.endpoint, + Compatibility::IsCompatible(), remote_exec_config->remote_address, remote_exec_config->platform_properties, remote_exec_config->dispatch); @@ -950,13 +955,15 @@ auto main(int argc, char* argv[]) -> int { // correctly-sharded target cache. auto const storage_config = CreateStorageConfig(arguments.endpoint, + Compatibility::IsCompatible(), remote_exec_config->remote_address, remote_exec_config->platform_properties, remote_exec_config->dispatch); #else // For bootstrapping the TargetCache sharding is not needed, so we can // default all execution arguments. - auto const storage_config = CreateStorageConfig(arguments.endpoint); + auto const storage_config = CreateStorageConfig( + arguments.endpoint, Compatibility::IsCompatible()); #endif // BOOTSTRAP_BUILD_TOOL if (not storage_config) { return kExitFailure; diff --git a/src/buildtool/storage/config.hpp b/src/buildtool/storage/config.hpp index a6213074..5861977e 100644 --- a/src/buildtool/storage/config.hpp +++ b/src/buildtool/storage/config.hpp @@ -65,6 +65,8 @@ struct StorageConfig final { // Number of total storage generations (default: two generations). std::size_t const num_generations = 2; + HashFunction const hash_function{HashFunction::JustHash::Native}; + // Hash of the execution backend description std::string const backend_description_id = DefaultBackendDescriptionId(); @@ -154,11 +156,10 @@ struct StorageConfig final { return dir / (is_compatible ? "compatible-sha256" : "git-sha1"); }; - [[nodiscard]] static auto DefaultBackendDescriptionId() noexcept - -> std::string { + [[nodiscard]] auto DefaultBackendDescriptionId() noexcept -> std::string { try { return ArtifactDigest::Create<ObjectType::File>( - HashFunction::Instance(), + hash_function, DescribeBackend(std::nullopt, {}, {}).value()) .hash(); } catch (...) { @@ -180,11 +181,16 @@ class StorageConfig::Builder final { return *this; } - auto SetRemoteExecutionArgs( - std::optional<ServerAddress> address, - std::map<std::string, std::string> properties, - std::vector<std::pair<std::map<std::string, std::string>, - ServerAddress>> dispatch) noexcept -> Builder& { + /// \brief Specify the type of the hash function + auto SetHashType(HashFunction::JustHash value) noexcept -> Builder& { + hash_type_ = value; + return *this; + } + + auto SetRemoteExecutionArgs(std::optional<ServerAddress> address, + ExecutionProperties properties, + std::vector<DispatchEndpoint> dispatch) noexcept + -> Builder& { remote_address_ = std::move(address); remote_platform_properties_ = std::move(properties); remote_dispatch_ = std::move(dispatch); @@ -216,14 +222,19 @@ class StorageConfig::Builder final { } } + auto hash_function = default_config.hash_function; + if (hash_type_.has_value()) { + hash_function = HashFunction{*hash_type_}; + } + // Hash the execution backend description auto backend_description_id = default_config.backend_description_id; auto desc = DescribeBackend( remote_address_, remote_platform_properties_, remote_dispatch_); if (desc) { - backend_description_id = ArtifactDigest::Create<ObjectType::File>( - HashFunction::Instance(), *desc) - .hash(); + backend_description_id = + ArtifactDigest::Create<ObjectType::File>(hash_function, *desc) + .hash(); } else { return unexpected{desc.error()}; @@ -232,18 +243,19 @@ class StorageConfig::Builder final { return StorageConfig{ .build_root = std::move(build_root), .num_generations = num_generations, + .hash_function = hash_function, .backend_description_id = std::move(backend_description_id)}; } private: std::optional<std::filesystem::path> build_root_; std::optional<std::size_t> num_generations_; + std::optional<HashFunction::JustHash> hash_type_; // Fields for computing remote execution backend description std::optional<ServerAddress> remote_address_; - std::map<std::string, std::string> remote_platform_properties_; - std::vector<std::pair<std::map<std::string, std::string>, ServerAddress>> - remote_dispatch_; + ExecutionProperties remote_platform_properties_; + std::vector<DispatchEndpoint> remote_dispatch_; }; #endif // INCLUDED_SRC_BUILDTOOL_STORAGE_CONFIG_HPP diff --git a/src/other_tools/just_mr/TARGETS b/src/other_tools/just_mr/TARGETS index 17e9b248..af4ef8df 100644 --- a/src/other_tools/just_mr/TARGETS +++ b/src/other_tools/just_mr/TARGETS @@ -25,6 +25,7 @@ , "setup" , "setup_utils" , "update" + , ["src/buildtool/crypto", "hash_function"] ] , "stage": ["src", "other_tools", "just_mr"] , "private-ldflags": diff --git a/src/other_tools/just_mr/main.cpp b/src/other_tools/just_mr/main.cpp index f104e730..698a6ccb 100644 --- a/src/other_tools/just_mr/main.cpp +++ b/src/other_tools/just_mr/main.cpp @@ -28,6 +28,7 @@ #include "src/buildtool/build_engine/expression/configuration.hpp" #include "src/buildtool/common/retry_cli.hpp" #include "src/buildtool/compatibility/compatibility.hpp" +#include "src/buildtool/crypto/hash_function.hpp" #include "src/buildtool/file_system/git_context.hpp" #include "src/buildtool/logging/log_config.hpp" #include "src/buildtool/logging/log_level.hpp" @@ -206,13 +207,16 @@ void SetupLogging(MultiRepoLogArguments const& clargs) { } } -[[nodiscard]] auto CreateStorageConfig( - MultiRepoCommonArguments const& args) noexcept +[[nodiscard]] auto CreateStorageConfig(MultiRepoCommonArguments const& args, + bool is_compatible) noexcept -> std::optional<StorageConfig> { StorageConfig::Builder builder; if (args.just_mr_paths->root.has_value()) { builder.SetBuildRoot(*args.just_mr_paths->root); } + builder.SetHashType(is_compatible ? HashFunction::JustHash::Compatible + : HashFunction::JustHash::Native); + // As just-mr does not require the TargetCache, we do not need to set any of // the remote execution fields for the backend description. auto config = builder.Build(); @@ -314,7 +318,8 @@ auto main(int argc, char* argv[]) -> int { // Setup LocalStorageConfig to store the local_build_root properly // and make the cas and git cache roots available - auto storage_config = CreateStorageConfig(arguments.common); + auto storage_config = CreateStorageConfig( + arguments.common, Compatibility::IsCompatible()); if (not storage_config) { Logger::Log(LogLevel::Error, "Failed to configure local build root."); diff --git a/test/buildtool/build_engine/target_map/target_map.test.cpp b/test/buildtool/build_engine/target_map/target_map.test.cpp index 0880422f..c64bb306 100644 --- a/test/buildtool/build_engine/target_map/target_map.test.cpp +++ b/test/buildtool/build_engine/target_map/target_map.test.cpp @@ -961,10 +961,14 @@ TEST_CASE("built-in rules", "[target_map]") { CHECK(error_msg == "NONE"); CHECK(bar_result->Artifacts()->ToJson()["foo.txt."]["type"] == "KNOWN"); CHECK(bar_result->Artifacts()->ToJson()["foo.txt."]["data"]["id"] == - HashFunction::Instance().ComputeBlobHash("bar").HexString()); + storage_config.Get() + .hash_function.ComputeBlobHash("bar") + .HexString()); CHECK(baz_result->Artifacts()->ToJson()["foo.txt."]["type"] == "KNOWN"); CHECK(baz_result->Artifacts()->ToJson()["foo.txt."]["data"]["id"] == - HashFunction::Instance().ComputeBlobHash("baz").HexString()); + storage_config.Get() + .hash_function.ComputeBlobHash("baz") + .HexString()); } } diff --git a/test/buildtool/execution_api/execution_service/TARGETS b/test/buildtool/execution_api/execution_service/TARGETS index f50057c7..754615d5 100644 --- a/test/buildtool/execution_api/execution_service/TARGETS +++ b/test/buildtool/execution_api/execution_service/TARGETS @@ -17,7 +17,6 @@ , ["@", "src", "src/buildtool/storage", "config"] , ["@", "src", "src/buildtool/storage", "storage"] , ["@", "gsl", "", "gsl"] - , ["@", "src", "src/buildtool/crypto", "hash_function"] ] , "stage": ["test", "buildtool", "execution_api", "execution_service"] } diff --git a/test/buildtool/execution_api/execution_service/cas_server.test.cpp b/test/buildtool/execution_api/execution_service/cas_server.test.cpp index 845d94aa..3e566861 100644 --- a/test/buildtool/execution_api/execution_service/cas_server.test.cpp +++ b/test/buildtool/execution_api/execution_service/cas_server.test.cpp @@ -17,7 +17,6 @@ #include "catch2/catch_test_macros.hpp" #include "gsl/gsl" #include "src/buildtool/common/artifact_digest.hpp" -#include "src/buildtool/crypto/hash_function.hpp" #include "src/buildtool/execution_api/execution_service/cas_server.hpp" #include "src/buildtool/file_system/git_repo.hpp" #include "src/buildtool/file_system/object_type.hpp" @@ -59,7 +58,7 @@ TEST_CASE("CAS Service: upload incomplete tree", "[execution_service]") { auto empty_tree = GitRepo::CreateShallowTree(empty_entries); REQUIRE(empty_tree); auto empty_tree_digest = ArtifactDigest::Create<ObjectType::Tree>( - HashFunction::Instance(), empty_tree->second); + storage_config.Get().hash_function, empty_tree->second); // Create a tree containing the empty tree. auto entries = GitRepo::tree_entries_t{}; @@ -67,7 +66,7 @@ TEST_CASE("CAS Service: upload incomplete tree", "[execution_service]") { auto tree = GitRepo::CreateShallowTree(entries); REQUIRE(tree); auto tree_digest = ArtifactDigest::Create<ObjectType::Tree>( - HashFunction::Instance(), tree->second); + storage_config.Get().hash_function, tree->second); // Upload tree. The tree invariant is violated, thus, a negative answer is // expected. diff --git a/test/buildtool/execution_api/local/TARGETS b/test/buildtool/execution_api/local/TARGETS index 212bf0b6..41870428 100644 --- a/test/buildtool/execution_api/local/TARGETS +++ b/test/buildtool/execution_api/local/TARGETS @@ -15,7 +15,6 @@ , ["utils", "test_storage_config"] , ["@", "src", "src/buildtool/storage", "storage"] , ["@", "src", "src/buildtool/storage", "config"] - , ["@", "src", "src/buildtool/crypto", "hash_function"] ] , "stage": ["test", "buildtool", "execution_api", "local"] } diff --git a/test/buildtool/execution_api/local/local_execution.test.cpp b/test/buildtool/execution_api/local/local_execution.test.cpp index cac83b2a..8aea5950 100644 --- a/test/buildtool/execution_api/local/local_execution.test.cpp +++ b/test/buildtool/execution_api/local/local_execution.test.cpp @@ -21,7 +21,6 @@ #include "catch2/catch_test_macros.hpp" #include "src/buildtool/common/artifact_description.hpp" #include "src/buildtool/common/repository_config.hpp" -#include "src/buildtool/crypto/hash_function.hpp" #include "src/buildtool/execution_api/local/config.hpp" #include "src/buildtool/execution_api/local/local_api.hpp" #include "src/buildtool/file_system/file_system_manager.hpp" @@ -174,7 +173,7 @@ TEST_CASE("LocalExecution: No input, create output", "[execution_api]") { std::string test_content("test"); auto test_digest = ArtifactDigest::Create<ObjectType::File>( - HashFunction::Instance(), test_content); + storage_config.Get().hash_function, test_content); std::string output_path{"output_file"}; std::vector<std::string> const cmdline = { @@ -234,7 +233,7 @@ TEST_CASE("LocalExecution: One input copied to output", "[execution_api]") { std::string test_content("test"); auto test_digest = ArtifactDigest::Create<ObjectType::File>( - HashFunction::Instance(), test_content); + storage_config.Get().hash_function, test_content); REQUIRE(api.Upload(ArtifactBlobContainer{{ArtifactBlob{ test_digest, test_content, /*is_exec=*/false}}}, false)); diff --git a/test/buildtool/graph_traverser/TARGETS b/test/buildtool/graph_traverser/TARGETS index 4b1a531f..0ed5533f 100644 --- a/test/buildtool/graph_traverser/TARGETS +++ b/test/buildtool/graph_traverser/TARGETS @@ -52,6 +52,7 @@ , ["utils", "test_remote_config"] , ["@", "src", "src/buildtool/storage", "storage"] , ["@", "src", "src/buildtool/storage", "config"] + , ["@", "src", "src/buildtool/crypto", "hash_function"] ] , "stage": ["test", "buildtool", "graph_traverser"] } diff --git a/test/buildtool/graph_traverser/graph_traverser_remote.test.cpp b/test/buildtool/graph_traverser/graph_traverser_remote.test.cpp index 504b96a1..fa8202ce 100644 --- a/test/buildtool/graph_traverser/graph_traverser_remote.test.cpp +++ b/test/buildtool/graph_traverser/graph_traverser_remote.test.cpp @@ -13,6 +13,7 @@ // limitations under the License. #include "catch2/catch_test_macros.hpp" +#include "src/buildtool/crypto/hash_function.hpp" #include "src/buildtool/execution_api/remote/config.hpp" #include "src/buildtool/storage/config.hpp" #include "src/buildtool/storage/storage.hpp" @@ -33,6 +34,9 @@ StorageConfig::Builder builder; auto config = builder.SetBuildRoot(cache_dir) + .SetHashType(Compatibility::IsCompatible() + ? HashFunction::JustHash::Compatible + : HashFunction::JustHash::Native) .SetRemoteExecutionArgs(remote_config.remote_address, remote_config.platform_properties, remote_config.dispatch) diff --git a/test/buildtool/storage/TARGETS b/test/buildtool/storage/TARGETS index a2b0b0d2..4a2881c3 100644 --- a/test/buildtool/storage/TARGETS +++ b/test/buildtool/storage/TARGETS @@ -24,7 +24,6 @@ , ["utils", "blob_creator"] , ["@", "src", "src/buildtool/storage", "storage"] , ["@", "src", "src/buildtool/storage", "config"] - , ["@", "src", "src/buildtool/crypto", "hash_function"] ] , "stage": ["test", "buildtool", "storage"] } @@ -42,7 +41,6 @@ , ["@", "src", "src/buildtool/storage", "storage"] , ["@", "src", "src/buildtool/storage", "config"] , ["utils", "test_storage_config"] - , ["@", "src", "src/buildtool/crypto", "hash_function"] ] , "stage": ["test", "buildtool", "storage"] } diff --git a/test/buildtool/storage/local_ac.test.cpp b/test/buildtool/storage/local_ac.test.cpp index b3a4fcd1..c692a75c 100644 --- a/test/buildtool/storage/local_ac.test.cpp +++ b/test/buildtool/storage/local_ac.test.cpp @@ -18,7 +18,6 @@ #include "gsl/gsl" #include "src/buildtool/common/artifact_digest.hpp" #include "src/buildtool/common/bazel_types.hpp" -#include "src/buildtool/crypto/hash_function.hpp" #include "src/buildtool/file_system/file_system_manager.hpp" #include "src/buildtool/file_system/object_type.hpp" #include "src/buildtool/storage/config.hpp" @@ -38,7 +37,7 @@ TEST_CASE("LocalAC: Single action, single result", "[storage]") { auto const& cas = storage.CAS(); auto action_id = ArtifactDigest::Create<ObjectType::File>( - HashFunction::Instance(), "action"); + storage_config.Get().hash_function, "action"); CHECK(not ac.CachedResult(action_id)); CHECK(RunDummyExecution(&ac, &cas, action_id, "result")); auto ac_result = ac.CachedResult(action_id); @@ -53,9 +52,9 @@ TEST_CASE("LocalAC: Two different actions, two different results", auto const& cas = storage.CAS(); auto action_id1 = ArtifactDigest::Create<ObjectType::File>( - HashFunction::Instance(), "action1"); + storage_config.Get().hash_function, "action1"); auto action_id2 = ArtifactDigest::Create<ObjectType::File>( - HashFunction::Instance(), "action2"); + storage_config.Get().hash_function, "action2"); CHECK(not ac.CachedResult(action_id1)); CHECK(not ac.CachedResult(action_id2)); @@ -84,9 +83,9 @@ TEST_CASE("LocalAC: Two different actions, same two results", "[storage]") { auto const& cas = storage.CAS(); auto action_id1 = ArtifactDigest::Create<ObjectType::File>( - HashFunction::Instance(), "action1"); + storage_config.Get().hash_function, "action1"); auto action_id2 = ArtifactDigest::Create<ObjectType::File>( - HashFunction::Instance(), "action2"); + storage_config.Get().hash_function, "action2"); CHECK(not ac.CachedResult(action_id1)); CHECK(not ac.CachedResult(action_id2)); @@ -115,7 +114,7 @@ TEST_CASE("LocalAC: Same two actions, two different results", "[storage]") { auto const& cas = storage.CAS(); auto action_id = ArtifactDigest::Create<ObjectType::File>( - HashFunction::Instance(), "same action"); + storage_config.Get().hash_function, "same action"); CHECK(not ac.CachedResult(action_id)); std::string result_content1{}; diff --git a/test/buildtool/storage/local_cas.test.cpp b/test/buildtool/storage/local_cas.test.cpp index c1d65b8e..b4ede8ca 100644 --- a/test/buildtool/storage/local_cas.test.cpp +++ b/test/buildtool/storage/local_cas.test.cpp @@ -17,7 +17,6 @@ #include "catch2/catch_test_macros.hpp" #include "src/buildtool/common/artifact_digest.hpp" -#include "src/buildtool/crypto/hash_function.hpp" #include "src/buildtool/execution_api/bazel_msg/bazel_blob_container.hpp" #include "src/buildtool/file_system/file_system_manager.hpp" #include "src/buildtool/file_system/object_type.hpp" @@ -34,7 +33,7 @@ TEST_CASE("LocalCAS: Add blob to storage from bytes", "[storage]") { std::string test_bytes("test"); auto test_digest = ArtifactDigest::Create<ObjectType::File>( - HashFunction::Instance(), test_bytes); + storage_config.Get().hash_function, test_bytes); // check blob not in storage CHECK(not cas.BlobPath(test_digest, true)); @@ -85,7 +84,7 @@ TEST_CASE("LocalCAS: Add blob to storage from non-executable file", "test/buildtool/storage/data/non_executable_file"}; auto test_blob = - CreateBlobFromPath(non_exec_file, HashFunction::Instance()); + CreateBlobFromPath(non_exec_file, storage_config.Get().hash_function); REQUIRE(test_blob); // check blob not in storage @@ -135,7 +134,8 @@ TEST_CASE("LocalCAS: Add blob to storage from executable file", "[storage]") { std::filesystem::path exec_file{ "test/buildtool/storage/data/executable_file"}; - auto test_blob = CreateBlobFromPath(exec_file, HashFunction::Instance()); + auto test_blob = + CreateBlobFromPath(exec_file, storage_config.Get().hash_function); REQUIRE(test_blob); // check blob not in storage diff --git a/test/utils/TARGETS b/test/utils/TARGETS index 13ad7626..58353130 100644 --- a/test/utils/TARGETS +++ b/test/utils/TARGETS @@ -52,6 +52,7 @@ , ["@", "src", "src/buildtool/logging", "logging"] , ["@", "src", "src/buildtool/storage", "config"] , ["@", "src", "src/utils/cpp", "tmp_dir"] + , ["@", "src", "src/buildtool/crypto", "hash_function"] ] , "stage": ["test", "utils"] } diff --git a/test/utils/hermeticity/test_storage_config.hpp b/test/utils/hermeticity/test_storage_config.hpp index e5346c75..26dc53df 100644 --- a/test/utils/hermeticity/test_storage_config.hpp +++ b/test/utils/hermeticity/test_storage_config.hpp @@ -21,6 +21,7 @@ #include <utility> //std::move #include "gsl/gsl" +#include "src/buildtool/crypto/hash_function.hpp" #include "src/buildtool/logging/log_level.hpp" #include "src/buildtool/logging/logger.hpp" #include "src/buildtool/storage/config.hpp" @@ -51,7 +52,11 @@ class TestStorageConfig final { } StorageConfig::Builder builder; - auto config = builder.SetBuildRoot(temp_dir->GetPath()).Build(); + auto config = builder.SetBuildRoot(temp_dir->GetPath()) + .SetHashType(Compatibility::IsCompatible() + ? HashFunction::JustHash::Compatible + : HashFunction::JustHash::Native) + .Build(); if (not config) { Logger::Log(LogLevel::Error, config.error()); std::exit(EXIT_FAILURE); |