Record whether a command's last run failed (fixes #187)

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),

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<NodeId> inputs = {};  ///< Input file operands (for %f expansion)
     Vec<NodeId> outputs = {}; ///< Output file operands (for %o expansion)

include/pup/index/format.hpp

@@ -58,7 +58,10 @@ inline constexpr auto INDEX_MAGIC = std::array<char, 4> { '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

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<pup::StringId> const& parsed_dirs = {},
     pup::Vec<pup::StringId> const& available_dirs = {},
-    pup::Vec<pup::StringId> const& pruned_dirs = {}
+    pup::Vec<pup::StringId> 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<StringId> changed_outputs;
     pup::Vec<pup::NodeId> forced_cmds;
+    pup::Vec<pup::NodeId> 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<pup::graph::Display>(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<StringId> {};
     auto forced_cmds = pup::Vec<pup::NodeId> {};
 
@@ -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() };

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),
     };

test/e2e/fixtures/failed_command/Tupfile.fixture

@@ -0,0 +1,2 @@
+# Placeholder - overwritten by test
+: |> true |>

test/e2e/fixtures/failed_command/Tupfile.ini

@@ -0,0 +1 @@
+# Project root marker

test/e2e/fixtures/failed_command/tup.config

@@ -0,0 +1 @@
+# Test config

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")

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);
 }