From b393090c220da61dd1c197adfac42dc47ad74f8a Mon Sep 17 00:00:00 2001 From: Paul Cristian Sarbu Date: Mon, 29 Jul 2024 11:13:08 +0200 Subject: ApiBundle: Use a creator method instead of constructor This will allow for ApiBundle to be used together with the TestApi implementation of IExecutionApi in tests. Also rename CreateRemote method to MakeRemote in order to remove any semantical confusion. --- src/buildtool/execution_api/common/api_bundle.cpp | 35 +++++++++++++++++------ src/buildtool/execution_api/common/api_bundle.hpp | 10 ++++--- src/buildtool/graph_traverser/graph_traverser.hpp | 6 ++-- src/buildtool/main/main.cpp | 18 ++++++------ src/buildtool/serve_api/serve_service/target.cpp | 4 +-- src/other_tools/just_mr/fetch.cpp | 6 ++-- src/other_tools/just_mr/setup.cpp | 6 ++-- 7 files changed, 53 insertions(+), 32 deletions(-) (limited to 'src') diff --git a/src/buildtool/execution_api/common/api_bundle.cpp b/src/buildtool/execution_api/common/api_bundle.cpp index 15e84440..8808e5d8 100644 --- a/src/buildtool/execution_api/common/api_bundle.cpp +++ b/src/buildtool/execution_api/common/api_bundle.cpp @@ -18,16 +18,33 @@ #include "src/buildtool/execution_api/local/local_api.hpp" #include "src/buildtool/execution_api/remote/bazel/bazel_api.hpp" -ApiBundle::ApiBundle(gsl::not_null const& local_context, - gsl::not_null const& remote_context, - RepositoryConfig const* repo_config) - : hash_function{local_context->storage_config->hash_function}, - local{std::make_shared(local_context, repo_config)}, - remote{CreateRemote(remote_context->exec_config->remote_address, - remote_context->auth, - remote_context->retry_config)} {} +/// \note Some logic from MakeRemote is duplicated here as that method cannot +/// be used without the hash_function field being set prior to the call. +auto ApiBundle::Create( + gsl::not_null const& local_context, + gsl::not_null const& remote_context, + RepositoryConfig const* repo_config) -> ApiBundle { + auto const hash_fct = local_context->storage_config->hash_function; + IExecutionApi::Ptr local_api = + std::make_shared(local_context, repo_config); + IExecutionApi::Ptr remote_api = local_api; + if (auto const address = remote_context->exec_config->remote_address) { + ExecutionConfiguration config; + config.skip_cache_lookup = false; + remote_api = std::make_shared("remote-execution", + address->host, + address->port, + remote_context->auth, + remote_context->retry_config, + config, + hash_fct); + } + return ApiBundle{.hash_function = hash_fct, + .local = std::move(local_api), + .remote = std::move(remote_api)}; +} -auto ApiBundle::CreateRemote( +auto ApiBundle::MakeRemote( std::optional const& address, gsl::not_null const& authentication, gsl::not_null const& retry_config) const diff --git a/src/buildtool/execution_api/common/api_bundle.hpp b/src/buildtool/execution_api/common/api_bundle.hpp index 17c64d1e..28595010 100644 --- a/src/buildtool/execution_api/common/api_bundle.hpp +++ b/src/buildtool/execution_api/common/api_bundle.hpp @@ -29,10 +29,13 @@ /// same time. If the remote api cannot be instantiated, it falls back to /// exactly the same instance that local api is (&*remote == & *local). struct ApiBundle final { - explicit ApiBundle( + /// \brief Create an ApiBundle instance. + /// \note A creator method is used instead of a constructor to allow for + /// tests to instantiate ApiBundles with own implementations of the APIs. + [[nodiscard]] static auto Create( gsl::not_null const& local_context, gsl::not_null const& remote_context, - RepositoryConfig const* repo_config); + RepositoryConfig const* repo_config) -> ApiBundle; /// \brief Create a Remote object based on the given arguments. /// \param address The endpoint address. @@ -40,13 +43,12 @@ struct ApiBundle final { /// \param retry_config The retry strategy configuration. /// \returns A configured api: BazelApi if a remote address is given, /// otherwise fall back to the already configured LocalApi instance. - [[nodiscard]] auto CreateRemote( + [[nodiscard]] auto MakeRemote( std::optional const& address, gsl::not_null const& authentication, gsl::not_null const& retry_config) const -> gsl::not_null; - // Needed to be set before creating the remote (via CreateRemote) HashFunction const hash_function; // 7 bytes of alignment. diff --git a/src/buildtool/graph_traverser/graph_traverser.hpp b/src/buildtool/graph_traverser/graph_traverser.hpp index a63f7d8c..301ef722 100644 --- a/src/buildtool/graph_traverser/graph_traverser.hpp +++ b/src/buildtool/graph_traverser/graph_traverser.hpp @@ -385,9 +385,9 @@ class GraphTraverser { std::vector const& artifact_ids) const -> bool { // setup rebuilder with api for cache endpoint auto api_cached = - apis_.CreateRemote(remote_context_.exec_config->cache_address, - remote_context_.auth, - remote_context_.retry_config); + apis_.MakeRemote(remote_context_.exec_config->cache_address, + remote_context_.auth, + remote_context_.retry_config); Rebuilder executor{repo_config_, &*apis_.local, &*apis_.remote, diff --git a/src/buildtool/main/main.cpp b/src/buildtool/main/main.cpp index 18caa871..e97b5a27 100644 --- a/src/buildtool/main/main.cpp +++ b/src/buildtool/main/main.cpp @@ -822,9 +822,10 @@ auto main(int argc, char* argv[]) -> int { .retry_config = &retry_config, .exec_config = &remote_exec_config}; - ApiBundle const exec_apis{&local_context, - &remote_context, - /*repo_config=*/nullptr}; + auto const exec_apis = + ApiBundle::Create(&local_context, + &remote_context, + /*repo_config=*/nullptr); return execution_server->Run(&local_context, &remote_context, @@ -888,9 +889,10 @@ auto main(int argc, char* argv[]) -> int { .retry_config = &*retry_config, .exec_config = &*remote_exec_config}; - ApiBundle const serve_apis{&local_context, - &remote_context, - /*repo_config=*/nullptr}; + auto const serve_apis = + ApiBundle::Create(&local_context, + &remote_context, + /*repo_config=*/nullptr); auto serve = ServeApi::Create(*serve_config, &local_context, &remote_context, @@ -988,8 +990,8 @@ auto main(int argc, char* argv[]) -> int { .retry_config = &*retry_config, .exec_config = &*remote_exec_config}; - ApiBundle const main_apis{ - &local_context, &remote_context, &repo_config}; + auto const main_apis = + ApiBundle::Create(&local_context, &remote_context, &repo_config); GraphTraverser const traverser{ {jobs, std::move(arguments.build), diff --git a/src/buildtool/serve_api/serve_service/target.cpp b/src/buildtool/serve_api/serve_service/target.cpp index ec8ff458..9ad1520b 100644 --- a/src/buildtool/serve_api/serve_service/target.cpp +++ b/src/buildtool/serve_api/serve_service/target.cpp @@ -501,8 +501,8 @@ auto TargetService::ServeTarget( // Use a new ApiBundle that knows about local repository config and // dispatch endpoint for traversing - ApiBundle const local_apis{ - &local_context_, &dispatch_context, &repository_config}; + auto const local_apis = ApiBundle::Create( + &local_context_, &dispatch_context, &repository_config); GraphTraverser const traverser{ std::move(traverser_args), diff --git a/src/other_tools/just_mr/fetch.cpp b/src/other_tools/just_mr/fetch.cpp index 088c9ace..d236b7e8 100644 --- a/src/other_tools/just_mr/fetch.cpp +++ b/src/other_tools/just_mr/fetch.cpp @@ -440,9 +440,9 @@ auto MultiRepoFetch(std::shared_ptr const& config, .exec_config = &*remote_exec_config}; // setup the APIs for archive fetches; only happens if in native mode - ApiBundle const apis{&local_context, - &remote_context, - /*repo_config=*/nullptr}; + auto const apis = ApiBundle::Create(&local_context, + &remote_context, + /*repo_config=*/nullptr); bool const has_remote_api = apis.local != apis.remote and not common_args.compatible; diff --git a/src/other_tools/just_mr/setup.cpp b/src/other_tools/just_mr/setup.cpp index 9f9e09bd..ba5e39b4 100644 --- a/src/other_tools/just_mr/setup.cpp +++ b/src/other_tools/just_mr/setup.cpp @@ -161,9 +161,9 @@ auto MultiRepoSetup(std::shared_ptr const& config, .retry_config = &*retry_config, .exec_config = &*remote_exec_config}; - ApiBundle const apis{&local_context, - &remote_context, - /*repo_config=*/nullptr}; + auto const apis = ApiBundle::Create(&local_context, + &remote_context, + /*repo_config=*/nullptr); bool const has_remote_api = apis.local != apis.remote and not common_args.compatible; -- cgit v1.2.3