From 35fe9c1e07464de85ea8138c574c0bc7d07c5a48 Mon Sep 17 00:00:00 2001 From: Paul Cristian Sarbu Date: Mon, 29 Jul 2024 12:49:31 +0200 Subject: executor: Use ApiBundle ...instead of separate local and remote instances. For tests, where different implementations of the IExecutionApi interface are used, ApiBundle instances are created by explicitly setting the struct fields instead of using ApiBundle::Create. --- .../execution_engine/executor/executor.hpp | 73 +++++++++------------- src/buildtool/graph_traverser/graph_traverser.hpp | 12 +--- test/buildtool/execution_engine/executor/TARGETS | 2 + .../execution_engine/executor/executor.test.cpp | 37 +++++------ .../executor/executor_api.test.hpp | 51 ++++++--------- test/utils/TARGETS | 12 ++++ test/utils/executor/test_api_bundle.hpp | 35 +++++++++++ 7 files changed, 119 insertions(+), 103 deletions(-) create mode 100644 test/utils/executor/test_api_bundle.hpp diff --git a/src/buildtool/execution_engine/executor/executor.hpp b/src/buildtool/execution_engine/executor/executor.hpp index e14df472..9b3a5f86 100644 --- a/src/buildtool/execution_engine/executor/executor.hpp +++ b/src/buildtool/execution_engine/executor/executor.hpp @@ -34,6 +34,7 @@ #include "src/buildtool/common/tree.hpp" #include "src/buildtool/compatibility/compatibility.hpp" #include "src/buildtool/crypto/hash_function.hpp" +#include "src/buildtool/execution_api/common/api_bundle.hpp" #include "src/buildtool/execution_api/common/artifact_blob_container.hpp" #include "src/buildtool/execution_api/common/common_api.hpp" #include "src/buildtool/execution_api/common/execution_api.hpp" @@ -172,8 +173,7 @@ class ExecutorImpl { Logger const& logger, gsl::not_null const& artifact, gsl::not_null const& repo_config, - IExecutionApi const& remote_api, - IExecutionApi const& local_api, + ApiBundle const& apis, HashFunction hash_function) noexcept -> bool { auto const object_info_opt = artifact->Content().Info(); auto const file_path_opt = artifact->Content().FilePath(); @@ -196,16 +196,17 @@ class ExecutorImpl { << std::endl; return oss.str(); }); - if (not remote_api.IsAvailable(object_info_opt->digest)) { + if (not apis.remote->IsAvailable(object_info_opt->digest)) { // Check if requested artifact is available in local CAS and // upload to remote CAS in case it is. - if (local_api.IsAvailable(object_info_opt->digest) and - local_api.RetrieveToCas({*object_info_opt}, remote_api)) { + if (apis.local->IsAvailable(object_info_opt->digest) and + apis.local->RetrieveToCas({*object_info_opt}, + *apis.remote)) { return true; } if (not VerifyOrUploadKnownArtifact( - remote_api, + *apis.remote, artifact->Content().Repository(), repo_config, *object_info_opt)) { @@ -233,7 +234,7 @@ class ExecutorImpl { }); auto repo = artifact->Content().Repository(); auto new_info = UploadFile( - remote_api, hash_function, repo, repo_config, *file_path_opt); + *apis.remote, hash_function, repo, repo_config, *file_path_opt); if (not new_info) { Logger::Log(LogLevel::Error, "artifact in {} could not be uploaded to CAS.", @@ -711,8 +712,7 @@ class Executor { public: explicit Executor( gsl::not_null const& repo_config, - gsl::not_null const& local_api, - gsl::not_null const& remote_api, + gsl::not_null const& apis, gsl::not_null const& remote_context, HashFunction hash_function, gsl::not_null const& stats, @@ -720,8 +720,7 @@ class Executor { Logger const* logger = nullptr, // log in caller logger, if given std::chrono::milliseconds timeout = IExecutionAction::kDefaultTimeout) : repo_config_{repo_config}, - local_api_{*local_api}, - remote_api_{*remote_api}, + apis_{*apis}, remote_context_{*remote_context}, hash_function_{hash_function}, stats_{stats}, @@ -742,7 +741,7 @@ class Executor { auto const response = Impl::ExecuteAction( *logger_, action, - remote_api_, + *apis_.remote, Impl::MergeProperties( remote_context_.exec_config->platform_properties, action->ExecutionProperties()), @@ -763,7 +762,7 @@ class Executor { auto const response = Impl::ExecuteAction( logger, action, - remote_api_, + *apis_.remote, Impl::MergeProperties( remote_context_.exec_config->platform_properties, action->ExecutionProperties()), @@ -790,27 +789,18 @@ class Executor { // to avoid always creating a logger we might not need, which is a // non-copyable and non-movable object, we need some code duplication if (logger_ != nullptr) { - return Impl::VerifyOrUploadArtifact(*logger_, - artifact, - repo_config_, - remote_api_, - local_api_, - hash_function_); + return Impl::VerifyOrUploadArtifact( + *logger_, artifact, repo_config_, apis_, hash_function_); } Logger logger("artifact:" + ToHexString(artifact->Content().Id())); - return Impl::VerifyOrUploadArtifact(logger, - artifact, - repo_config_, - remote_api_, - local_api_, - hash_function_); + return Impl::VerifyOrUploadArtifact( + logger, artifact, repo_config_, apis_, hash_function_); } private: gsl::not_null repo_config_; - IExecutionApi const& local_api_; - IExecutionApi const& remote_api_; + ApiBundle const& apis_; RemoteContext const& remote_context_; HashFunction const hash_function_; gsl::not_null stats_; @@ -832,18 +822,18 @@ class Rebuilder { /// \param timeout Timeout for action execution. Rebuilder( gsl::not_null const& repo_config, - gsl::not_null const& local_api, - gsl::not_null const& remote_api, - gsl::not_null const& api_cached, + gsl::not_null const& apis, gsl::not_null const& remote_context, HashFunction hash_function, gsl::not_null const& stats, gsl::not_null const& progress, std::chrono::milliseconds timeout = IExecutionAction::kDefaultTimeout) : repo_config_{repo_config}, - local_api_{*local_api}, - remote_api_{*remote_api}, - api_cached_{*api_cached}, + apis_{*apis}, + api_cached_{ + apis->MakeRemote(remote_context->exec_config->cache_address, + remote_context->auth, + remote_context->retry_config)}, remote_context_{*remote_context}, hash_function_{hash_function}, stats_{stats}, @@ -858,7 +848,7 @@ class Rebuilder { auto response = Impl::ExecuteAction( logger, action, - remote_api_, + *apis_.remote, Impl::MergeProperties( remote_context_.exec_config->platform_properties, action->ExecutionProperties()), @@ -877,7 +867,7 @@ class Rebuilder { auto response_cached = Impl::ExecuteAction( logger_cached, action, - api_cached_, + *api_cached_, Impl::MergeProperties( remote_context_.exec_config->platform_properties, action->ExecutionProperties()), @@ -907,12 +897,8 @@ class Rebuilder { gsl::not_null const& artifact) const noexcept -> bool { Logger logger("artifact:" + ToHexString(artifact->Content().Id())); - return Impl::VerifyOrUploadArtifact(logger, - artifact, - repo_config_, - remote_api_, - local_api_, - hash_function_); + return Impl::VerifyOrUploadArtifact( + logger, artifact, repo_config_, apis_, hash_function_); } [[nodiscard]] auto DumpFlakyActions() const noexcept -> nlohmann::json { @@ -929,9 +915,8 @@ class Rebuilder { private: gsl::not_null repo_config_; - IExecutionApi const& local_api_; - IExecutionApi const& remote_api_; - IExecutionApi const& api_cached_; + ApiBundle const& apis_; + gsl::not_null const api_cached_; RemoteContext const& remote_context_; HashFunction const hash_function_; gsl::not_null stats_; diff --git a/src/buildtool/graph_traverser/graph_traverser.hpp b/src/buildtool/graph_traverser/graph_traverser.hpp index 301ef722..50eb95c5 100644 --- a/src/buildtool/graph_traverser/graph_traverser.hpp +++ b/src/buildtool/graph_traverser/graph_traverser.hpp @@ -355,8 +355,7 @@ class GraphTraverser { DependencyGraph const& g, std::vector const& artifact_ids) const -> bool { Executor executor{repo_config_, - &*apis_.local, - &*apis_.remote, + &apis_, &remote_context_, apis_.hash_function, stats_, @@ -383,15 +382,8 @@ class GraphTraverser { [[nodiscard]] auto TraverseRebuild( DependencyGraph const& g, std::vector const& artifact_ids) const -> bool { - // setup rebuilder with api for cache endpoint - auto api_cached = - apis_.MakeRemote(remote_context_.exec_config->cache_address, - remote_context_.auth, - remote_context_.retry_config); Rebuilder executor{repo_config_, - &*apis_.local, - &*apis_.remote, - &*api_cached, + &apis_, &remote_context_, apis_.hash_function, stats_, diff --git a/test/buildtool/execution_engine/executor/TARGETS b/test/buildtool/execution_engine/executor/TARGETS index ef01b6fc..ab257156 100644 --- a/test/buildtool/execution_engine/executor/TARGETS +++ b/test/buildtool/execution_engine/executor/TARGETS @@ -17,6 +17,7 @@ , ["@", "src", "src/buildtool/compatibility", "compatibility"] , ["@", "catch2", "", "catch2"] , ["@", "gsl", "", "gsl"] + , ["utils", "test_api_bundle"] , ["utils", "test_remote_config"] ] , "stage": ["test", "buildtool", "execution_engine", "executor"] @@ -42,6 +43,7 @@ , ["", "catch-main"] , ["@", "catch2", "", "catch2"] , ["@", "gsl", "", "gsl"] + , ["utils", "test_api_bundle"] ] , "stage": ["test", "buildtool", "execution_engine", "executor"] } diff --git a/test/buildtool/execution_engine/executor/executor.test.cpp b/test/buildtool/execution_engine/executor/executor.test.cpp index 2e8a03aa..15752c94 100644 --- a/test/buildtool/execution_engine/executor/executor.test.cpp +++ b/test/buildtool/execution_engine/executor/executor.test.cpp @@ -35,6 +35,7 @@ #include "src/buildtool/execution_engine/executor/executor.hpp" #include "src/buildtool/file_system/file_system_manager.hpp" #include "src/buildtool/progress_reporting/progress.hpp" +#include "test/utils/executor/test_api_bundle.hpp" /// \brief Mockup API test config. struct TestApiConfig { @@ -296,9 +297,9 @@ TEST_CASE("Executor: Process artifact", "[executor]") { RemoteContext const remote_context{.auth = &auth, .retry_config = &retry_config, .exec_config = &remote_config}; + auto const apis = CreateTestApiBundle(hash_function, api); Executor runner{&repo_config, - api.get(), - api.get(), + &apis, &remote_context, hash_function, &stats, @@ -320,9 +321,9 @@ TEST_CASE("Executor: Process artifact", "[executor]") { RemoteContext const remote_context{.auth = &auth, .retry_config = &retry_config, .exec_config = &remote_config}; + auto const apis = CreateTestApiBundle(hash_function, api); Executor runner{&repo_config, - api.get(), - api.get(), + &apis, &remote_context, hash_function, &stats, @@ -344,9 +345,9 @@ TEST_CASE("Executor: Process artifact", "[executor]") { RemoteContext const remote_context{.auth = &auth, .retry_config = &retry_config, .exec_config = &remote_config}; + auto const apis = CreateTestApiBundle(hash_function, api); Executor runner{&repo_config, - api.get(), - api.get(), + &apis, &remote_context, hash_function, &stats, @@ -393,9 +394,9 @@ TEST_CASE("Executor: Process action", "[executor]") { RemoteContext const remote_context{.auth = &auth, .retry_config = &retry_config, .exec_config = &remote_config}; + auto const apis = CreateTestApiBundle(hash_function, api); Executor runner{&repo_config, - api.get(), - api.get(), + &apis, &remote_context, hash_function, &stats, @@ -420,9 +421,9 @@ TEST_CASE("Executor: Process action", "[executor]") { RemoteContext const remote_context{.auth = &auth, .retry_config = &retry_config, .exec_config = &remote_config}; + auto const apis = CreateTestApiBundle(hash_function, api); Executor runner{&repo_config, - api.get(), - api.get(), + &apis, &remote_context, hash_function, &stats, @@ -447,9 +448,9 @@ TEST_CASE("Executor: Process action", "[executor]") { RemoteContext const remote_context{.auth = &auth, .retry_config = &retry_config, .exec_config = &remote_config}; + auto const apis = CreateTestApiBundle(hash_function, api); Executor runner{&repo_config, - api.get(), - api.get(), + &apis, &remote_context, hash_function, &stats, @@ -477,9 +478,9 @@ TEST_CASE("Executor: Process action", "[executor]") { RemoteContext const remote_context{.auth = &auth, .retry_config = &retry_config, .exec_config = &remote_config}; + auto const apis = CreateTestApiBundle(hash_function, api); Executor runner{&repo_config, - api.get(), - api.get(), + &apis, &remote_context, hash_function, &stats, @@ -504,9 +505,9 @@ TEST_CASE("Executor: Process action", "[executor]") { RemoteContext const remote_context{.auth = &auth, .retry_config = &retry_config, .exec_config = &remote_config}; + auto const apis = CreateTestApiBundle(hash_function, api); Executor runner{&repo_config, - api.get(), - api.get(), + &apis, &remote_context, hash_function, &stats, @@ -534,9 +535,9 @@ TEST_CASE("Executor: Process action", "[executor]") { RemoteContext const remote_context{.auth = &auth, .retry_config = &retry_config, .exec_config = &remote_config}; + auto const apis = CreateTestApiBundle(hash_function, api); Executor runner{&repo_config, - api.get(), - api.get(), + &apis, &remote_context, hash_function, &stats, diff --git a/test/buildtool/execution_engine/executor/executor_api.test.hpp b/test/buildtool/execution_engine/executor/executor_api.test.hpp index 7bbc9d8f..bef97e85 100644 --- a/test/buildtool/execution_engine/executor/executor_api.test.hpp +++ b/test/buildtool/execution_engine/executor/executor_api.test.hpp @@ -38,6 +38,7 @@ #include "src/buildtool/execution_engine/executor/executor.hpp" #include "src/buildtool/file_system/file_system_manager.hpp" #include "src/buildtool/progress_reporting/progress.hpp" +#include "test/utils/executor/test_api_bundle.hpp" #include "test/utils/remote_execution/test_remote_config.hpp" using ApiFactory = std::function; @@ -147,13 +148,9 @@ static inline void RunHelloWorldCompilation( : HashFunction::Type::GitSHA1}; auto api = factory(); - Executor runner{repo_config, - api.get(), - api.get(), - &remote_context, - hash_function, - stats, - progress}; + auto const apis = CreateTestApiBundle(hash_function, api); + Executor runner{ + repo_config, &apis, &remote_context, hash_function, stats, progress}; // upload local artifacts auto const* main_cpp_node = g.ArtifactNodeWithId(main_cpp_id); @@ -281,13 +278,9 @@ static inline void RunGreeterCompilation( : HashFunction::Type::GitSHA1}; auto api = factory(); - Executor runner{repo_config, - api.get(), - api.get(), - &remote_context, - hash_function, - stats, - progress}; + auto const apis = CreateTestApiBundle(hash_function, api); + Executor runner{ + repo_config, &apis, &remote_context, hash_function, stats, progress}; // upload local artifacts for (auto const& id : {greet_hpp_id, greet_cpp_id, main_cpp_id}) { @@ -455,13 +448,9 @@ static inline void TestUploadAndDownloadTrees( .retry_config = &retry_config, .exec_config = &*remote_config}; - Executor runner{repo_config, - api.get(), - api.get(), - &remote_context, - hash_function, - stats, - progress}; + auto const apis = CreateTestApiBundle(hash_function, api); + Executor runner{ + repo_config, &apis, &remote_context, hash_function, stats, progress}; REQUIRE(runner.Process(g.ArtifactNodeWithId(foo_id))); REQUIRE(runner.Process(g.ArtifactNodeWithId(bar_id))); @@ -630,9 +619,9 @@ static inline void TestRetrieveOutputDirectories( // run action auto api = factory(); + auto const apis = CreateTestApiBundle(hash_function, api); Executor runner{repo_config, - api.get(), - api.get(), + &apis, &remote_context, hash_function, stats, @@ -683,9 +672,9 @@ static inline void TestRetrieveOutputDirectories( // run action auto api = factory(); + auto const apis = CreateTestApiBundle(hash_function, api); Executor runner{repo_config, - api.get(), - api.get(), + &apis, &remote_context, hash_function, stats, @@ -753,9 +742,9 @@ static inline void TestRetrieveOutputDirectories( // run action auto api = factory(); + auto const apis = CreateTestApiBundle(hash_function, api); Executor runner{repo_config, - api.get(), - api.get(), + &apis, &remote_context, hash_function, stats, @@ -825,9 +814,9 @@ static inline void TestRetrieveOutputDirectories( // run action auto api = factory(); + auto const apis = CreateTestApiBundle(hash_function, api); Executor runner{repo_config, - api.get(), - api.get(), + &apis, &remote_context, hash_function, stats, @@ -850,9 +839,9 @@ static inline void TestRetrieveOutputDirectories( // run action auto api = factory(); + auto const apis = CreateTestApiBundle(hash_function, api); Executor runner{repo_config, - api.get(), - api.get(), + &apis, &remote_context, hash_function, stats, diff --git a/test/utils/TARGETS b/test/utils/TARGETS index dd16466a..8a8aea6c 100644 --- a/test/utils/TARGETS +++ b/test/utils/TARGETS @@ -170,6 +170,18 @@ , ["@", "src", "src/buildtool/crypto", "hash_function"] ] } +, "test_api_bundle": + { "type": ["@", "rules", "CC", "library"] + , "name": ["test_api_bundle"] + , "hdrs": ["executor/test_api_bundle.hpp"] + , "stage": ["test", "utils"] + , "deps": + [ ["@", "gsl", "", "gsl"] + , ["@", "src", "src/buildtool/crypto", "hash_function"] + , ["@", "src", "src/buildtool/execution_api/common", "api_bundle"] + , ["@", "src", "src/buildtool/execution_api/common", "common"] + ] + } , "TESTS": { "type": "install" , "tainted": ["test"] diff --git a/test/utils/executor/test_api_bundle.hpp b/test/utils/executor/test_api_bundle.hpp new file mode 100644 index 00000000..1b582ce6 --- /dev/null +++ b/test/utils/executor/test_api_bundle.hpp @@ -0,0 +1,35 @@ +// Copyright 2024 Huawei Cloud Computing Technology Co., Ltd. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef INCLUDED_SRC_TEST_UTILS_EXECUTOR_TEST_API_BUNDLE_HPP +#define INCLUDED_SRC_TEST_UTILS_EXECUTOR_TEST_API_BUNDLE_HPP + +#include // std::move + +#include "gsl/gsl" +#include "src/buildtool/crypto/hash_function.hpp" +#include "src/buildtool/execution_api/common/api_bundle.hpp" +#include "src/buildtool/execution_api/common/execution_api.hpp" + +/// \brief Creates an ApiBundle that contains a given IExecutionApi +/// implementation. As only the hash_function field is actually needed, the +/// remote_context and repo_config are not needed to be provided. +[[nodiscard]] static auto CreateTestApiBundle( + HashFunction hash_function, + gsl::not_null const& api) noexcept -> ApiBundle { + return ApiBundle{ + .hash_function = hash_function, .local = api, .remote = api}; +} + +#endif // INCLUDED_SRC_TEST_UTILS_EXECUTOR_TEST_API_BUNDLE_HPP -- cgit v1.2.3