diff --git a/include/pup/index/format.hpp b/include/pup/index/format.hpp index 874df617..a18f03b3 100644 --- a/include/pup/index/format.hpp +++ b/include/pup/index/format.hpp @@ -36,7 +36,12 @@ inline constexpr auto INDEX_MAGIC = std::array { 'P', 'U', 'P', 'I' }; /// 14 - Guard-unsatisfied commands are no longer serialized; v13 indexes carry /// their identities and would mask reactivated output-less commands as /// already known, so they never run (issue #118) -inline constexpr auto INDEX_VERSION = std::uint32_t { 14 }; +/// 15 - Command identity folds the rule's source directory; v14 identities are +/// directory-blind, so identical rules in sibling directories collide and +/// resolve to one another through the identity join (issue #167). Also +/// covers OrderOnly edges, persisted since issue #166 and load-bearing for +/// removal routing; v14 indexes predate them and would lose one removal. +inline constexpr auto INDEX_VERSION = std::uint32_t { 15 }; /// Index file header (56 bytes) - v9 struct alignas(8) RawHeader { diff --git a/src/cli/cmd_build.cpp b/src/cli/cmd_build.cpp index 8dc64ab1..3fbf81d3 100644 --- a/src/cli/cmd_build.cpp +++ b/src/cli/cmd_build.cpp @@ -979,7 +979,8 @@ auto is_dir_authoritative( /// Carry forward old-index commands from directories this run has no /// authoritative knowledge of, so a scoped build's saved index still describes /// the whole project (issue #125). Copies each command with its operand files -/// (stat data preserved) and Normal/Sticky edges, remapping ids to the new +/// (stat data preserved) and every edge except Implicit — OrderOnly among them, +/// which carries removal routing for order-only inputs — remapping ids to the new /// index's dense sequences; implicit edges are re-attached afterwards by /// preserve_old_implicit_edges through the identity match. auto merge_out_of_scope_commands( @@ -1469,11 +1470,11 @@ auto reconcile_input_set( if (!file) { continue; } - // The complementary half of the input edge types; expand_implicit_deps - // routes Implicit and Sticky through the same index for any changed file. + // Edge types that reach a command in one hop; expand_implicit_deps routes + // Implicit and Sticky. Group is neither: it runs file -> group, and the + // group -> command hop is a separate OrderOnly edge, so it needs two (#169). for (auto const* edge : idx.edges_from(pup::NodeId { file->id })) { - if (edge->type != pup::LinkType::Normal && edge->type != pup::LinkType::OrderOnly - && edge->type != pup::LinkType::Group) { + if (edge->type != pup::LinkType::Normal && edge->type != pup::LinkType::OrderOnly) { continue; } auto const* cmd = idx.find_command_by_id(pup::NodeId { edge->to }); diff --git a/src/graph/dag.cpp b/src/graph/dag.cpp index 2f244c1f..7b9a85d7 100644 --- a/src/graph/dag.cpp +++ b/src/graph/dag.cpp @@ -1035,10 +1035,16 @@ auto compute_command_identity(Graph const& graph, NodeId cmd_id, PathCache& cach { auto& pool = global_pool(); auto state = sha256_init(); + auto constexpr SEP = std::byte { 0 }; // Base: the fully-expanded command text (instruction + operand paths + in-text vars). state = sha256_update(state, pool.get(expand_instruction(graph, cmd_id, cache))); + // Command text is Tupfile-relative, so the same rule in sibling directories renders + // identically; without the directory those distinct rules share one identity. + state = sha256_update(state, std::span { &SEP, 1 }); + state = sha256_update(state, pool.get(get(graph, cmd_id))); + // Fold in (name, value-hash) of each Variable node reached via a Sticky edge. // This captures vars that affect output without appearing in the rendered text — // exported env vars the subprocess reads as $VAR, config vars gating an export, etc. @@ -1057,7 +1063,6 @@ auto compute_command_identity(Graph const& graph, NodeId cmd_id, PathCache& cach vars.end() ); - auto constexpr SEP = std::byte { 0 }; for (auto const& [name, value_hash] : vars) { state = sha256_update(state, std::span { &SEP, 1 }); state = sha256_update(state, name); diff --git a/test/e2e/fixtures/identity_collision/Tupfile.ini b/test/e2e/fixtures/identity_collision/Tupfile.ini new file mode 100644 index 00000000..e69de29b diff --git a/test/e2e/fixtures/identity_collision/a/Tupfile.fixture b/test/e2e/fixtures/identity_collision/a/Tupfile.fixture new file mode 100644 index 00000000..d7a4438e --- /dev/null +++ b/test/e2e/fixtures/identity_collision/a/Tupfile.fixture @@ -0,0 +1,2 @@ +: main.c |> gcc -c %f -o %o |> main.o +: main.o |> gcc %f -o %o |> program diff --git a/test/e2e/fixtures/identity_collision/a/config.h b/test/e2e/fixtures/identity_collision/a/config.h new file mode 100644 index 00000000..411dc946 --- /dev/null +++ b/test/e2e/fixtures/identity_collision/a/config.h @@ -0,0 +1,4 @@ +#ifndef CONFIG_H +#define CONFIG_H +#define VERSION 1 +#endif diff --git a/test/e2e/fixtures/identity_collision/a/main.c b/test/e2e/fixtures/identity_collision/a/main.c new file mode 100644 index 00000000..2ae5408d --- /dev/null +++ b/test/e2e/fixtures/identity_collision/a/main.c @@ -0,0 +1,3 @@ +#include +#include "config.h" +int main(void) { printf("Version %d\n", VERSION); return 0; } diff --git a/test/e2e/fixtures/identity_collision/b/Tupfile.fixture b/test/e2e/fixtures/identity_collision/b/Tupfile.fixture new file mode 100644 index 00000000..d7a4438e --- /dev/null +++ b/test/e2e/fixtures/identity_collision/b/Tupfile.fixture @@ -0,0 +1,2 @@ +: main.c |> gcc -c %f -o %o |> main.o +: main.o |> gcc %f -o %o |> program diff --git a/test/e2e/fixtures/identity_collision/b/config.h b/test/e2e/fixtures/identity_collision/b/config.h new file mode 100644 index 00000000..411dc946 --- /dev/null +++ b/test/e2e/fixtures/identity_collision/b/config.h @@ -0,0 +1,4 @@ +#ifndef CONFIG_H +#define CONFIG_H +#define VERSION 1 +#endif diff --git a/test/e2e/fixtures/identity_collision/b/main.c b/test/e2e/fixtures/identity_collision/b/main.c new file mode 100644 index 00000000..2ae5408d --- /dev/null +++ b/test/e2e/fixtures/identity_collision/b/main.c @@ -0,0 +1,3 @@ +#include +#include "config.h" +int main(void) { printf("Version %d\n", VERSION); return 0; } diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index c6d1481e..1ca228e4 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -1107,6 +1107,44 @@ SCENARIO("Implicit dependencies track header changes", "[e2e][incremental]") } } +SCENARIO("Implicit deps survive identical rules in sibling directories", "[e2e][incremental][identity]") +{ + // Command text is Tupfile-relative, so these two rules render the same string. + // If identity ignores the directory they collide, and one directory's header + // edges get attached to the other's command. + auto env = EnvGuard { "PUP_IMPLICIT_DEPS", "1" }; + + GIVEN("two directories whose rules and sources are byte-identical") + { + auto f = E2EFixture { "identity_collision" }; + REQUIRE(f.init().success()); + REQUIRE(f.build().success()); + REQUIRE(f.run("a/program").stdout_output == "Version 1\n"); + REQUIRE(f.run("b/program").stdout_output == "Version 1\n"); + + WHEN("only the second directory's header changes") + { + f.write_file("b/config.h", "#ifndef CONFIG_H\n" + "#define CONFIG_H\n" + "#define VERSION 2\n" + "#endif\n"); + auto result = f.build(); + + THEN("that directory rebuilds") + { + REQUIRE(result.success()); + REQUIRE(f.run("b/program").stdout_output == "Version 2\n"); + } + + THEN("the other directory is untouched") + { + REQUIRE(result.success()); + REQUIRE(f.run("a/program").stdout_output == "Version 1\n"); + } + } + } +} + SCENARIO("Implicit deps cover every source of a multi-source command", "[e2e][incremental]") { // gcc -M emits one rule per source; stopping at the first leaves b.h untracked and the program silently stale diff --git a/test/unit/test_graph.cpp b/test/unit/test_graph.cpp index 6c1997c7..bfa3c129 100644 --- a/test/unit/test_graph.cpp +++ b/test/unit/test_graph.cpp @@ -1636,3 +1636,45 @@ TEST_CASE("node_id encoding: 30-bit index roundtrips and kinds are mutually excl REQUIRE_FALSE(pup::node_id::is_file(pup::INVALID_NODE_ID)); } + +TEST_CASE("compute_command_identity separates rules by directory", "[graph][identity]") +{ + auto bs = make_build_graph(); + auto& g = bs.graph; + + // Command text is Tupfile-relative, so identical rules in sibling directories + // render the same string; identity must still tell them apart. + auto in_a = add_command_node(g, CommandNode { + .source_dir = intern("a"), + .instruction_id = intern("cat src.txt > out.txt"), + }); + auto in_b = add_command_node(g, CommandNode { + .source_dir = intern("b"), + .instruction_id = intern("cat src.txt > out.txt"), + }); + REQUIRE(in_a.has_value()); + REQUIRE(in_b.has_value()); + + SECTION("same text in different directories yields different identities") + { + CHECK(compute_command_identity(g, *in_a, bs.path_cache) + != compute_command_identity(g, *in_b, bs.path_cache)); + } + + SECTION("identity is stable for the same command") + { + CHECK(compute_command_identity(g, *in_a, bs.path_cache) + == compute_command_identity(g, *in_a, bs.path_cache)); + } + + SECTION("different text in the same directory yields different identities") + { + auto other = add_command_node(g, CommandNode { + .source_dir = intern("a"), + .instruction_id = intern("cat other.txt > out.txt"), + }); + REQUIRE(other.has_value()); + CHECK(compute_command_identity(g, *in_a, bs.path_cache) + != compute_command_identity(g, *other, bs.path_cache)); + } +}