diff options
| -rw-r--r-- | src/buildtool/execution_engine/traverser/traverser.hpp | 26 | ||||
| -rw-r--r-- | src/buildtool/graph_traverser/graph_traverser.hpp | 22 | ||||
| -rw-r--r-- | test/buildtool/execution_engine/traverser/traverser.test.cpp | 73 | ||||
| -rw-r--r-- | test/end-to-end/TARGETS | 1 | ||||
| -rw-r--r-- | test/end-to-end/build-fails/TARGETS | 9 | ||||
| -rwxr-xr-x | test/end-to-end/build-fails/single_fail_dep.sh | 47 |
6 files changed, 137 insertions, 41 deletions
diff --git a/src/buildtool/execution_engine/traverser/traverser.hpp b/src/buildtool/execution_engine/traverser/traverser.hpp index 930cf0fb..4d3fc7e9 100644 --- a/src/buildtool/execution_engine/traverser/traverser.hpp +++ b/src/buildtool/execution_engine/traverser/traverser.hpp @@ -1,6 +1,8 @@ #ifndef INCLUDED_SRC_BUILDTOOL_EXECUTION_ENGINE_TRAVERSER_TRAVERSER_HPP #define INCLUDED_SRC_BUILDTOOL_EXECUTION_ENGINE_TRAVERSER_TRAVERSER_HPP +#include <atomic> + #include "gsl-lite/gsl-lite.hpp" #include "src/buildtool/execution_engine/dag/dag.hpp" #include "src/buildtool/logging/logger.hpp" @@ -27,8 +29,14 @@ concept Runnable = requires(T const r, template <Runnable Executor> class Traverser { public: - Traverser(Executor const& r, DependencyGraph const& graph, std::size_t jobs) - : runner_{r}, graph_{graph}, tasker_{jobs} {} + Traverser(Executor const& r, + DependencyGraph const& graph, + std::size_t jobs, + gsl::not_null<std::atomic<bool>*> fail_flag) + : runner_{r}, + graph_{graph}, + failed_{std::move(fail_flag)}, + tasker_{jobs} {} Traverser() = delete; Traverser(Traverser const&) = delete; Traverser(Traverser&&) = delete; @@ -52,6 +60,7 @@ class Traverser { private: Executor const& runner_{}; DependencyGraph const& graph_; + gsl::not_null<std::atomic<bool>*> failed_; TaskSystem tasker_{}; // THIS SHOULD BE THE LAST MEMBER VARIABLE // Visits discover nodes and queue visits to their children nodes. @@ -77,7 +86,7 @@ class Traverser { void QueueVisit(NodeTypePtr node) noexcept { // in case the node was already discovered, there is no need to queue // the visit - if (node->TraversalState()->GetAndMarkDiscovered()) { + if (failed_->load() or node->TraversalState()->GetAndMarkDiscovered()) { return; } tasker_.QueueTask([this, node]() noexcept { Visit(node); }); @@ -89,7 +98,7 @@ class Traverser { // was successful template <typename NodeTypePtr> void QueueProcessing(NodeTypePtr node) noexcept { - if (not node->TraversalState()->IsRequired() or + if (failed_->load() or not node->TraversalState()->IsRequired() or node->TraversalState()->GetAndMarkQueuedToBeProcessed()) { return; } @@ -99,12 +108,16 @@ class Traverser { NotifyAvailable(node); } else { - Logger::Log(LogLevel::Error, "Build failed."); - std::exit(EXIT_FAILURE); + Abort(); } }; tasker_.QueueTask(process_node); } + + void Abort() noexcept { + failed_->store(true); + tasker_.Shutdown(); // skip execution of pending tasks + } }; template <Runnable Executor> @@ -116,6 +129,7 @@ auto Traverser<Executor>::Traverse( QueueVisit(artifact_node); } else { + Abort(); Logger::Log( LogLevel::Error, "artifact with id {} can not be found in dependency graph.", diff --git a/src/buildtool/graph_traverser/graph_traverser.hpp b/src/buildtool/graph_traverser/graph_traverser.hpp index fac93e27..c00fe61b 100644 --- a/src/buildtool/graph_traverser/graph_traverser.hpp +++ b/src/buildtool/graph_traverser/graph_traverser.hpp @@ -324,20 +324,21 @@ class GraphTraverser { Executor executor{&(*api_), RemoteExecutionConfig::PlatformProperties(), clargs_.build.timeout}; - bool result{}; + bool traversing{}; std::atomic<bool> done = false; + std::atomic<bool> failed = false; std::condition_variable cv{}; auto observer = std::thread([this, &done, &cv]() { reporter_(&done, &cv); }); { - Traverser t{executor, g, clargs_.jobs}; - result = + Traverser t{executor, g, clargs_.jobs, &failed}; + traversing = t.Traverse({std::begin(artifact_ids), std::end(artifact_ids)}); } done = true; cv.notify_all(); observer.join(); - return result; + return traversing and not failed; } [[nodiscard]] auto TraverseRebuild( @@ -350,18 +351,19 @@ class GraphTraverser { &(*api_cached), RemoteExecutionConfig::PlatformProperties(), clargs_.build.timeout}; - bool success{false}; + bool traversing{false}; + std::atomic<bool> failed{false}; { - Traverser t{executor, g, clargs_.jobs}; - success = + Traverser t{executor, g, clargs_.jobs, &failed}; + traversing = t.Traverse({std::begin(artifact_ids), std::end(artifact_ids)}); } - if (success and clargs_.rebuild->dump_flaky) { + if (traversing and not failed and clargs_.rebuild->dump_flaky) { std::ofstream file{*clargs_.rebuild->dump_flaky}; file << executor.DumpFlakyActions().dump(2); } - return success; + return traversing and not failed; } /// \brief Retrieves nodes corresponding to artifacts with ids in artifacts. @@ -463,7 +465,7 @@ class GraphTraverser { if (clargs_.rebuild ? not TraverseRebuild(*graph, artifact_ids) : not Traverse(*graph, artifact_ids)) { - Logger::Log(LogLevel::Error, "traversing graph failed."); + Logger::Log(LogLevel::Error, "Build failed."); return std::nullopt; } diff --git a/test/buildtool/execution_engine/traverser/traverser.test.cpp b/test/buildtool/execution_engine/traverser/traverser.test.cpp index 693097d5..5f06cc00 100644 --- a/test/buildtool/execution_engine/traverser/traverser.test.cpp +++ b/test/buildtool/execution_engine/traverser/traverser.test.cpp @@ -212,14 +212,16 @@ TEST_CASE("Executable", "[traverser]") { DependencyGraph g; CHECK(p.FillGraph(&g)); TestBuildInfo build_info; + std::atomic<bool> failed{}; std::string name = "This is a long name that shouldn't be corrupted"; build_info.SetName(name); SECTION("Traverse()") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); CHECK(traverser.Traverse()); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( @@ -235,7 +237,7 @@ TEST_CASE("Executable", "[traverser]") { SECTION("Traverse(executable)") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); auto const exec_id = ArtifactFactory::Identifier( ArtifactFactory::DescribeActionArtifact("action", @@ -272,14 +274,16 @@ TEST_CASE("Executable depends on library", "[traverser]") { DependencyGraph g; CHECK(p.FillGraph(&g)); TestBuildInfo build_info; + std::atomic<bool> failed{}; std::string name = "This is a long name that shouldn't be corrupted"; build_info.SetName(name); SECTION("Full build (without specifying artifacts)") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); CHECK(traverser.Traverse()); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( @@ -295,12 +299,13 @@ TEST_CASE("Executable depends on library", "[traverser]") { SECTION("Full build (executable)") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); auto const exec_id = ArtifactFactory::Identifier( ArtifactFactory::DescribeActionArtifact("make_exe", "executable")); CHECK(traverser.Traverse({exec_id})); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( @@ -318,9 +323,10 @@ TEST_CASE("Executable depends on library", "[traverser]") { ArtifactFactory::DescribeActionArtifact("make_lib", "library")); { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); CHECK(traverser.Traverse({lib_id})); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( @@ -354,14 +360,16 @@ TEST_CASE("Two artifacts depend on another", "[traverser]") { DependencyGraph g; CHECK(p.FillGraph(&g)); TestBuildInfo build_info; + std::atomic<bool> failed{}; std::string name = "This is a long name that shouldn't be corrupted"; build_info.SetName(name); SECTION("Full build") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); CHECK(traverser.Traverse()); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( @@ -379,9 +387,10 @@ TEST_CASE("Two artifacts depend on another", "[traverser]") { ArtifactFactory::DescribeActionArtifact("action1", "toplevel1")); { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); CHECK(traverser.Traverse({toplevel1_id})); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( @@ -406,14 +415,16 @@ TEST_CASE("Action with two outputs, no deps", "[traverser]") { DependencyGraph g; CHECK(p.FillGraph(&g)); TestBuildInfo build_info; + std::atomic<bool> failed{}; std::string name = "This is a long name that shouldn't be corrupted"; build_info.SetName(name); SECTION("Traverse()") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); CHECK(traverser.Traverse()); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( @@ -430,7 +441,7 @@ TEST_CASE("Action with two outputs, no deps", "[traverser]") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); auto const traversed = traverser.Traverse({output1_id}); CHECK(traversed); } @@ -449,7 +460,7 @@ TEST_CASE("Action with two outputs, no deps", "[traverser]") { SECTION("Traverse(output1, output2)") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); auto const traversed = traverser.Traverse({output1_id, output2_id}); CHECK(traversed); } @@ -480,14 +491,16 @@ TEST_CASE("Action with two outputs, one dep", "[traverser]") { DependencyGraph g; CHECK(p.FillGraph(&g)); TestBuildInfo build_info; + std::atomic<bool> failed{}; std::string name = "This is a long name that shouldn't be corrupted"; build_info.SetName(name); SECTION("Traverse()") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); CHECK(traverser.Traverse()); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( @@ -503,7 +516,7 @@ TEST_CASE("Action with two outputs, one dep", "[traverser]") { SECTION("Traverse(output1)") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); auto const traversed = traverser.Traverse({output1_id}); CHECK(traversed); } @@ -522,7 +535,7 @@ TEST_CASE("Action with two outputs, one dep", "[traverser]") { SECTION("Traverse(output1, output2)") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); auto const traversed = traverser.Traverse({output1_id, output2_id}); CHECK(traversed); } @@ -543,7 +556,7 @@ TEST_CASE("Action with two outputs, one dep", "[traverser]") { ArtifactFactory::DescribeLocalArtifact("dep", "repo")); { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); auto const traversed = traverser.Traverse({dep_id, output2_id}); CHECK(traversed); } @@ -583,14 +596,16 @@ TEST_CASE("Action with two outputs, actions depend on each of outputs", DependencyGraph g; CHECK(p.FillGraph(&g)); TestBuildInfo build_info; + std::atomic<bool> failed{}; std::string name = "This is a long name that shouldn't be corrupted"; build_info.SetName(name); SECTION("Traverse()") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); CHECK(traverser.Traverse()); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( @@ -606,7 +621,7 @@ TEST_CASE("Action with two outputs, actions depend on each of outputs", SECTION("Traverse(exec1)") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); auto const traversed = traverser.Traverse({exec1_id}); CHECK(traversed); } @@ -625,7 +640,7 @@ TEST_CASE("Action with two outputs, actions depend on each of outputs", SECTION("Traverse(exec2, output1)") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); auto const traversed = traverser.Traverse({output1_id, exec2_id}); CHECK(traversed); } @@ -644,7 +659,7 @@ TEST_CASE("Action with two outputs, actions depend on each of outputs", SECTION("Traverse(exec1, exec2)") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); auto const traversed = traverser.Traverse({exec1_id, exec2_id}); CHECK(traversed); } @@ -697,14 +712,16 @@ TEST_CASE("lib2 depends on lib1, executable depends on lib1 and lib2") { DependencyGraph g; CHECK(p.FillGraph(&g)); TestBuildInfo build_info; + std::atomic<bool> failed{}; std::string name = "This is a long name that shouldn't be corrupted "; build_info.SetName(name); SECTION(" Full build(without specifying artifacts) ") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); CHECK(traverser.Traverse()); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( @@ -720,9 +737,10 @@ TEST_CASE("lib2 depends on lib1, executable depends on lib1 and lib2") { SECTION("Full build (executable)") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); CHECK(traverser.Traverse({exec_id})); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( @@ -738,9 +756,10 @@ TEST_CASE("lib2 depends on lib1, executable depends on lib1 and lib2") { SECTION("Full build (executable + lib1)") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); CHECK(traverser.Traverse({exec_id, lib1_id})); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( @@ -756,9 +775,10 @@ TEST_CASE("lib2 depends on lib1, executable depends on lib1 and lib2") { SECTION("Full build (executable + lib2)") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); CHECK(traverser.Traverse({exec_id, lib2_id})); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( @@ -774,9 +794,10 @@ TEST_CASE("lib2 depends on lib1, executable depends on lib1 and lib2") { SECTION("Full build (executable + lib1 + lib2)") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); CHECK(traverser.Traverse({exec_id, lib1_id, lib2_id})); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( @@ -792,10 +813,11 @@ TEST_CASE("lib2 depends on lib1, executable depends on lib1 and lib2") { SECTION("First call does not build all artifacts") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); CHECK(traverser.Traverse({lib1_id})); CHECK(traverser.Traverse({exec_id})); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( @@ -814,9 +836,10 @@ TEST_CASE("lib2 depends on lib1, executable depends on lib1 and lib2") { "action") { { TestExecutor runner{&build_info}; - Traverser traverser(runner, g, kNumJobs); + Traverser traverser(runner, g, kNumJobs, &failed); CHECK(traverser.Traverse({lib2_id})); } + CHECK_FALSE(failed); CHECK_THAT( build_info.CorrectlyBuilt(), HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>( diff --git a/test/end-to-end/TARGETS b/test/end-to-end/TARGETS index 4737c7aa..a5586446 100644 --- a/test/end-to-end/TARGETS +++ b/test/end-to-end/TARGETS @@ -9,6 +9,7 @@ , [["./", "targets", "TESTS"], "targets"] , [["./", "user-errors", "TESTS"], "user-errors"] , [["./", "built-in-rules", "TESTS"], "built-in-rules"] + , [["./", "build-fails", "TESTS"], "build-fails"] ] } } diff --git a/test/end-to-end/build-fails/TARGETS b/test/end-to-end/build-fails/TARGETS new file mode 100644 index 00000000..05e2c957 --- /dev/null +++ b/test/end-to-end/build-fails/TARGETS @@ -0,0 +1,9 @@ +{ "single_fail_dep": + { "type": ["@", "rules", "shell/test", "script"] + , "name": ["single_fail_dep"] + , "test": ["single_fail_dep.sh"] + , "deps": [["test/end-to-end", "tool-under-test"]] + } +, "TESTS": + {"type": "install", "tainted": ["test"], "deps": ["single_fail_dep"]} +} diff --git a/test/end-to-end/build-fails/single_fail_dep.sh b/test/end-to-end/build-fails/single_fail_dep.sh new file mode 100755 index 00000000..c4e713b3 --- /dev/null +++ b/test/end-to-end/build-fails/single_fail_dep.sh @@ -0,0 +1,47 @@ +#!/bin/bash + +set -eu + +readonly NUM_DEPS=100 +readonly FAIL_DEP=42 + +create_target() { + local ID=$1 + if [ $ID = $FAIL_DEP ]; then + # failing target + echo '{"type": "generic", "cmds": ["false"], "out_dirs": ["out_'$ID'"]}' + else + # success target + echo '{"type": "generic", "cmds": ["echo '$ID' > out_'$ID'/id"], "out_dirs": ["out_'$ID'"]}' + fi +} + +mkdir -p .buildroot + +touch ROOT + +cat <<EOF > TARGETS +{ $(for i in $(seq $NUM_DEPS); do + if [ $i = 1 ]; then echo -n ' '; else echo -n ' , '; fi; + echo '"dep_'$i'": '$(create_target $i); done) +, "main": + { "type": "generic" + , "cmds": ["cat out_*/id | sort -n > id"] + , "outs": ["id"] + , "deps": + [ $(for i in $(seq $NUM_DEPS); do + if [ $i = 1 ]; then echo -n ' '; else echo -n ' , '; fi; + echo '"dep_'$i'"'; + done) + ] + } +} +EOF + +cat TARGETS + +echo "main" >&2 +bin/tool-under-test build --local-build-root .buildroot main && exit 1 || [ $? = 1 ] +echo "done" >&2 + +exit |