diff --git a/include/pup/cli/index_serialize.hpp b/include/pup/cli/index_serialize.hpp index 5987ef82..8ec4ca0c 100644 --- a/include/pup/cli/index_serialize.hpp +++ b/include/pup/cli/index_serialize.hpp @@ -35,7 +35,8 @@ auto serialize_graph_nodes( auto serialize_command_nodes( graph::BuildGraph const& state, index::Index& index, - PathIdMap const& path_to_id + PathIdMap const& path_to_id, + NodeIdMap32 const& failed_cmds = {} ) -> NodeIdMap32; /// Serialize graph edges to the index (order-only edges are ephemeral), diff --git a/include/pup/index/entry.hpp b/include/pup/index/entry.hpp index 806a588b..46073947 100644 --- a/include/pup/index/entry.hpp +++ b/include/pup/index/entry.hpp @@ -56,6 +56,7 @@ struct CommandEntry { Hash256 key = {}; ///< Which rule this is: the cross-build join key Hash256 signature = {}; ///< What it will do: changes mean re-run, not a different rule + bool failed = false; ///< Its last run exited nonzero, so it must run again Vec inputs = {}; ///< Input file operands (for %f expansion) Vec outputs = {}; ///< Output file operands (for %o expansion) diff --git a/include/pup/index/format.hpp b/include/pup/index/format.hpp index cea9614b..10449acc 100644 --- a/include/pup/index/format.hpp +++ b/include/pup/index/format.hpp @@ -58,7 +58,10 @@ inline constexpr auto INDEX_MAGIC = std::array { 'P', 'U', 'P', 'I' }; /// `signature` (what it will do), because one value cannot both stay stable /// across a recipe edit and change when the recipe changes (issue #188). /// v18 entries carry neither field. -inline constexpr auto INDEX_VERSION = std::uint32_t { 19 }; +/// 20 - A command records whether its last run failed. v19 inferred that from the +/// output being absent, which is false whenever a failing command writes its +/// output before failing, so the failure was forgotten (issue #187). +inline constexpr auto INDEX_VERSION = std::uint32_t { 20 }; /// Index file header (56 bytes) - v9 struct alignas(8) RawHeader { @@ -106,11 +109,16 @@ struct alignas(8) RawCommandEntry { std::uint32_t cmd_offset = 0; ///< Offset to template string with %f/%o patterns (v8) std::uint32_t display_offset = 0; ///< Display text offset (length-prefixed) std::uint32_t env_offset = 0; ///< Environment variables offset (length-prefixed) + std::uint32_t flags = 0; ///< COMMAND_FLAG_* bits (v20) + std::uint32_t reserved = 0; ///< Explicit tail padding; keeps serialized bytes deterministic Hash256 key = {}; ///< Which rule this is, for the cross-build join (v19) Hash256 signature = {}; ///< What it will do, for deciding whether to re-run (v19) }; -static_assert(sizeof(RawCommandEntry) == 80, "RawCommandEntry must be 80 bytes"); +/// Its last run exited nonzero, so it must run again whatever its outputs look like. +inline constexpr auto COMMAND_FLAG_FAILED = std::uint32_t { 1 }; + +static_assert(sizeof(RawCommandEntry) == 88, "RawCommandEntry must be 88 bytes"); /// Raw edge entry (16 bytes) /// Represents dependencies between nodes diff --git a/src/cli/cmd_build.cpp b/src/cli/cmd_build.cpp index 0836e29f..63ba5a45 100644 --- a/src/cli/cmd_build.cpp +++ b/src/cli/cmd_build.cpp @@ -701,7 +701,8 @@ auto serialize_graph_nodes( auto serialize_command_nodes( pup::graph::BuildGraph const& state, pup::index::Index& index, - PathIdMap const& path_to_id + PathIdMap const& path_to_id, + pup::NodeIdMap32 const& failed_cmds ) -> pup::NodeIdMap32 { auto const& g = state.graph; @@ -736,6 +737,7 @@ auto serialize_command_nodes( .env = pup::StringId::Empty, .key = pup::graph::compute_command_key(g, id, state.path_cache), .signature = pup::graph::compute_command_signature(g, id, state.path_cache), + .failed = failed_cmds.contains(id), .inputs = std::move(inputs), .outputs = std::move(outputs), }; @@ -1222,6 +1224,7 @@ auto merge_out_of_scope_commands( .env = cmd.env, .key = cmd.key, .signature = cmd.signature, + .failed = cmd.failed, .inputs = std::move(new_inputs), .outputs = std::move(new_outputs), }); @@ -1347,13 +1350,14 @@ auto build_index( pup::parser::IgnoreList const& excludes = {}, pup::Vec const& parsed_dirs = {}, pup::Vec const& available_dirs = {}, - pup::Vec const& pruned_dirs = {} + pup::Vec const& pruned_dirs = {}, + pup::NodeIdMap32 const& failed_cmds = {} ) -> pup::index::Index { // Serialize file/directory nodes from the build graph auto [index, path_to_id] = serialize_graph_nodes(state, source_root, config_root, output_root); - auto cmd_remap = serialize_command_nodes(state, index, path_to_id); + auto cmd_remap = serialize_command_nodes(state, index, path_to_id, failed_cmds); serialize_edges(state, index, cmd_remap); @@ -1423,6 +1427,7 @@ auto validate_output_targets( struct NewCommands { pup::Vec changed_outputs; pup::Vec forced_cmds; + pup::Vec retry_after_failure; ///< Recorded as failed; still failed unless they succeed now }; /// Commands that must run because of what they are rather than because a file changed: @@ -1496,8 +1501,15 @@ auto detect_new_commands( } } + // A recorded failure outranks the signature: a command that failed must run again + // even when nothing about it changed, and its outputs must be treated as changed so + // consumers of whatever it half-wrote are rescheduled too. auto signature = pup::graph::compute_command_signature(g, id, state.path_cache); - if (previous && pup::hash_equal(previous->signature, signature)) { + auto retry_after_failure = previous != nullptr && previous->failed; + if (retry_after_failure) { + result.retry_after_failure.push_back(id); + } + if (previous && !retry_after_failure && pup::hash_equal(previous->signature, signature)) { continue; } @@ -1509,7 +1521,8 @@ auto detect_new_commands( } if (verbose) { auto display_sv = pup::global_pool().get(pup::graph::get(g, id)); - vprint(variant_name, " {}: {}\n", previous ? "Changed command" : "New command", display_sv); + auto reason = retry_after_failure ? "Failed last run" : (previous ? "Changed command" : "New command"); + vprint(variant_name, " {}: {}\n", reason, display_sv); } } return result; @@ -1818,6 +1831,10 @@ auto build_single_variant( auto index_path = pup::global_pool().get(ctx.layout().index_path()); auto const* old_idx_ptr = ctx.old_index(); auto use_incremental = false; + // Carries "has not succeeded since it last failed": seeded from the previous index, cleared + // only by a successful run. A build in which the command does not run at all -- a target or + // scoped build -- must not forget it. + auto failed_cmds = pup::NodeIdMap32 {}; auto changed_files = pup::Vec {}; auto forced_cmds = pup::Vec {}; @@ -1899,6 +1916,9 @@ auto build_single_variant( for (auto cmd_id : input_delta.forced_cmds) { forced_cmds.push_back(cmd_id); } + for (auto cmd_id : new_cmds.retry_after_failure) { + failed_cmds.set(cmd_id, 1); + } if (opts.rerun) { for (auto id : pup::graph::all_nodes(bs.graph)) { @@ -1978,7 +1998,11 @@ auto build_single_variant( scheduler.on_job_complete([&](pup::exec::BuildJob const& job, pup::exec::JobResult const& job_result) { auto& pool = pup::global_pool(); + if (job_result.success) { + failed_cmds.remove(job.id); + } if (!job_result.success) { + failed_cmds.set(job.id, 1); if (use_tty_progress) { pup::exec::finalize_progress(prev_lines); } @@ -2131,7 +2155,8 @@ auto build_single_variant( excludes, ctx.parsed_dirs(), ctx.available_dirs(), - ctx.pruned_dirs() + ctx.pruned_dirs(), + failed_cmds ) }; auto index_save_start = pup::SteadyClock::time_point { pup::SteadyClock::now() }; diff --git a/src/index/entry.cpp b/src/index/entry.cpp index 77ec8396..55b81c6b 100644 --- a/src/index/entry.cpp +++ b/src/index/entry.cpp @@ -68,6 +68,7 @@ auto CommandEntry::to_raw( raw.env_offset = env_offset; raw.key = key; raw.signature = signature; + raw.flags = failed ? COMMAND_FLAG_FAILED : 0U; return raw; } @@ -89,6 +90,7 @@ auto CommandEntry::from_raw( .env = global_pool().intern(env_str), .key = raw.key, .signature = raw.signature, + .failed = (raw.flags & COMMAND_FLAG_FAILED) != 0U, .inputs = std::move(inputs), .outputs = std::move(outputs), }; diff --git a/test/e2e/fixtures/failed_command/Tupfile.fixture b/test/e2e/fixtures/failed_command/Tupfile.fixture new file mode 100644 index 00000000..21d376eb --- /dev/null +++ b/test/e2e/fixtures/failed_command/Tupfile.fixture @@ -0,0 +1,2 @@ +# Placeholder - overwritten by test +: |> true |> diff --git a/test/e2e/fixtures/failed_command/Tupfile.ini b/test/e2e/fixtures/failed_command/Tupfile.ini new file mode 100644 index 00000000..05e60154 --- /dev/null +++ b/test/e2e/fixtures/failed_command/Tupfile.ini @@ -0,0 +1 @@ +# Project root marker diff --git a/test/e2e/fixtures/failed_command/tup.config b/test/e2e/fixtures/failed_command/tup.config new file mode 100644 index 00000000..f4bd2fae --- /dev/null +++ b/test/e2e/fixtures/failed_command/tup.config @@ -0,0 +1 @@ +# Test config diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index 02004e77..73e94044 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -4522,6 +4522,132 @@ SCENARIO("Editing a rule's recipe does not make it a different rule", "[e2e][ide } } +SCENARIO("A command that failed is re-run on the next build", "[e2e][incremental][failure]") +{ + GIVEN("a built project whose command is then changed to fail after writing its output") + { + auto f = E2EFixture { "failed_command" }; + f.write_file("Tupfile", ": src.txt |> cp %f %o |> out.txt\n"); + f.write_file("src.txt", "ORIGINAL\n"); + f.mkdir("build"); + REQUIRE(f.pup({ "configure", "-B", "build" }).success()); + REQUIRE(f.build({ "-B", "build" }).success()); + + WHEN("it fails under keep-going and the build is repeated") + { + f.write_file("src.txt", "NEWCONTENT\n"); + f.write_file("Tupfile", ": src.txt |> cp %f %o && false |> out.txt\n"); + REQUIRE_FALSE(f.build({ "-B", "build", "-k" }).success()); + + auto again = f.build({ "-B", "build", "-k" }); + + THEN("the failure is remembered rather than reported as up to date") + { + INFO("stdout: " << again.stdout_output); + REQUIRE(again.stdout_output.find("Nothing to do") == std::string::npos); + REQUIRE_FALSE(again.success()); + } + } + } +} + +SCENARIO("A failed command's consumer runs once the command succeeds", "[e2e][incremental][failure]") +{ + // The consumer's output already exists holding V1, so nothing schedules the consumer except + // propagation from the producer's re-run. Routing the retry through forced_cmds instead of + // changed_outputs leaves final.txt at V1 forever, which is what this pins. + GIVEN("a built producer/consumer pair whose producer then fails after writing partial output") + { + auto f = E2EFixture { "failed_command" }; + f.write_file("Tupfile", + ": src.txt |> sh -c 'if [ -f FAILMARK ]; then echo PARTIAL > %o; exit 1; else cp %f %o; fi' |> mid.txt\n" + ": mid.txt |> cp %f %o |> final.txt\n"); + f.write_file("src.txt", "V1\n"); + f.mkdir("build"); + REQUIRE(f.pup({ "configure", "-B", "build" }).success()); + REQUIRE(f.build({ "-B", "build", "-k" }).success()); + REQUIRE(f.read_file("build/final.txt") == "V1\n"); + + f.write_file("src.txt", "V2\n"); + f.write_file("FAILMARK", "x\n"); + REQUIRE_FALSE(f.build({ "-B", "build", "-k" }).success()); + REQUIRE(f.read_file("build/mid.txt") == "PARTIAL\n"); + + WHEN("the cause of the failure is removed and nothing else changes") + { + f.remove_file("FAILMARK"); + auto result = f.build({ "-B", "build", "-k" }); + + THEN("the producer re-runs and the consumer picks up the corrected output") + { + INFO("stdout: " << result.stdout_output); + REQUIRE(result.success()); + REQUIRE(f.read_file("build/mid.txt") == "V2\n"); + REQUIRE(f.read_file("build/final.txt") == "V2\n"); + } + } + } +} + +SCENARIO("A target build does not forget another command's failure", "[e2e][incremental][failure]") +{ + GIVEN("one failed command and one healthy command") + { + auto f = E2EFixture { "failed_command" }; + f.write_file("Tupfile", + ": a.txt |> cp %f %o && false |> outa.txt\n" + ": b.txt |> cp %f %o |> outb.txt\n"); + f.write_file("a.txt", "A\n"); + f.write_file("b.txt", "B\n"); + f.mkdir("build"); + REQUIRE(f.pup({ "configure", "-B", "build" }).success()); + REQUIRE_FALSE(f.build({ "-B", "build", "-k" }).success()); + + WHEN("only the healthy target is built, then the whole project") + { + (void)f.build({ "-B", "build", "-k", "build/outb.txt" }); + auto full = f.build({ "-B", "build", "-k" }); + + THEN("the failure is still remembered") + { + INFO("stdout: " << full.stdout_output); + REQUIRE(full.stdout_output.find("Nothing to do") == std::string::npos); + REQUIRE_FALSE(full.success()); + } + } + } +} + +SCENARIO("A scoped build does not forget an out-of-scope failure", "[e2e][incremental][failure][scope]") +{ + GIVEN("a failing command in one directory and a healthy one in another") + { + auto f = E2EFixture { "failed_command" }; + f.mkdir("a"); + f.mkdir("b"); + f.write_file("a/Tupfile", ": a.txt |> cp %f %o && false |> outa.txt\n"); + f.write_file("b/Tupfile", ": b.txt |> cp %f %o |> outb.txt\n"); + f.write_file("a/a.txt", "A\n"); + f.write_file("b/b.txt", "B\n"); + f.mkdir("build"); + REQUIRE(f.pup({ "configure", "-B", "build" }).success()); + REQUIRE_FALSE(f.build({ "-B", "build", "-k" }).success()); + + WHEN("only the healthy directory is built, then the whole project") + { + (void)f.build({ "-B", "build", "-k", "b/" }); + auto full = f.build({ "-B", "build", "-k" }); + + THEN("the out-of-scope failure is still remembered") + { + INFO("stdout: " << full.stdout_output); + REQUIRE(full.stdout_output.find("Nothing to do") == std::string::npos); + REQUIRE_FALSE(full.success()); + } + } + } +} + SCENARIO("A glob's %f order does not depend on the build directory's name", "[e2e][glob][pathspace]") { GIVEN("a glob matching one generated and one source file, built out-of-tree") diff --git a/test/unit/test_index.cpp b/test/unit/test_index.cpp index b9861250..7d2ab3f4 100644 --- a/test/unit/test_index.cpp +++ b/test/unit/test_index.cpp @@ -80,9 +80,9 @@ TEST_CASE("Index format struct sizes", "[index]") REQUIRE(sizeof(RawFileEntry) == 64); } - SECTION("RawCommandEntry is 80 bytes (v19: + key and signature hashes)") + SECTION("RawCommandEntry is 88 bytes (v20: + flags)") { - REQUIRE(sizeof(RawCommandEntry) == 80); + REQUIRE(sizeof(RawCommandEntry) == 88); } SECTION("RawEdge is 16 bytes") @@ -178,6 +178,7 @@ TEST_CASE("CommandEntry conversion", "[index]") .env = intern("CC=gcc"), .key = key, .signature = signature, + .failed = true, .inputs = { 10 }, .outputs = { 20 }, }; @@ -190,6 +191,7 @@ TEST_CASE("CommandEntry conversion", "[index]") REQUIRE(raw.env_offset == 100); REQUIRE(raw.key == key); REQUIRE(raw.signature == signature); + REQUIRE(raw.flags == pup::index::COMMAND_FLAG_FAILED); // ID is computed from array index (4 + 1 = 5, then node_id::make_command) auto& pool = global_pool(); @@ -205,6 +207,7 @@ TEST_CASE("CommandEntry conversion", "[index]") REQUIRE(restored.env == cmd.env); REQUIRE(restored.key == key); REQUIRE(restored.signature == signature); + REQUIRE(restored.failed); REQUIRE(restored.inputs == cmd.inputs); REQUIRE(restored.outputs == cmd.outputs); }