From 3987f571650c591ec895cb82745fac2c895c66d2 Mon Sep 17 00:00:00 2001 From: Paul Cristian Sarbu Date: Mon, 8 Jan 2024 11:13:42 +0100 Subject: Refactor GraphTraverser to take in platform properties and dispatch list In order for the serve endpoint to correctly dispatch a build to the correct remote-execution endpoint, the platform properties and dispatch list for a build need to be passed explicitly to the executor (via the graph traverser instance) instead of always being taken from the RemoteExecutionConfig struct. This commit implements these changes, including updating existing tests accordingly. --- .../execution_engine/executor/executor.test.cpp | 18 ++--- .../executor/executor_api.test.hpp | 24 ++++--- .../graph_traverser/graph_traverser.test.hpp | 78 +++++++++++++++++----- 3 files changed, 87 insertions(+), 33 deletions(-) (limited to 'test') diff --git a/test/buildtool/execution_engine/executor/executor.test.cpp b/test/buildtool/execution_engine/executor/executor.test.cpp index 55417016..c60326c1 100644 --- a/test/buildtool/execution_engine/executor/executor.test.cpp +++ b/test/buildtool/execution_engine/executor/executor.test.cpp @@ -264,7 +264,7 @@ TEST_CASE("Executor: Process artifact", "[executor]") { SECTION("Processing succeeds for valid config") { auto api = TestApi::Ptr{new TestApi{config}}; - Executor runner{&repo_config, api.get(), api.get(), {}}; + Executor runner{&repo_config, api.get(), api.get(), {}, {}}; CHECK(runner.Process(g.ArtifactNodeWithId(local_cpp_id))); CHECK(runner.Process(g.ArtifactNodeWithId(known_cpp_id))); @@ -274,7 +274,7 @@ TEST_CASE("Executor: Process artifact", "[executor]") { config.artifacts["local.cpp"].uploads = false; auto api = TestApi::Ptr{new TestApi{config}}; - Executor runner{&repo_config, api.get(), api.get(), {}}; + Executor runner{&repo_config, api.get(), api.get(), {}, {}}; CHECK(not runner.Process(g.ArtifactNodeWithId(local_cpp_id))); CHECK(runner.Process(g.ArtifactNodeWithId(known_cpp_id))); @@ -284,7 +284,7 @@ TEST_CASE("Executor: Process artifact", "[executor]") { config.artifacts["known.cpp"].available = false; auto api = TestApi::Ptr{new TestApi{config}}; - Executor runner{&repo_config, api.get(), api.get(), {}}; + Executor runner{&repo_config, api.get(), api.get(), {}, {}}; CHECK(runner.Process(g.ArtifactNodeWithId(local_cpp_id))); CHECK(not runner.Process(g.ArtifactNodeWithId(known_cpp_id))); @@ -317,7 +317,7 @@ TEST_CASE("Executor: Process action", "[executor]") { SECTION("Processing succeeds for valid config") { auto api = TestApi::Ptr{new TestApi{config}}; - Executor runner{&repo_config, api.get(), api.get(), {}}; + Executor runner{&repo_config, api.get(), api.get(), {}, {}}; CHECK(runner.Process(g.ArtifactNodeWithId(local_cpp_id))); CHECK(runner.Process(g.ArtifactNodeWithId(known_cpp_id))); @@ -330,7 +330,7 @@ TEST_CASE("Executor: Process action", "[executor]") { config.response.cached = false; auto api = TestApi::Ptr{new TestApi{config}}; - Executor runner{&repo_config, api.get(), api.get(), {}}; + Executor runner{&repo_config, api.get(), api.get(), {}, {}}; CHECK(runner.Process(g.ArtifactNodeWithId(local_cpp_id))); CHECK(runner.Process(g.ArtifactNodeWithId(known_cpp_id))); @@ -343,7 +343,7 @@ TEST_CASE("Executor: Process action", "[executor]") { config.artifacts["output2.exe"].available = false; auto api = TestApi::Ptr{new TestApi{config}}; - Executor runner{&repo_config, api.get(), api.get(), {}}; + Executor runner{&repo_config, api.get(), api.get(), {}, {}}; CHECK(runner.Process(g.ArtifactNodeWithId(local_cpp_id))); CHECK(runner.Process(g.ArtifactNodeWithId(known_cpp_id))); @@ -359,7 +359,7 @@ TEST_CASE("Executor: Process action", "[executor]") { config.execution.failed = true; auto api = TestApi::Ptr{new TestApi{config}}; - Executor runner{&repo_config, api.get(), api.get(), {}}; + Executor runner{&repo_config, api.get(), api.get(), {}, {}}; CHECK(runner.Process(g.ArtifactNodeWithId(local_cpp_id))); CHECK(runner.Process(g.ArtifactNodeWithId(known_cpp_id))); @@ -372,7 +372,7 @@ TEST_CASE("Executor: Process action", "[executor]") { config.response.exit_code = 1; auto api = TestApi::Ptr{new TestApi{config}}; - Executor runner{&repo_config, api.get(), api.get(), {}}; + Executor runner{&repo_config, api.get(), api.get(), {}, {}}; CHECK(runner.Process(g.ArtifactNodeWithId(local_cpp_id))); CHECK(runner.Process(g.ArtifactNodeWithId(known_cpp_id))); @@ -388,7 +388,7 @@ TEST_CASE("Executor: Process action", "[executor]") { config.execution.outputs = {"output1.exe" /*, "output2.exe"*/}; auto api = TestApi::Ptr{new TestApi{config}}; - Executor runner{&repo_config, api.get(), api.get(), {}}; + Executor runner{&repo_config, api.get(), api.get(), {}, {}}; CHECK(runner.Process(g.ArtifactNodeWithId(local_cpp_id))); CHECK(runner.Process(g.ArtifactNodeWithId(known_cpp_id))); diff --git a/test/buildtool/execution_engine/executor/executor_api.test.hpp b/test/buildtool/execution_engine/executor/executor_api.test.hpp index 85b19911..ad7a5216 100755 --- a/test/buildtool/execution_engine/executor/executor_api.test.hpp +++ b/test/buildtool/execution_engine/executor/executor_api.test.hpp @@ -110,7 +110,8 @@ static inline void RunHelloWorldCompilation(RepositoryConfig* repo_config, Executor runner{repo_config, api.get(), api.get(), - RemoteExecutionConfig::PlatformProperties()}; + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; // upload local artifacts auto const* main_cpp_node = g.ArtifactNodeWithId(main_cpp_id); @@ -216,7 +217,8 @@ static inline void RunGreeterCompilation(RepositoryConfig* repo_config, Executor runner{repo_config, api.get(), api.get(), - RemoteExecutionConfig::PlatformProperties()}; + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; // upload local artifacts for (auto const& id : {greet_hpp_id, greet_cpp_id, main_cpp_id}) { @@ -339,7 +341,8 @@ static inline void TestUploadAndDownloadTrees(RepositoryConfig* repo_config, Executor runner{repo_config, api.get(), api.get(), - RemoteExecutionConfig::PlatformProperties()}; + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; REQUIRE(runner.Process(g.ArtifactNodeWithId(foo_id))); REQUIRE(runner.Process(g.ArtifactNodeWithId(bar_id))); @@ -485,7 +488,8 @@ static inline void TestRetrieveOutputDirectories(RepositoryConfig* repo_config, Executor runner{repo_config, api.get(), api.get(), - RemoteExecutionConfig::PlatformProperties()}; + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; REQUIRE(runner.Process(action)); // read output @@ -532,7 +536,8 @@ static inline void TestRetrieveOutputDirectories(RepositoryConfig* repo_config, Executor runner{repo_config, api.get(), api.get(), - RemoteExecutionConfig::PlatformProperties()}; + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; REQUIRE(runner.Process(action)); // read output @@ -595,7 +600,8 @@ static inline void TestRetrieveOutputDirectories(RepositoryConfig* repo_config, Executor runner{repo_config, api.get(), api.get(), - RemoteExecutionConfig::PlatformProperties()}; + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; REQUIRE(runner.Process(action)); // read output @@ -663,7 +669,8 @@ static inline void TestRetrieveOutputDirectories(RepositoryConfig* repo_config, Executor runner{repo_config, api.get(), api.get(), - RemoteExecutionConfig::PlatformProperties()}; + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; CHECK_FALSE(runner.Process(action)); } @@ -684,7 +691,8 @@ static inline void TestRetrieveOutputDirectories(RepositoryConfig* repo_config, Executor runner{repo_config, api.get(), api.get(), - RemoteExecutionConfig::PlatformProperties()}; + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; CHECK_FALSE(runner.Process(action)); } } diff --git a/test/buildtool/graph_traverser/graph_traverser.test.hpp b/test/buildtool/graph_traverser/graph_traverser.test.hpp index a7eeb1e0..58675f91 100644 --- a/test/buildtool/graph_traverser/graph_traverser.test.hpp +++ b/test/buildtool/graph_traverser/graph_traverser.test.hpp @@ -129,7 +129,10 @@ class TestProject { TestProject p("hello_world_copy_message"); auto const clargs = p.CmdLineArgs(); - GraphTraverser const gt{clargs.gtargs, p.GetRepoConfig()}; + GraphTraverser const gt{clargs.gtargs, + p.GetRepoConfig(), + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; auto const result = gt.BuildAndStage(clargs.graph_description, clargs.artifacts); @@ -148,7 +151,11 @@ class TestProject { SECTION("Executable is retrieved as executable") { auto const clargs_exec = p.CmdLineArgs("_entry_points_get_executable"); - GraphTraverser const gt_get_exec{clargs_exec.gtargs, p.GetRepoConfig()}; + GraphTraverser const gt_get_exec{ + clargs_exec.gtargs, + p.GetRepoConfig(), + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; auto const exec_result = gt_get_exec.BuildAndStage( clargs_exec.graph_description, clargs_exec.artifacts); @@ -172,7 +179,10 @@ class TestProject { TestProject p("copy_local_file"); auto const clargs = p.CmdLineArgs(); - GraphTraverser const gt{clargs.gtargs, p.GetRepoConfig()}; + GraphTraverser const gt{clargs.gtargs, + p.GetRepoConfig(), + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; auto const result = gt.BuildAndStage(clargs.graph_description, clargs.artifacts); @@ -191,7 +201,10 @@ class TestProject { TestProject p("sequence_printer_build_library_only"); auto const clargs = p.CmdLineArgs(); - GraphTraverser const gt{clargs.gtargs, p.GetRepoConfig()}; + GraphTraverser const gt{clargs.gtargs, + p.GetRepoConfig(), + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; auto const result = gt.BuildAndStage(clargs.graph_description, clargs.artifacts); @@ -200,8 +213,11 @@ class TestProject { CHECK(FileSystemManager::IsFile(result->output_paths.at(0))); auto const clargs_full_build = p.CmdLineArgs("_entry_points_full_build"); - GraphTraverser const gt_full_build{clargs_full_build.gtargs, - p.GetRepoConfig()}; + GraphTraverser const gt_full_build{ + clargs_full_build.gtargs, + p.GetRepoConfig(), + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; auto const full_build_result = gt_full_build.BuildAndStage( clargs_full_build.graph_description, clargs_full_build.artifacts); @@ -225,7 +241,9 @@ class TestProject { auto const clargs_update_cpp = full_hello_world.CmdLineArgs("_entry_points_upload_source"); GraphTraverser const gt_upload{clargs_update_cpp.gtargs, - full_hello_world.GetRepoConfig()}; + full_hello_world.GetRepoConfig(), + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; auto const cpp_result = gt_upload.BuildAndStage( clargs_update_cpp.graph_description, clargs_update_cpp.artifacts); @@ -241,7 +259,10 @@ class TestProject { TestProject hello_world_known_cpp("hello_world_known_source"); auto const clargs = hello_world_known_cpp.CmdLineArgs(); - GraphTraverser const gt{clargs.gtargs, full_hello_world.GetRepoConfig()}; + GraphTraverser const gt{clargs.gtargs, + full_hello_world.GetRepoConfig(), + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; auto const result = gt.BuildAndStage(clargs.graph_description, clargs.artifacts); @@ -262,7 +283,10 @@ static void TestBlobsUploadedAndUsed(bool is_hermetic = true) { TestProject p("use_uploaded_blobs"); auto const clargs = p.CmdLineArgs(); - GraphTraverser gt{clargs.gtargs, p.GetRepoConfig()}; + GraphTraverser gt{clargs.gtargs, + p.GetRepoConfig(), + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; auto const result = gt.BuildAndStage(clargs.graph_description, clargs.artifacts); @@ -288,7 +312,10 @@ static void TestEnvironmentVariablesSetAndUsed(bool is_hermetic = true) { TestProject p("use_env_variables"); auto const clargs = p.CmdLineArgs(); - GraphTraverser gt{clargs.gtargs, p.GetRepoConfig()}; + GraphTraverser gt{clargs.gtargs, + p.GetRepoConfig(), + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; auto const result = gt.BuildAndStage(clargs.graph_description, clargs.artifacts); @@ -314,7 +341,10 @@ static void TestTreesUsed(bool is_hermetic = true) { TestProject p("use_trees"); auto const clargs = p.CmdLineArgs(); - GraphTraverser gt{clargs.gtargs, p.GetRepoConfig()}; + GraphTraverser gt{clargs.gtargs, + p.GetRepoConfig(), + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; auto const result = gt.BuildAndStage(clargs.graph_description, clargs.artifacts); @@ -340,7 +370,10 @@ static void TestNestedTreesUsed(bool is_hermetic = true) { TestProject p("use_nested_trees"); auto const clargs = p.CmdLineArgs(); - GraphTraverser gt{clargs.gtargs, p.GetRepoConfig()}; + GraphTraverser gt{clargs.gtargs, + p.GetRepoConfig(), + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; auto const result = gt.BuildAndStage(clargs.graph_description, clargs.artifacts); @@ -367,7 +400,10 @@ static void TestFlakyHelloWorldDetected(bool /*is_hermetic*/ = true) { { auto clargs = p.CmdLineArgs("_entry_points_ctimes"); - GraphTraverser const gt{clargs.gtargs, p.GetRepoConfig()}; + GraphTraverser const gt{clargs.gtargs, + p.GetRepoConfig(), + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; auto const result = gt.BuildAndStage(clargs.graph_description, clargs.artifacts); @@ -381,7 +417,10 @@ static void TestFlakyHelloWorldDetected(bool /*is_hermetic*/ = true) { // make_exe[flaky]->make_output[miss] auto clargs_output = p.CmdLineArgs(); clargs_output.gtargs.rebuild = RebuildArguments{}; - GraphTraverser const gt_output{clargs_output.gtargs, p.GetRepoConfig()}; + GraphTraverser const gt_output{clargs_output.gtargs, + p.GetRepoConfig(), + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; REQUIRE(gt_output.BuildAndStage(clargs_output.graph_description, clargs_output.artifacts)); CHECK(Statistics::Instance().ActionsFlakyCounter() == 1); @@ -392,7 +431,11 @@ static void TestFlakyHelloWorldDetected(bool /*is_hermetic*/ = true) { // make_exe[flaky]->make_output[miss]->strip_time [miss] auto clargs_stripped = p.CmdLineArgs("_entry_points_stripped"); clargs_stripped.gtargs.rebuild = RebuildArguments{}; - GraphTraverser const gt_stripped{clargs_stripped.gtargs, p.GetRepoConfig()}; + GraphTraverser const gt_stripped{ + clargs_stripped.gtargs, + p.GetRepoConfig(), + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; REQUIRE(gt_stripped.BuildAndStage(clargs_stripped.graph_description, clargs_stripped.artifacts)); CHECK(Statistics::Instance().ActionsFlakyCounter() == 1); @@ -403,7 +446,10 @@ static void TestFlakyHelloWorldDetected(bool /*is_hermetic*/ = true) { // make_exe[flaky]->make_output[miss]->strip_time[miss]->list_ctimes [flaky] auto clargs_ctimes = p.CmdLineArgs("_entry_points_ctimes"); clargs_ctimes.gtargs.rebuild = RebuildArguments{}; - GraphTraverser const gt_ctimes{clargs_ctimes.gtargs, p.GetRepoConfig()}; + GraphTraverser const gt_ctimes{clargs_ctimes.gtargs, + p.GetRepoConfig(), + RemoteExecutionConfig::PlatformProperties(), + RemoteExecutionConfig::DispatchList()}; REQUIRE(gt_ctimes.BuildAndStage(clargs_ctimes.graph_description, clargs_ctimes.artifacts)); CHECK(Statistics::Instance().ActionsFlakyCounter() == 2); -- cgit v1.2.3