From 1ee5c0bfb55b61a9236aee43e1a0511c8c2e80a8 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 27 Jul 2026 18:43:33 +0800 Subject: [PATCH 1/4] Reject two commands that share one identity (fixes #170) Command identity is the cross-build join key: find_by_identity maps an index entry onto a graph node, and expand_implicit_deps, reconcile_input_set, remove_stale_outputs, preserve_old_implicit_edges and merge_out_of_scope_commands all rely on that answer being unique. Nothing enforced it. build_identity_map appended without a uniqueness check and find_by_identity is a lower_bound, so a collision silently returned whichever twin sorted first. Identity is H(rendered text || source_dir || sticky (name, value-hash) pairs). Operand paths reach it only through % flags, so two rules in one directory naming neither %f nor %o collide. Both shapes built cleanly, exit 0: : a.txt |> ./check |> no outputs, so the duplicate-output check at : b.txt |> ./check |> builder.cpp:2063 never fires : a.txt |> ./gen |> out1.txt outputs differ, so it does not fire either : b.txt |> ./gen |> out2.txt finalize_graph now rejects both, naming the ambiguous command. It runs after pass 2 because % replacement rewrites command text -- identity is not final until every rewrite has landed. Upstream tup rejects the same shape: its command node is keyed on (dir, command string), so a second identical rule resolves to the same node and tupid_tree_add reports a duplicate command id. Only guard-satisfied commands are considered. Complementary branches of one conditional are both in the graph under the phi model and legitimately render the same text; at most one is ever live, and the index records only the live one. build_identity_map now applies that same filter -- it was the one walk over graph commands that did not, while serialize_command_nodes, detect_new_commands, collect_inactive_output_paths, cmd_show and the scheduler all do. Leaving the map wider than the set injectivity is enforced over would have readmitted exactly the duplicate keys this rejection exists to rule out. context.cpp discarded finalize_graph's Result with a (void) cast, so nothing it returned could ever reach a user; it now propagates. hash_less moves to core/hash.hpp beside hash_equal, collapsing three copies of the same memcmp comparator in cmd_build.cpp. Cost, measured with --stat: 3.5ms on a 5000-command graph and 0.4ms on putup's own 122-command graph. Full builds and parse --check pay it; incremental builds already computed identity for every command. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA --- include/pup/core/hash.hpp | 4 ++ src/cli/cmd_build.cpp | 31 ++++------ src/cli/context.cpp | 5 +- src/core/hash.cpp | 5 ++ src/graph/builder.cpp | 42 ++++++++++++- .../duplicate_command/Tupfile.fixture | 2 + .../fixtures/duplicate_command/Tupfile.ini | 1 + .../e2e/fixtures/duplicate_command/tup.config | 1 + test/unit/test_builder.cpp | 62 +++++++++++++++++++ test/unit/test_e2e.cpp | 53 ++++++++++++++++ 10 files changed, 186 insertions(+), 20 deletions(-) create mode 100644 test/e2e/fixtures/duplicate_command/Tupfile.fixture create mode 100644 test/e2e/fixtures/duplicate_command/Tupfile.ini create mode 100644 test/e2e/fixtures/duplicate_command/tup.config diff --git a/include/pup/core/hash.hpp b/include/pup/core/hash.hpp index 03a21068..80f5ab5f 100644 --- a/include/pup/core/hash.hpp +++ b/include/pup/core/hash.hpp @@ -64,6 +64,10 @@ auto hex_to_hash(std::string_view hex) -> Result; [[nodiscard]] auto hash_equal(Hash256 const& a, Hash256 const& b) -> bool; +/// Byte-lexicographic order, for sorting and binary-searching hash-keyed ranges. +[[nodiscard]] +auto hash_less(Hash256 const& a, Hash256 const& b) -> bool; + /// Zero hash constant inline constexpr auto ZERO_HASH = Hash256 {}; diff --git a/src/cli/cmd_build.cpp b/src/cli/cmd_build.cpp index e1384ef1..2229666f 100644 --- a/src/cli/cmd_build.cpp +++ b/src/cli/cmd_build.cpp @@ -850,15 +850,12 @@ auto preserve_old_implicit_edges( // positional and shift across builds (e.g. when an earlier-created command is // removed), so the old edge's `to` id cannot be trusted to mean the same command. // Identity is stable, so we re-resolve each carried edge's command through it. - auto hash_less = [](pup::Hash256 const& a, pup::Hash256 const& b) { - return std::memcmp(a.data(), b.data(), a.size()) < 0; - }; auto identity_to_new_id = pup::Vec> {}; identity_to_new_id.reserve(ctx.index.commands().size()); for (auto const& cmd : ctx.index.commands()) { identity_to_new_id.emplace_back(cmd.identity, cmd.id); } - std::sort(identity_to_new_id.begin(), identity_to_new_id.end(), [&](auto const& a, auto const& b) { return hash_less(a.first, b.first); }); + std::sort(identity_to_new_id.begin(), identity_to_new_id.end(), [&](auto const& a, auto const& b) { return pup::hash_less(a.first, b.first); }); for (auto const& edge : old_index.edges()) { if (edge.type != pup::LinkType::Implicit) { @@ -872,7 +869,7 @@ auto preserve_old_implicit_edges( if (!old_cmd) { continue; } - auto match = std::lower_bound(identity_to_new_id.begin(), identity_to_new_id.end(), old_cmd->identity, [&](auto const& p, pup::Hash256 const& key) { return hash_less(p.first, key); }); + auto match = std::lower_bound(identity_to_new_id.begin(), identity_to_new_id.end(), old_cmd->identity, [&](auto const& p, pup::Hash256 const& key) { return pup::hash_less(p.first, key); }); if (match == identity_to_new_id.end() || match->first != old_cmd->identity) { continue; } @@ -904,34 +901,32 @@ auto preserve_old_implicit_edges( } } -auto hash_less(pup::Hash256 const& a, pup::Hash256 const& b) -> bool -{ - return std::memcmp(a.data(), b.data(), a.size()) < 0; -} - /// Sorted (identity → graph NodeId) map: the cross-build join key for commands. using IdentityMap = Vec>; +/// Guard-satisfied only: that is the set the index records, and the set finalize_graph +/// enforces injectivity over. Widening it here would readmit the duplicate keys that +/// enforcement exists to rule out. auto build_identity_map(pup::graph::BuildGraph const& state) -> IdentityMap { auto map = IdentityMap {}; for (auto id : pup::graph::all_nodes(state.graph)) { - if (pup::node_id::is_command(id)) { + if (pup::node_id::is_command(id) && pup::graph::is_guard_satisfied(state.graph, id)) { map.emplace_back( pup::graph::compute_command_identity(state.graph, id, state.path_cache), id ); } } - std::sort(map.begin(), map.end(), [](auto const& a, auto const& b) { return hash_less(a.first, b.first); }); + std::sort(map.begin(), map.end(), [](auto const& a, auto const& b) { return pup::hash_less(a.first, b.first); }); return map; } auto find_by_identity(IdentityMap const& map, pup::Hash256 const& identity) -> std::optional { auto const* it = std::lower_bound( - map.begin(), map.end(), identity, [](auto const& p, auto const& k) { return hash_less(p.first, k); } + map.begin(), map.end(), identity, [](auto const& p, auto const& k) { return pup::hash_less(p.first, k); } ); - if (it == map.end() || std::memcmp(it->first.data(), identity.data(), identity.size()) != 0) { + if (it == map.end() || !pup::hash_equal(it->first, identity)) { return std::nullopt; } return it->second; @@ -1000,7 +995,7 @@ auto merge_out_of_scope_commands( for (auto const& cmd : ctx.index.commands()) { new_identities.push_back(cmd.identity); } - std::sort(new_identities.begin(), new_identities.end(), hash_less); + std::sort(new_identities.begin(), new_identities.end(), pup::hash_less); // serialize_graph_nodes registers File/Generated/Directory paths but not // Ghosts; without this the merge would duplicate a ghost's entry by path. @@ -1096,7 +1091,7 @@ auto merge_out_of_scope_commands( if (is_dir_authoritative(old_index, cmd.dir_id, parse_scopes, excludes, parsed_dirs, available_dirs, pruned_dirs)) { continue; } - if (std::binary_search(new_identities.begin(), new_identities.end(), cmd.identity, hash_less)) { + if (std::binary_search(new_identities.begin(), new_identities.end(), cmd.identity, pup::hash_less)) { continue; } if (any_dep_changed(cmd)) { @@ -1362,7 +1357,7 @@ auto detect_new_commands( for (auto const& cmd : idx.commands()) { old_identities.push_back(cmd.identity); } - std::sort(old_identities.begin(), old_identities.end(), hash_less); + std::sort(old_identities.begin(), old_identities.end(), pup::hash_less); for (auto id : pup::graph::all_nodes(g)) { if (!pup::node_id::is_command(id)) { @@ -1372,7 +1367,7 @@ auto detect_new_commands( continue; } auto identity = pup::graph::compute_command_identity(g, id, state.path_cache); - if (!std::binary_search(old_identities.begin(), old_identities.end(), identity, hash_less)) { + if (!std::binary_search(old_identities.begin(), old_identities.end(), identity, pup::hash_less)) { auto pushed_outputs = false; for (auto output_id : pup::graph::get_outputs(g, id)) { auto output_path_sv = pup::graph::get_full_path(g, output_id, state.path_cache); diff --git a/src/cli/context.cpp b/src/cli/context.cpp index 79697b21..ab1b2b8a 100644 --- a/src/cli/context.cpp +++ b/src/cli/context.cpp @@ -948,7 +948,10 @@ auto build_context( } } - (void)graph::finalize_graph(ctx.impl_->graph, builder_state); + auto finalized = graph::finalize_graph(ctx.impl_->graph, builder_state); + if (!finalized) { + return unexpected(finalized.error()); + } for (auto warning_id : builder_state.warnings) { eprint("warning: {}\n", pool.get(warning_id)); diff --git a/src/core/hash.cpp b/src/core/hash.cpp index 52e34478..dec02ce8 100644 --- a/src/core/hash.cpp +++ b/src/core/hash.cpp @@ -213,4 +213,9 @@ auto hash_equal(Hash256 const& a, Hash256 const& b) -> bool return std::memcmp(a.data(), b.data(), a.size()) == 0; } +auto hash_less(Hash256 const& a, Hash256 const& b) -> bool +{ + return std::memcmp(a.data(), b.data(), a.size()) < 0; +} + } // namespace pup diff --git a/src/graph/builder.cpp b/src/graph/builder.cpp index c56d3960..2111c718 100644 --- a/src/graph/builder.cpp +++ b/src/graph/builder.cpp @@ -2500,6 +2500,44 @@ auto add_tupfile( return {}; } +/// Command identity is the cross-build join key, and every consumer looks a command up by +/// identity alone; two commands sharing one make that lookup answer arbitrarily. Reject the +/// graph here rather than join it wrong later. Only guard-satisfied commands are considered, +/// matching the set the index records: complementary branches of one conditional legitimately +/// render the same text, and at most one of them is ever live. +auto reject_ambiguous_identities(BuildGraph& build_state) -> Result +{ + auto& g = build_state.graph; + + auto identities = Vec> {}; + for (auto id : all_nodes(g)) { + if (!node_id::is_command(id) || !is_guard_satisfied(g, id)) { + continue; + } + identities.emplace_back(compute_command_identity(g, id, build_state.path_cache), id); + } + + std::sort(identities.begin(), identities.end(), [](auto const& a, auto const& b) { + return hash_less(a.first, b.first); + }); + auto const* dup = std::adjacent_find(identities.begin(), identities.end(), [](auto const& a, auto const& b) { + return hash_equal(a.first, b.first); + }); + if (dup == identities.end()) { + return {}; + } + + auto dir_sv = str(get(g, dup->second)); + auto err = Buf {}; + err.fmt( + "Duplicate command in '{}': two rules render the same command line, so no build can " + "tell them apart:\n {}", + dir_sv.empty() ? "." : dir_sv, + str(expand_instruction(g, dup->second, build_state.path_cache)) + ); + return make_error(ErrorCode::DuplicateNode, err.view()); +} + auto finalize_graph( BuildGraph& build_state, Builder& state @@ -2628,7 +2666,9 @@ auto finalize_graph( } state.deferred_edges.clear(); - return {}; + + // Last: pass 2 rewrites command text, so identity is not final before it. + return reject_ambiguous_identities(build_state); } } // namespace pup::graph diff --git a/test/e2e/fixtures/duplicate_command/Tupfile.fixture b/test/e2e/fixtures/duplicate_command/Tupfile.fixture new file mode 100644 index 00000000..28e2e628 --- /dev/null +++ b/test/e2e/fixtures/duplicate_command/Tupfile.fixture @@ -0,0 +1,2 @@ +: |> ./gen |> a.out +: |> ./gen |> b.out diff --git a/test/e2e/fixtures/duplicate_command/Tupfile.ini b/test/e2e/fixtures/duplicate_command/Tupfile.ini new file mode 100644 index 00000000..05e60154 --- /dev/null +++ b/test/e2e/fixtures/duplicate_command/Tupfile.ini @@ -0,0 +1 @@ +# Project root marker diff --git a/test/e2e/fixtures/duplicate_command/tup.config b/test/e2e/fixtures/duplicate_command/tup.config new file mode 100644 index 00000000..f4bd2fae --- /dev/null +++ b/test/e2e/fixtures/duplicate_command/tup.config @@ -0,0 +1 @@ +# Test config diff --git a/test/unit/test_builder.cpp b/test/unit/test_builder.cpp index 32628ed1..e9cde663 100644 --- a/test/unit/test_builder.cpp +++ b/test/unit/test_builder.cpp @@ -1892,3 +1892,65 @@ TEST_CASE("GraphBuilder variable assigned under a config branch keeps later cond CHECK(wrapped > baseline); } + +namespace { + +auto finalize_tupfile( + BuilderTestFixture const& fixture, + std::string_view source, + VarDb const* config_vars = nullptr +) -> pup::Result +{ + auto bs = make_build_graph(); + auto vars = VarDb {}; + auto ctx = EvalContext { .vars = &vars, .config_vars = config_vars }; + + auto options = BuilderOptions { + .source_root = intern(fixture.root_str()), + .config_root = intern(fixture.root_str()), + .output_root = pup::StringId::Empty, + .config_path = pup::StringId::Empty, + .expand_globs = false, + .validate_inputs = false, + }; + auto builder_state = make_builder(options); + + auto parse_result = parse_tupfile(source, fixture.tupfile_path("")); + REQUIRE(parse_result.success()); + REQUIRE(add_tupfile(bs, parse_result.tupfile, ctx, builder_state).has_value()); + return finalize_graph(bs, builder_state); +} + +} // namespace + +TEST_CASE("GraphBuilder rejects two commands that share one identity", "[builder][identity]") +{ + auto fixture = BuilderTestFixture {}; + + auto result = finalize_tupfile(fixture, ": |> ./gen |> a.out\n: |> ./gen |> b.out\n"); + + REQUIRE_FALSE(result.has_value()); + CHECK(sv(result.error().message).find("./gen") != std::string_view::npos); +} + +TEST_CASE("GraphBuilder accepts commands whose rendered text differs", "[builder][identity]") +{ + auto fixture = BuilderTestFixture {}; + + CHECK(finalize_tupfile(fixture, ": |> ./gen a |> a.out\n: |> ./gen b |> b.out\n").has_value()); +} + +TEST_CASE("GraphBuilder accepts identical text under complementary guards", "[builder][identity][phi]") +{ + auto fixture = BuilderTestFixture {}; + auto config = VarDb {}; + config.set("M", "y"); + + auto result = finalize_tupfile( + fixture, + "ifeq (@(M),y)\n: |> ./gen |> out.txt\nelse\n: |> ./gen |> out.txt\nendif\n", + &config + ); + + CHECK(result.has_value()); +} diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index 19230bb8..b201b42b 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -4473,6 +4473,59 @@ SCENARIO("Duplicate output detection", "[e2e][duplicate]") } } +SCENARIO("Duplicate command detection", "[e2e][duplicate][identity]") +{ + GIVEN("a Tupfile with two rules that render the same command line") + { + auto f = E2EFixture { "duplicate_command" }; + + WHEN("pup parses the project") + { + auto result = f.build(); + + THEN("build fails naming the ambiguous command") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(result.success()); + REQUIRE(result.stderr_output.find("Duplicate command") != std::string::npos); + REQUIRE(result.stderr_output.find("./gen") != std::string::npos); + } + } + } +} + +SCENARIO("Complementary branches may render the same command line", "[e2e][duplicate][identity][phi]") +{ + GIVEN("a project whose two conditional branches carry identical rule text") + { + auto f = E2EFixture { "phi_same_output" }; + f.write_file("Tupfile", R"( +ifeq (@(MODE),release) +: input.txt |> cp %f %o |> output.txt +else +: input.txt |> cp %f %o |> output.txt +endif +)"); + f.write_file("input.txt", "test\n"); + f.mkdir("build"); + f.write_file("build/tup.config", "CONFIG_MODE=debug\n"); + REQUIRE(f.pup({ "configure", "-B", "build" }).success()); + + WHEN("built") + { + auto result = f.build({ "-B", "build" }); + + THEN("only the guard-satisfied branch counts, so the build succeeds") + { + INFO("stderr: " << result.stderr_output); + REQUIRE(result.success()); + REQUIRE(f.exists("build/output.txt")); + } + } + } +} + // ============================================================================= // Platform Conditional Tests // ============================================================================= From 90a56b12c3f4f7bb10ad288811eb04312f8ab913 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:03:50 +0800 Subject: [PATCH 2/4] Distinguish dep-scan commands by parent; exempt empty-rendered ones CI found two false positives in the duplicate-command rejection, both real graphs that the rule wrongly called ambiguous. A dep-scan command is generated per compile with the parent's -o dropped, so two compiles of one source with equal flags render byte-identical scans. The GCC BSP example does exactly that and was rejected on: g++ -M -std=gnu++14 -DIN_GCC ... ggc-none.cc Those two scans are not duplicates -- they inject deps into different parents -- but nothing in their identity said so, which is the same defect #170 is about one level down: the discriminator was missing from the hash, so find_by_identity could route a scan's implicit deps to the wrong compile. Identity now folds the parent command's identity, one level deep since a parent is rule-authored and has no parent of its own. INDEX_VERSION 17 -> 18: every dep-scan identity changes. The configure pass evaluates before tup.config exists, so every rule gated on an unset config var renders empty and collides with the next one. That is why the coverage and Windows configure steps failed with an empty command line in the message. Those commands never run -- a real build rejects empty-rendered commands outright (#148) and configure schedules only the config-generating rules -- so the injectivity check skips them. Verified against both failures: the coverage configure pass completes, and the GCC BSP example parses (24 Tupfiles, 1853 commands) and builds past 2800 of 3531 commands where it previously died during graph construction. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA --- include/pup/index/format.hpp | 6 ++++- src/graph/builder.cpp | 7 ++++++ src/graph/dag.cpp | 9 +++++++ test/unit/test_builder.cpp | 18 +++++++++++++- test/unit/test_graph.cpp | 46 ++++++++++++++++++++++++++++++++++++ 5 files changed, 84 insertions(+), 2 deletions(-) diff --git a/include/pup/index/format.hpp b/include/pup/index/format.hpp index c05bd157..1fa0665a 100644 --- a/include/pup/index/format.hpp +++ b/include/pup/index/format.hpp @@ -50,7 +50,11 @@ inline constexpr auto INDEX_MAGIC = std::array { 'P', 'U', 'P', 'I' }; /// both the membership and the order of %f changed for any pattern a /// generated file matches (issues #177, #178); v16 identities no longer /// join for those commands. -inline constexpr auto INDEX_VERSION = std::uint32_t { 17 }; +/// 18 - A dep-scan command's identity folds its parent command's identity. Two +/// compiles of one source with equal flags render byte-identical scans, so +/// v17 gave them one identity and the join between them was arbitrary +/// (issue #170); every dep-scan command's identity changes. +inline constexpr auto INDEX_VERSION = std::uint32_t { 18 }; /// Index file header (56 bytes) - v9 struct alignas(8) RawHeader { diff --git a/src/graph/builder.cpp b/src/graph/builder.cpp index 2111c718..aaed4e39 100644 --- a/src/graph/builder.cpp +++ b/src/graph/builder.cpp @@ -2514,6 +2514,13 @@ auto reject_ambiguous_identities(BuildGraph& build_state) -> Result if (!node_id::is_command(id) || !is_guard_satisfied(g, id)) { continue; } + // The configure pass evaluates before tup.config exists, so every rule gated on an + // unset config var renders empty and collides with the next one. Those commands + // never run: a real build rejects them outright, and configure schedules only the + // config-generating rules. + if (str(expand_instruction(g, id, build_state.path_cache)).find_first_not_of(" \t") == std::string_view::npos) { + continue; + } identities.emplace_back(compute_command_identity(g, id, build_state.path_cache), id); } diff --git a/src/graph/dag.cpp b/src/graph/dag.cpp index 7b9a85d7..402f935d 100644 --- a/src/graph/dag.cpp +++ b/src/graph/dag.cpp @@ -1045,6 +1045,15 @@ auto compute_command_identity(Graph const& graph, NodeId cmd_id, PathCache& cach state = sha256_update(state, std::span { &SEP, 1 }); state = sha256_update(state, pool.get(get(graph, cmd_id))); + // A dep-scan command drops its parent's -o, so two compiles of one source with equal + // flags render byte-identical scans that differ only in whose deps they inject. + // Recursion is one level: a parent is a rule-authored command and has no parent. + if (auto parent = get_parent_command(graph, cmd_id); parent != INVALID_NODE_ID) { + auto parent_identity = compute_command_identity(graph, parent, cache); + state = sha256_update(state, std::span { &SEP, 1 }); + state = sha256_update(state, std::span { parent_identity.data(), parent_identity.size() }); + } + // 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. diff --git a/test/unit/test_builder.cpp b/test/unit/test_builder.cpp index e9cde663..3e96f111 100644 --- a/test/unit/test_builder.cpp +++ b/test/unit/test_builder.cpp @@ -1898,7 +1898,8 @@ namespace { auto finalize_tupfile( BuilderTestFixture const& fixture, std::string_view source, - VarDb const* config_vars = nullptr + VarDb const* config_vars = nullptr, + bool reject_empty_commands = true ) -> pup::Result { auto bs = make_build_graph(); @@ -1912,6 +1913,7 @@ auto finalize_tupfile( .config_path = pup::StringId::Empty, .expand_globs = false, .validate_inputs = false, + .reject_empty_commands = reject_empty_commands, }; auto builder_state = make_builder(options); @@ -1940,6 +1942,20 @@ TEST_CASE("GraphBuilder accepts commands whose rendered text differs", "[builder CHECK(finalize_tupfile(fixture, ": |> ./gen a |> a.out\n: |> ./gen b |> b.out\n").has_value()); } +TEST_CASE("GraphBuilder accepts empty-rendered duplicates in the configure pass", "[builder][identity][configure]") +{ + auto fixture = BuilderTestFixture {}; + + // The configure pass evaluates before tup.config exists, so every rule gated on an + // unset config var renders empty and collides with the next one. Those commands are + // never scheduled -- only the config-generating rules are -- so the collision is inert. + auto result = finalize_tupfile( + fixture, ": |> @(CC) |> a.out\n: |> @(CC) |> b.out\n", nullptr, false + ); + + CHECK(result.has_value()); +} + TEST_CASE("GraphBuilder accepts identical text under complementary guards", "[builder][identity][phi]") { auto fixture = BuilderTestFixture {}; diff --git a/test/unit/test_graph.cpp b/test/unit/test_graph.cpp index bfa3c129..b7ba5e7d 100644 --- a/test/unit/test_graph.cpp +++ b/test/unit/test_graph.cpp @@ -1678,3 +1678,49 @@ TEST_CASE("compute_command_identity separates rules by directory", "[graph][iden != compute_command_identity(g, *other, bs.path_cache)); } } + +TEST_CASE("compute_command_identity separates dep-scan commands by parent", "[graph][identity]") +{ + auto bs = make_build_graph(); + auto& g = bs.graph; + + // A dep-scan command drops its parent's -o, so two compiles of one source with + // equal flags render byte-identical scans; only the parent tells them apart. + auto compile_a = add_command_node(g, CommandNode { + .source_dir = intern("a"), + .instruction_id = intern("g++ -c ggc.cc -o one.o"), + }); + auto compile_b = add_command_node(g, CommandNode { + .source_dir = intern("a"), + .instruction_id = intern("g++ -c ggc.cc -o two.o"), + }); + REQUIRE(compile_a.has_value()); + REQUIRE(compile_b.has_value()); + + auto scan_a = add_command_node(g, CommandNode { + .source_dir = intern("a"), + .instruction_id = intern("g++ -M ggc.cc"), + .output_action = OutputAction::InjectImplicitDeps, + .parent_command = *compile_a, + }); + auto scan_b = add_command_node(g, CommandNode { + .source_dir = intern("a"), + .instruction_id = intern("g++ -M ggc.cc"), + .output_action = OutputAction::InjectImplicitDeps, + .parent_command = *compile_b, + }); + REQUIRE(scan_a.has_value()); + REQUIRE(scan_b.has_value()); + + SECTION("identical scan text under different parents yields different identities") + { + CHECK(compute_command_identity(g, *scan_a, bs.path_cache) + != compute_command_identity(g, *scan_b, bs.path_cache)); + } + + SECTION("a scan's identity is stable") + { + CHECK(compute_command_identity(g, *scan_a, bs.path_cache) + == compute_command_identity(g, *scan_a, bs.path_cache)); + } +} From c08c7ef39194b22409929031426fba56f82b5f92 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 27 Jul 2026 20:20:18 +0800 Subject: [PATCH 3/4] Split command identity into a join key and a signature (fixes #188) One hash served two jobs that want opposite things. As the cross-build join key it wants to stay stable so a command keeps its history; as the change detector it wants to move the moment anything affecting the output changes. Fused, it could only do the second, so editing a rule's recipe made it a different rule: : in.txt |> cp %f %o |> out.txt -> : in.txt |> cat %f > %o |> out.txt New command: Removed stale: out.txt Removed command: Same rule, same inputs, same outputs, an edited recipe -- and putup deletes the artifact, buries the old command and adopts a stranger. Every consumer that joins by identity (implicit-dep routing, input-set reconciliation, stale-output removal, carried implicit edges, out-of-scope merge) loses the command's history with it. Upstream tup does not conflate them. From parser.c:3573: /* If we already have our command string in the db, then use that. * Otherwise, we try to find an existing command of a different * name that points to the output files we are trying to create. */ find_existing_command walks the rule's outputs and takes each one's incoming link. When that join lands on a command with a different string, tup calls tup_db_set_name and sets command_modified -- the node survives, the string is the change signal, not the name. So: key which rule this is. A command that produces files is named by the files: output ownership is already unique among guard-satisfied commands, enforced where the output edge is created. Output-less commands have nothing else to be named by and fall back to a textual key of (text, dir, parent key). signature what it will do: rendered text, source dir, and the values of the vars it depends on. Compared only after a join succeeds. The join key is the output path itself, which both the graph and the index already hold as edges, so no lookup structure needs a hash at all -- the command index phase drops from 0.5ms to 0.0ms on putup's own graph and 3.5ms to 1.6ms on a 5000-command one. detect_new_commands now joins first and compares signatures, distinguishing "Changed command" from "New command". reject_ambiguous_keys narrows accordingly: two rules rendering the same text are only ambiguous when neither produces anything, so the pair that #170 rejected is now legal and correctly told apart by its outputs. The payoff beyond the deletion: a term added to the signature no longer breaks every join, so the next version bump costs one re-run per command instead of discarding the index's knowledge of which command is which. INDEX_VERSION 18 -> 19: RawCommandEntry carries key and signature, 48 -> 80 bytes. Verified: recipe edit keeps the artifact and reports "Changed command"; a deleted rule still removes its output while its neighbour's survives; a renamed output is correctly a new command with the old artifact removed. GCC BSP parses (24 Tupfiles, 1853 commands), busybox parses (31 Tupfiles, 293 commands), coverage configure completes. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA --- include/pup/graph/dag.hpp | 26 ++- include/pup/index/entry.hpp | 3 +- include/pup/index/format.hpp | 12 +- src/cli/cmd_build.cpp | 196 ++++++++++++------ src/graph/builder.cpp | 33 +-- src/graph/dag.cpp | 30 ++- src/index/entry.cpp | 6 +- .../duplicate_command/Tupfile.fixture | 4 +- test/unit/test_builder.cpp | 15 +- test/unit/test_e2e.cpp | 31 ++- test/unit/test_graph.cpp | 34 +-- test/unit/test_index.cpp | 35 ++-- 12 files changed, 288 insertions(+), 137 deletions(-) diff --git a/include/pup/graph/dag.hpp b/include/pup/graph/dag.hpp index 7042f4d3..480e6c33 100644 --- a/include/pup/graph/dag.hpp +++ b/include/pup/graph/dag.hpp @@ -390,15 +390,23 @@ auto expand_instruction( [[nodiscard]] auto expand_instruction(Graph const& graph, NodeId cmd_id) -> StringId; -/// Compute a command's structural identity: a content hash that determines whether -/// the command must re-run. It folds the fully-expanded command text together with -/// the values (content hashes) of every Variable node the command depends on via a -/// Sticky edge — capturing config/env vars that affect the output without appearing -/// in the rendered text (e.g. an exported env var the subprocess reads via $VAR). -/// Two builds yield the same identity iff the command's effective definition is the -/// same, so it is the correct cross-build join key (unlike the rendered string). -[[nodiscard]] -auto compute_command_identity(Graph const& graph, NodeId cmd_id, PathCache& cache) -> Hash256; +/// What the command will do, for deciding whether it must re-run: the fully-expanded +/// command text plus the values (content hashes) of every Variable node it depends on +/// via a Sticky edge — capturing config/env vars that affect the output without +/// appearing in the rendered text (e.g. an exported env var read as $VAR). +/// +/// Not a cross-build key. Editing a recipe changes the signature and must not make the +/// rule a different rule; that is what compute_command_key answers. +[[nodiscard]] +auto compute_command_signature(Graph const& graph, NodeId cmd_id, PathCache& cache) -> Hash256; + +/// Which rule this is, for joining a command against the previous build. A command that +/// produces files is keyed by the files themselves — output ownership is unique among +/// guard-satisfied commands, enforced when the output edge is created — so this textual +/// key is the fallback for output-less commands, which have nothing else to be named by. +/// It deliberately excludes variable values: a var change means re-run, not a new rule. +[[nodiscard]] +auto compute_command_key(Graph const& graph, NodeId cmd_id, PathCache& cache) -> Hash256; /// Set the build root name (relative path from source root to build root) /// For in-tree builds, this should be empty. For variant builds, e.g. "build". diff --git a/include/pup/index/entry.hpp b/include/pup/index/entry.hpp index e7b3d720..806a588b 100644 --- a/include/pup/index/entry.hpp +++ b/include/pup/index/entry.hpp @@ -54,7 +54,8 @@ struct CommandEntry { StringId display = StringId::Empty; ///< Display text (from ^ ^ markers) StringId env = StringId::Empty; ///< Environment variables - Hash256 identity = {}; ///< Structural identity: command text + values of vars it depends on + 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 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 1fa0665a..cea9614b 100644 --- a/include/pup/index/format.hpp +++ b/include/pup/index/format.hpp @@ -54,7 +54,11 @@ inline constexpr auto INDEX_MAGIC = std::array { 'P', 'U', 'P', 'I' }; /// compiles of one source with equal flags render byte-identical scans, so /// v17 gave them one identity and the join between them was arbitrary /// (issue #170); every dep-scan command's identity changes. -inline constexpr auto INDEX_VERSION = std::uint32_t { 18 }; +/// 19 - The single `identity` splits into `key` (which rule this is) and +/// `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 }; /// Index file header (56 bytes) - v9 struct alignas(8) RawHeader { @@ -102,11 +106,11 @@ 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) - Hash256 identity = {}; ///< Structural identity (v11): SHA-256 over command text - ///< plus the values of vars it depends on (sticky/exported) + 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) == 48, "RawCommandEntry must be 48 bytes"); +static_assert(sizeof(RawCommandEntry) == 80, "RawCommandEntry must be 80 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 2229666f..8b9b0583 100644 --- a/src/cli/cmd_build.cpp +++ b/src/cli/cmd_build.cpp @@ -734,7 +734,8 @@ auto serialize_command_nodes( .instruction_pattern = pup::graph::get(g, id), .display = pup::graph::get(g, id), .env = pup::StringId::Empty, - .identity = pup::graph::compute_command_identity(g, id, state.path_cache), + .key = pup::graph::compute_command_key(g, id, state.path_cache), + .signature = pup::graph::compute_command_signature(g, id, state.path_cache), .inputs = std::move(inputs), .outputs = std::move(outputs), }; @@ -853,7 +854,7 @@ auto preserve_old_implicit_edges( auto identity_to_new_id = pup::Vec> {}; identity_to_new_id.reserve(ctx.index.commands().size()); for (auto const& cmd : ctx.index.commands()) { - identity_to_new_id.emplace_back(cmd.identity, cmd.id); + identity_to_new_id.emplace_back(cmd.key, cmd.id); } std::sort(identity_to_new_id.begin(), identity_to_new_id.end(), [&](auto const& a, auto const& b) { return pup::hash_less(a.first, b.first); }); @@ -869,8 +870,8 @@ auto preserve_old_implicit_edges( if (!old_cmd) { continue; } - auto match = std::lower_bound(identity_to_new_id.begin(), identity_to_new_id.end(), old_cmd->identity, [&](auto const& p, pup::Hash256 const& key) { return pup::hash_less(p.first, key); }); - if (match == identity_to_new_id.end() || match->first != old_cmd->identity) { + auto match = std::lower_bound(identity_to_new_id.begin(), identity_to_new_id.end(), old_cmd->key, [&](auto const& p, pup::Hash256 const& key) { return pup::hash_less(p.first, key); }); + if (match == identity_to_new_id.end() || match->first != old_cmd->key) { continue; } auto new_to_id = match->second; @@ -902,34 +903,71 @@ auto preserve_old_implicit_edges( } /// Sorted (identity → graph NodeId) map: the cross-build join key for commands. -using IdentityMap = Vec>; +/// Recognises a command from the previous build in the current graph. A command that +/// produces files is found by the files: output ownership is unique among guard-satisfied +/// commands, enforced where the output edge is created, so a produced path names its +/// producer and survives any edit to the recipe. Output-less commands have nothing else +/// to be named by and fall back to their textual key. +/// +/// Guard-satisfied only, matching the set the index records. +struct CommandJoin { + Vec> by_output; ///< produced path -> producer + Vec> by_key; +}; -/// Guard-satisfied only: that is the set the index records, and the set finalize_graph -/// enforces injectivity over. Widening it here would readmit the duplicate keys that -/// enforcement exists to rule out. -auto build_identity_map(pup::graph::BuildGraph const& state) -> IdentityMap +auto build_command_join(pup::graph::BuildGraph const& state) -> CommandJoin { - auto map = IdentityMap {}; - for (auto id : pup::graph::all_nodes(state.graph)) { - if (pup::node_id::is_command(id) && pup::graph::is_guard_satisfied(state.graph, id)) { - map.emplace_back( - pup::graph::compute_command_identity(state.graph, id, state.path_cache), id - ); + auto const& g = state.graph; + auto join = CommandJoin {}; + + for (auto id : pup::graph::all_nodes(g)) { + if (!pup::node_id::is_command(id) || !pup::graph::is_guard_satisfied(g, id)) { + continue; + } + auto produced_any = false; + for (auto output_id : pup::graph::get_outputs(g, id)) { + auto path_sv = pup::graph::get_full_path(g, output_id, state.path_cache); + if (!path_sv.empty()) { + join.by_output.emplace_back(pup::global_pool().intern(path_sv), id); + produced_any = true; + } + } + if (!produced_any) { + join.by_key.emplace_back(pup::graph::compute_command_key(g, id, state.path_cache), id); } } - std::sort(map.begin(), map.end(), [](auto const& a, auto const& b) { return pup::hash_less(a.first, b.first); }); - return map; + + std::sort(join.by_output.begin(), join.by_output.end(), [](auto const& a, auto const& b) { return pup::handle_less(a.first, b.first); }); + std::sort(join.by_key.begin(), join.by_key.end(), [](auto const& a, auto const& b) { return pup::hash_less(a.first, b.first); }); + return join; } -auto find_by_identity(IdentityMap const& map, pup::Hash256 const& identity) -> std::optional +auto find_joined_command( + CommandJoin const& join, + pup::index::Index const& idx, + pup::index::CommandEntry const& cmd +) -> std::optional { + for (auto const* edge : idx.edges_from(cmd.id)) { + auto const* file = idx.find_file_by_id(edge->to); + if (!file || file->type != pup::NodeType::Generated) { + continue; + } + auto const* it = std::lower_bound( + join.by_output.begin(), join.by_output.end(), file->path, [](auto const& p, StringId k) { return pup::handle_less(p.first, k); } + ); + if (it != join.by_output.end() && it->first == file->path) { + return it->second; + } + } + auto const* it = std::lower_bound( - map.begin(), map.end(), identity, [](auto const& p, auto const& k) { return pup::hash_less(p.first, k); } + join.by_key.begin(), join.by_key.end(), cmd.key, [](auto const& p, auto const& k) { return pup::hash_less(p.first, k); } ); - if (it == map.end() || !pup::hash_equal(it->first, identity)) { - return std::nullopt; + if (it != join.by_key.end() && pup::hash_equal(it->first, cmd.key)) { + return it->second; } - return it->second; + return std::nullopt; } auto contains_id(pup::Vec const& v, pup::StringId id) -> bool @@ -993,7 +1031,7 @@ auto merge_out_of_scope_commands( auto new_identities = pup::Vec {}; new_identities.reserve(ctx.index.commands().size()); for (auto const& cmd : ctx.index.commands()) { - new_identities.push_back(cmd.identity); + new_identities.push_back(cmd.key); } std::sort(new_identities.begin(), new_identities.end(), pup::hash_less); @@ -1091,7 +1129,7 @@ auto merge_out_of_scope_commands( if (is_dir_authoritative(old_index, cmd.dir_id, parse_scopes, excludes, parsed_dirs, available_dirs, pruned_dirs)) { continue; } - if (std::binary_search(new_identities.begin(), new_identities.end(), cmd.identity, pup::hash_less)) { + if (std::binary_search(new_identities.begin(), new_identities.end(), cmd.key, pup::hash_less)) { continue; } if (any_dep_changed(cmd)) { @@ -1131,7 +1169,8 @@ auto merge_out_of_scope_commands( .instruction_pattern = cmd.instruction_pattern, .display = cmd.display, .env = cmd.env, - .identity = cmd.identity, + .key = cmd.key, + .signature = cmd.signature, .inputs = std::move(new_inputs), .outputs = std::move(new_outputs), }); @@ -1173,7 +1212,7 @@ auto expand_implicit_deps( pup::Vec const& changed, pup::index::Index const& index, pup::graph::BuildGraph const& state, - IdentityMap const& identity_map + CommandJoin const& join ) -> pup::Vec { auto result = pup::Vec { changed }; @@ -1222,7 +1261,7 @@ auto expand_implicit_deps( continue; } - auto cmd_node_id = find_by_identity(identity_map, cmd->identity); + auto cmd_node_id = find_joined_command(join, index, *cmd); if (!cmd_node_id) { continue; } @@ -1335,9 +1374,13 @@ struct NewCommands { pup::Vec forced_cmds; }; -/// Detect new commands (in graph but not index). Their outputs join the changed-file -/// set; a command that contributes no output paths cannot be reached through that -/// currency, so it is returned for direct scheduling instead. +/// Commands that must run because of what they are rather than because a file changed: +/// no counterpart in the previous build, or a counterpart whose signature differs. Their +/// outputs join the changed-file set; a command that contributes no output paths cannot +/// be reached through that currency, so it is returned for direct scheduling instead. +/// +/// The join runs graph -> index, the mirror of find_joined_command: a produced path names +/// its producer, and output-less commands fall back to the textual key. auto detect_new_commands( pup::graph::BuildGraph const& state, pup::index::Index const& idx, @@ -1348,16 +1391,23 @@ auto detect_new_commands( auto const& g = state.graph; auto result = NewCommands {}; - // Identity-keyed membership: a command must (re)build when its structural identity - // is absent from the previous index. Identity folds command text together with the - // values of the vars it depends on, so a change invisible to the rendered string - // (e.g. an exported env var the subprocess reads via $VAR) still flips the identity. - auto old_identities = pup::Vec {}; - old_identities.reserve(idx.commands().size()); + auto old_by_output = pup::Vec> {}; + auto old_by_key = pup::Vec> {}; for (auto const& cmd : idx.commands()) { - old_identities.push_back(cmd.identity); + auto produced_any = false; + for (auto const* edge : idx.edges_from(cmd.id)) { + auto const* file = idx.find_file_by_id(edge->to); + if (file && file->type == pup::NodeType::Generated && !pup::is_empty(file->path)) { + old_by_output.emplace_back(file->path, &cmd); + produced_any = true; + } + } + if (!produced_any) { + old_by_key.emplace_back(cmd.key, &cmd); + } } - std::sort(old_identities.begin(), old_identities.end(), pup::hash_less); + std::sort(old_by_output.begin(), old_by_output.end(), [](auto const& a, auto const& b) { return pup::handle_less(a.first, b.first); }); + std::sort(old_by_key.begin(), old_by_key.end(), [](auto const& a, auto const& b) { return pup::hash_less(a.first, b.first); }); for (auto id : pup::graph::all_nodes(g)) { if (!pup::node_id::is_command(id)) { @@ -1366,24 +1416,50 @@ auto detect_new_commands( if (!pup::graph::is_guard_satisfied(g, id)) { continue; } - auto identity = pup::graph::compute_command_identity(g, id, state.path_cache); - if (!std::binary_search(old_identities.begin(), old_identities.end(), identity, pup::hash_less)) { - auto pushed_outputs = false; - for (auto output_id : pup::graph::get_outputs(g, id)) { - auto output_path_sv = pup::graph::get_full_path(g, output_id, state.path_cache); - if (!output_path_sv.empty()) { - result.changed_outputs.push_back(pup::global_pool().intern(output_path_sv)); - pushed_outputs = true; - } + + auto output_paths = pup::Vec {}; + for (auto output_id : pup::graph::get_outputs(g, id)) { + auto output_path_sv = pup::graph::get_full_path(g, output_id, state.path_cache); + if (!output_path_sv.empty()) { + output_paths.push_back(pup::global_pool().intern(output_path_sv)); } - if (!pushed_outputs) { - result.forced_cmds.push_back(id); + } + + pup::index::CommandEntry const* previous = nullptr; + for (auto path_id : output_paths) { + auto const* it = std::lower_bound( + old_by_output.begin(), old_by_output.end(), path_id, [](auto const& p, StringId k) { return pup::handle_less(p.first, k); } + ); + if (it != old_by_output.end() && it->first == path_id) { + previous = it->second; + break; } - if (verbose) { - auto display_sv = pup::global_pool().get(pup::graph::get(g, id)); - vprint(variant_name, " New command: {}\n", display_sv); + } + if (!previous && output_paths.empty()) { + auto key = pup::graph::compute_command_key(g, id, state.path_cache); + auto const* it = std::lower_bound( + old_by_key.begin(), old_by_key.end(), key, [](auto const& p, auto const& k) { return pup::hash_less(p.first, k); } + ); + if (it != old_by_key.end() && pup::hash_equal(it->first, key)) { + previous = it->second; } } + + auto signature = pup::graph::compute_command_signature(g, id, state.path_cache); + if (previous && pup::hash_equal(previous->signature, signature)) { + continue; + } + + for (auto path_id : output_paths) { + result.changed_outputs.push_back(path_id); + } + if (output_paths.empty()) { + result.forced_cmds.push_back(id); + } + 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); + } } return result; } @@ -1402,7 +1478,7 @@ auto reconcile_input_set( pup::graph::BuildGraph const& state, pup::index::Index const& idx, pup::Vec const& changed, - IdentityMap const& identity_map, + CommandJoin const& join, std::string_view variant_name, bool verbose ) -> InputSetDelta @@ -1476,7 +1552,7 @@ auto reconcile_input_set( if (!cmd) { continue; } - if (auto cmd_node_id = find_by_identity(identity_map, cmd->identity)) { + if (auto cmd_node_id = find_joined_command(join, idx, *cmd)) { orphaned.push_back(*cmd_node_id); if (verbose) { vprint( @@ -1511,7 +1587,7 @@ auto reconcile_input_set( /// Remove stale outputs from removed commands and report them. auto remove_stale_outputs( pup::index::Index const& idx, - IdentityMap const& identity_map, + CommandJoin const& join, pup::Vec const& parse_scopes, pup::parser::IgnoreList const& excludes, pup::Vec const& parsed_dirs, @@ -1530,7 +1606,7 @@ auto remove_stale_outputs( continue; } - if (find_by_identity(identity_map, cmd.identity)) { + if (find_joined_command(join, idx, cmd)) { continue; } @@ -1698,7 +1774,7 @@ auto build_single_variant( // Build the identity → NodeId map: the cross-build join key for commands. // Must happen after parsing (operands set) but before incremental logic. auto cmd_index_start = pup::SteadyClock::now(); - auto const identity_map = build_identity_map(bs); + auto const join = build_command_join(bs); auto cmd_index_elapsed = pup::SteadyClock::now() - cmd_index_start; pup::thread_metrics().command_index_time = std::chrono::duration_cast(cmd_index_elapsed); @@ -1749,13 +1825,13 @@ auto build_single_variant( auto change_detect_elapsed = pup::SteadyClock::now() - change_detect_start; pup::thread_metrics().change_detection_time = std::chrono::duration_cast(change_detect_elapsed); - auto input_delta = reconcile_input_set(bs, idx, changed_files, identity_map, variant_name, opts.verbose); + auto input_delta = reconcile_input_set(bs, idx, changed_files, join, variant_name, opts.verbose); for (auto path_id : input_delta.changed_paths) { changed_files.push_back(path_id); } auto implicit_deps_start = pup::SteadyClock::now(); - changed_files = expand_implicit_deps(changed_files, idx, bs, identity_map); + changed_files = expand_implicit_deps(changed_files, idx, bs, join); auto implicit_deps_elapsed = pup::SteadyClock::now() - implicit_deps_start; pup::thread_metrics().implicit_deps_time = std::chrono::duration_cast(implicit_deps_elapsed); @@ -1789,7 +1865,7 @@ auto build_single_variant( auto stale_start = pup::SteadyClock::now(); remove_stale_outputs( idx, - identity_map, + join, parse_scopes, excludes, ctx.parsed_dirs(), diff --git a/src/graph/builder.cpp b/src/graph/builder.cpp index aaed4e39..74783bf2 100644 --- a/src/graph/builder.cpp +++ b/src/graph/builder.cpp @@ -2500,18 +2500,21 @@ auto add_tupfile( return {}; } -/// Command identity is the cross-build join key, and every consumer looks a command up by -/// identity alone; two commands sharing one make that lookup answer arbitrarily. Reject the -/// graph here rather than join it wrong later. Only guard-satisfied commands are considered, -/// matching the set the index records: complementary branches of one conditional legitimately -/// render the same text, and at most one of them is ever live. -auto reject_ambiguous_identities(BuildGraph& build_state) -> Result +/// A command that produces files is joined across builds by the files, and output ownership +/// is already unique among guard-satisfied commands. Output-less commands are joined by +/// their textual key instead, and nothing enforced that key's uniqueness: two such rules in +/// one directory are indistinguishable to every later build. Reject the graph here rather +/// than join it wrong later. +/// +/// Guard-satisfied only, matching the set the index records: complementary branches of one +/// conditional legitimately render the same text, and at most one of them is ever live. +auto reject_ambiguous_keys(BuildGraph& build_state) -> Result { auto& g = build_state.graph; - auto identities = Vec> {}; + auto keys = Vec> {}; for (auto id : all_nodes(g)) { - if (!node_id::is_command(id) || !is_guard_satisfied(g, id)) { + if (!node_id::is_command(id) || !is_guard_satisfied(g, id) || !get_outputs(g, id).empty()) { continue; } // The configure pass evaluates before tup.config exists, so every rule gated on an @@ -2521,24 +2524,24 @@ auto reject_ambiguous_identities(BuildGraph& build_state) -> Result if (str(expand_instruction(g, id, build_state.path_cache)).find_first_not_of(" \t") == std::string_view::npos) { continue; } - identities.emplace_back(compute_command_identity(g, id, build_state.path_cache), id); + keys.emplace_back(compute_command_key(g, id, build_state.path_cache), id); } - std::sort(identities.begin(), identities.end(), [](auto const& a, auto const& b) { + std::sort(keys.begin(), keys.end(), [](auto const& a, auto const& b) { return hash_less(a.first, b.first); }); - auto const* dup = std::adjacent_find(identities.begin(), identities.end(), [](auto const& a, auto const& b) { + auto const* dup = std::adjacent_find(keys.begin(), keys.end(), [](auto const& a, auto const& b) { return hash_equal(a.first, b.first); }); - if (dup == identities.end()) { + if (dup == keys.end()) { return {}; } auto dir_sv = str(get(g, dup->second)); auto err = Buf {}; err.fmt( - "Duplicate command in '{}': two rules render the same command line, so no build can " - "tell them apart:\n {}", + "Duplicate command in '{}': two rules produce no output and render the same command " + "line, so no build can tell them apart:\n {}", dir_sv.empty() ? "." : dir_sv, str(expand_instruction(g, dup->second, build_state.path_cache)) ); @@ -2675,7 +2678,7 @@ auto finalize_graph( state.deferred_edges.clear(); // Last: pass 2 rewrites command text, so identity is not final before it. - return reject_ambiguous_identities(build_state); + return reject_ambiguous_keys(build_state); } } // namespace pup::graph diff --git a/src/graph/dag.cpp b/src/graph/dag.cpp index 402f935d..dd6bbae6 100644 --- a/src/graph/dag.cpp +++ b/src/graph/dag.cpp @@ -1031,29 +1031,43 @@ auto expand_instruction(Graph const& graph, NodeId cmd_id) -> StringId return expand_instruction(graph, cmd_id, cache); } -auto compute_command_identity(Graph const& graph, NodeId cmd_id, PathCache& cache) -> Hash256 +auto compute_command_key(Graph const& graph, NodeId cmd_id, PathCache& cache) -> Hash256 { 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. + // identically; without the directory those distinct rules share one key. state = sha256_update(state, std::span { &SEP, 1 }); state = sha256_update(state, pool.get(get(graph, cmd_id))); - // A dep-scan command drops its parent's -o, so two compiles of one source with equal - // flags render byte-identical scans that differ only in whose deps they inject. - // Recursion is one level: a parent is a rule-authored command and has no parent. + // A dep-scan command is output-less and drops its parent's -o, so two compiles of one + // source with equal flags render byte-identical scans; whose deps they inject is the + // only thing that tells them apart. One level: a parent is rule-authored, so has none. if (auto parent = get_parent_command(graph, cmd_id); parent != INVALID_NODE_ID) { - auto parent_identity = compute_command_identity(graph, parent, cache); + auto parent_key = compute_command_key(graph, parent, cache); state = sha256_update(state, std::span { &SEP, 1 }); - state = sha256_update(state, std::span { parent_identity.data(), parent_identity.size() }); + state = sha256_update(state, std::span { parent_key.data(), parent_key.size() }); } + return sha256_finalize(state); +} + +auto compute_command_signature(Graph const& graph, NodeId cmd_id, PathCache& cache) -> Hash256 +{ + 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))); + + 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. diff --git a/src/index/entry.cpp b/src/index/entry.cpp index 559c09a8..77ec8396 100644 --- a/src/index/entry.cpp +++ b/src/index/entry.cpp @@ -66,7 +66,8 @@ auto CommandEntry::to_raw( raw.cmd_offset = instruction_offset; raw.display_offset = display_offset; raw.env_offset = env_offset; - raw.identity = identity; + raw.key = key; + raw.signature = signature; return raw; } @@ -86,7 +87,8 @@ auto CommandEntry::from_raw( .instruction_pattern = global_pool().intern(instruction_pattern), .display = global_pool().intern(display_str), .env = global_pool().intern(env_str), - .identity = raw.identity, + .key = raw.key, + .signature = raw.signature, .inputs = std::move(inputs), .outputs = std::move(outputs), }; diff --git a/test/e2e/fixtures/duplicate_command/Tupfile.fixture b/test/e2e/fixtures/duplicate_command/Tupfile.fixture index 28e2e628..0a8e649f 100644 --- a/test/e2e/fixtures/duplicate_command/Tupfile.fixture +++ b/test/e2e/fixtures/duplicate_command/Tupfile.fixture @@ -1,2 +1,2 @@ -: |> ./gen |> a.out -: |> ./gen |> b.out +: |> ./check |> +: |> ./check |> diff --git a/test/unit/test_builder.cpp b/test/unit/test_builder.cpp index 3e96f111..33afdcbc 100644 --- a/test/unit/test_builder.cpp +++ b/test/unit/test_builder.cpp @@ -1925,14 +1925,23 @@ auto finalize_tupfile( } // namespace -TEST_CASE("GraphBuilder rejects two commands that share one identity", "[builder][identity]") +TEST_CASE("GraphBuilder rejects two output-less commands that share one key", "[builder][identity]") { auto fixture = BuilderTestFixture {}; - auto result = finalize_tupfile(fixture, ": |> ./gen |> a.out\n: |> ./gen |> b.out\n"); + // No outputs to be named by, and identical text: no later build can tell them apart. + auto result = finalize_tupfile(fixture, ": |> ./check |>\n: |> ./check |>\n"); REQUIRE_FALSE(result.has_value()); - CHECK(sv(result.error().message).find("./gen") != std::string_view::npos); + CHECK(sv(result.error().message).find("./check") != std::string_view::npos); +} + +TEST_CASE("GraphBuilder accepts identical text when the outputs differ", "[builder][identity]") +{ + auto fixture = BuilderTestFixture {}; + + // Same recipe, but each is named by what it produces, so the join stays unambiguous. + CHECK(finalize_tupfile(fixture, ": |> ./gen |> a.out\n: |> ./gen |> b.out\n").has_value()); } TEST_CASE("GraphBuilder accepts commands whose rendered text differs", "[builder][identity]") diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index b201b42b..0154cfd7 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -4475,7 +4475,7 @@ SCENARIO("Duplicate output detection", "[e2e][duplicate]") SCENARIO("Duplicate command detection", "[e2e][duplicate][identity]") { - GIVEN("a Tupfile with two rules that render the same command line") + GIVEN("a Tupfile with two output-less rules that render the same command line") { auto f = E2EFixture { "duplicate_command" }; @@ -4489,7 +4489,34 @@ SCENARIO("Duplicate command detection", "[e2e][duplicate][identity]") INFO("stderr: " << result.stderr_output); REQUIRE_FALSE(result.success()); REQUIRE(result.stderr_output.find("Duplicate command") != std::string::npos); - REQUIRE(result.stderr_output.find("./gen") != std::string::npos); + REQUIRE(result.stderr_output.find("./check") != std::string::npos); + } + } + } +} + +SCENARIO("Editing a rule's recipe does not make it a different rule", "[e2e][identity][join]") +{ + GIVEN("a built project whose rule produces one output") + { + auto f = E2EFixture { "phi_same_output" }; + f.write_file("Tupfile", ": input.txt |> cp %f %o |> output.txt\n"); + f.write_file("input.txt", "test\n"); + f.mkdir("build"); + REQUIRE(f.pup({ "configure", "-B", "build" }).success()); + REQUIRE(f.build({ "-B", "build" }).success()); + + WHEN("only the command text changes, with the same inputs and outputs") + { + f.write_file("Tupfile", ": input.txt |> cat %f > %o |> output.txt\n"); + auto result = f.build({ "-B", "build", "-v" }); + + THEN("the rule is recognised as the same one and its output is not deleted") + { + INFO("stdout: " << result.stdout_output); + REQUIRE(result.success()); + REQUIRE(result.stdout_output.find("Removed stale") == std::string::npos); + REQUIRE(f.exists("build/output.txt")); } } } diff --git a/test/unit/test_graph.cpp b/test/unit/test_graph.cpp index b7ba5e7d..c2b6dc07 100644 --- a/test/unit/test_graph.cpp +++ b/test/unit/test_graph.cpp @@ -1637,7 +1637,7 @@ 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]") +TEST_CASE("compute_command_key separates rules by directory", "[graph][identity]") { auto bs = make_build_graph(); auto& g = bs.graph; @@ -1655,31 +1655,31 @@ TEST_CASE("compute_command_identity separates rules by directory", "[graph][iden REQUIRE(in_a.has_value()); REQUIRE(in_b.has_value()); - SECTION("same text in different directories yields different identities") + SECTION("same text in different directories yields different keys") { - CHECK(compute_command_identity(g, *in_a, bs.path_cache) - != compute_command_identity(g, *in_b, bs.path_cache)); + CHECK(compute_command_key(g, *in_a, bs.path_cache) + != compute_command_key(g, *in_b, bs.path_cache)); } - SECTION("identity is stable for the same command") + SECTION("the key 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)); + CHECK(compute_command_key(g, *in_a, bs.path_cache) + == compute_command_key(g, *in_a, bs.path_cache)); } - SECTION("different text in the same directory yields different identities") + SECTION("different text in the same directory yields different keys") { 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)); + CHECK(compute_command_key(g, *in_a, bs.path_cache) + != compute_command_key(g, *other, bs.path_cache)); } } -TEST_CASE("compute_command_identity separates dep-scan commands by parent", "[graph][identity]") +TEST_CASE("compute_command_key separates dep-scan commands by parent", "[graph][identity]") { auto bs = make_build_graph(); auto& g = bs.graph; @@ -1712,15 +1712,15 @@ TEST_CASE("compute_command_identity separates dep-scan commands by parent", "[gr REQUIRE(scan_a.has_value()); REQUIRE(scan_b.has_value()); - SECTION("identical scan text under different parents yields different identities") + SECTION("identical scan text under different parents yields different keys") { - CHECK(compute_command_identity(g, *scan_a, bs.path_cache) - != compute_command_identity(g, *scan_b, bs.path_cache)); + CHECK(compute_command_key(g, *scan_a, bs.path_cache) + != compute_command_key(g, *scan_b, bs.path_cache)); } - SECTION("a scan's identity is stable") + SECTION("a scan's key is stable") { - CHECK(compute_command_identity(g, *scan_a, bs.path_cache) - == compute_command_identity(g, *scan_a, bs.path_cache)); + CHECK(compute_command_key(g, *scan_a, bs.path_cache) + == compute_command_key(g, *scan_a, bs.path_cache)); } } diff --git a/test/unit/test_index.cpp b/test/unit/test_index.cpp index 5356c8e4..b9861250 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 48 bytes (v11: + identity hash)") + SECTION("RawCommandEntry is 80 bytes (v19: + key and signature hashes)") { - REQUIRE(sizeof(RawCommandEntry) == 48); + REQUIRE(sizeof(RawCommandEntry) == 80); } SECTION("RawEdge is 16 bytes") @@ -162,9 +162,13 @@ TEST_CASE("FileEntry conversion", "[index]") TEST_CASE("CommandEntry conversion", "[index]") { - auto identity = pup::Hash256 {}; - identity[0] = std::byte { 0xAB }; - identity[31] = std::byte { 0xCD }; + // Distinct values: a roundtrip that swapped the two fields must fail. + auto key = pup::Hash256 {}; + key[0] = std::byte { 0xAB }; + key[31] = std::byte { 0xCD }; + auto signature = pup::Hash256 {}; + signature[0] = std::byte { 0x12 }; + signature[31] = std::byte { 0x34 }; auto cmd = CommandEntry { .id = node_id::make_command(5), @@ -172,7 +176,8 @@ TEST_CASE("CommandEntry conversion", "[index]") .instruction_pattern = intern("gcc -c %f -o %o"), .display = intern("CC main.c"), .env = intern("CC=gcc"), - .identity = identity, + .key = key, + .signature = signature, .inputs = { 10 }, .outputs = { 20 }, }; @@ -183,7 +188,8 @@ TEST_CASE("CommandEntry conversion", "[index]") REQUIRE(raw.cmd_offset == 0); REQUIRE(raw.display_offset == 50); REQUIRE(raw.env_offset == 100); - REQUIRE(raw.identity == identity); + REQUIRE(raw.key == key); + REQUIRE(raw.signature == signature); // ID is computed from array index (4 + 1 = 5, then node_id::make_command) auto& pool = global_pool(); @@ -197,7 +203,8 @@ TEST_CASE("CommandEntry conversion", "[index]") REQUIRE(restored.instruction_pattern == cmd.instruction_pattern); REQUIRE(restored.display == cmd.display); REQUIRE(restored.env == cmd.env); - REQUIRE(restored.identity == identity); + REQUIRE(restored.key == key); + REQUIRE(restored.signature == signature); REQUIRE(restored.inputs == cmd.inputs); REQUIRE(restored.outputs == cmd.outputs); } @@ -440,17 +447,17 @@ TEST_CASE("Index serialization roundtrip", "[e2e][index]") .size = 8192, }); - // Command 1 (v8: template + operands; v11: + identity hash) - auto cmd_identity = pup::Hash256 {}; - cmd_identity[0] = std::byte { 0x11 }; - cmd_identity[31] = std::byte { 0x99 }; + // Command 1 (v8: template + operands; v19: + key and signature hashes) + auto cmd_key = pup::Hash256 {}; + cmd_key[0] = std::byte { 0x11 }; + cmd_key[31] = std::byte { 0x99 }; index.add_command(CommandEntry { .id = cmd_id, .dir_id = 0, .instruction_pattern = intern("g++ -c %f -o %o"), .display = intern("CXX main.cpp"), .env = {}, - .identity = cmd_identity, + .key = cmd_key, .inputs = { 3 }, // main.cpp .outputs = { 4 }, // main.o }); @@ -518,7 +525,7 @@ TEST_CASE("Index serialization roundtrip", "[e2e][index]") REQUIRE(cmd != nullptr); REQUIRE(cmd->instruction_pattern == intern("g++ -c %f -o %o")); REQUIRE(cmd->display == intern("CXX main.cpp")); - REQUIRE(cmd->identity == cmd_identity); + REQUIRE(cmd->key == cmd_key); REQUIRE(cmd->inputs == pup::Vec { 3 }); REQUIRE(cmd->outputs == pup::Vec { 4 }); From 17bf5a2a287e5be77d2fc232b1f430963669bcf5 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 27 Jul 2026 21:05:52 +0800 Subject: [PATCH 4/4] Make stale removal file-granular; skip key checking in the configure pass Two blockers from an adversarial review of the key/signature split, both reproduced against a main-built binary before fixing. Stale-output removal asked "did this command survive" and skipped the whole entry when it had. That was sound while the join was strict -- any edit changed the identity and the command read as removed -- but the output-based join is deliberately forgiving, so a rule that drops one of its outputs now joins through the ones it kept and the dropped file was never deleted. It is also absent from the new index, so its ownership record is gone and no later build can find it: : in.txt |> sh -c "cp in.txt a.out && cp in.txt b.out" |> a.out b.out : in.txt |> sh -c "cp in.txt a.out" |> a.out # b.out leaked forever Staleness is a property of a file, not of a command: an output whose path no longer has a live producer is stale, whoever used to own it. That is what the output-based join makes directly checkable, and remove_stale_outputs now says so. "Removed command" stays command-granular since it is only a message. The ambiguity check ran during the configure pass, where rendered text does not yet distinguish anything because tup.config does not exist. Two output-less rules differing only in config vars both render "./run " and the check rejected them -- failing the very pass whose job is to create the file that tells them apart, with no workaround. The exemption was for fully-empty renders, which was too narrow; the check now sits out the configure pass entirely, keyed off the flag that already marks it. Two smaller findings from the same review, fixed together because they are one cause: the join rule existed in three ad-hoc copies (graph->index, index->graph, index->index) and they had drifted apart. find_joined_command let an output-ful command fall through to the textual key while detect_new_commands did not, so the two directions disagreed about whether a command survived; and preserve_old_implicit_edges and merge_out_of_scope_commands still matched on bare key, an invariant only enforced for output-less commands. All three now go through one CommandLookup/CommandAddress pair, so "the same command" has a single definition and the exclusivity of the two addressing modes is stated once. Verified: dropping one of two outputs removes exactly that file; configure completes on the config-distinguished pair; GCC BSP parses (24 Tupfiles, 1853 commands); busybox parses (31 Tupfiles, 293 commands). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA --- src/cli/cmd_build.cpp | 255 ++++++++++++++++++++++--------------- src/graph/builder.cpp | 14 +- test/unit/test_builder.cpp | 19 ++- test/unit/test_e2e.cpp | 29 +++++ 4 files changed, 205 insertions(+), 112 deletions(-) diff --git a/src/cli/cmd_build.cpp b/src/cli/cmd_build.cpp index 8b9b0583..0836e29f 100644 --- a/src/cli/cmd_build.cpp +++ b/src/cli/cmd_build.cpp @@ -832,6 +832,136 @@ auto process_implicit_deps( } } +/// How a set of commands is addressed for joining, in one place so that every join -- +/// graph to index, index to graph, index to index -- agrees on what "the same command" +/// means. A command that produces files is addressed by the files: output ownership is +/// unique among guard-satisfied commands, enforced where the output edge is created, so a +/// produced path names its producer and survives any edit to the recipe. A command that +/// produces nothing has no such name and is addressed by its textual key instead. +/// +/// The two are exclusive. A command that produced files is never looked up by key, or the +/// two directions of a join would disagree about whether two commands correspond. +struct CommandLookup { + Vec> by_output; ///< produced path -> producer + Vec> by_key; +}; + +/// What one command answers to. Empty outputs is what selects the textual key. +struct CommandAddress { + Vec outputs; + pup::Hash256 key = {}; +}; + +auto sort_lookup(CommandLookup& lookup) -> void +{ + std::sort(lookup.by_output.begin(), lookup.by_output.end(), [](auto const& a, auto const& b) { return pup::handle_less(a.first, b.first); }); + std::sort(lookup.by_key.begin(), lookup.by_key.end(), [](auto const& a, auto const& b) { return pup::hash_less(a.first, b.first); }); +} + +auto graph_command_address(pup::graph::BuildGraph const& state, pup::NodeId id) -> CommandAddress +{ + auto address = CommandAddress {}; + for (auto output_id : pup::graph::get_outputs(state.graph, id)) { + auto path_sv = pup::graph::get_full_path(state.graph, output_id, state.path_cache); + if (!path_sv.empty()) { + address.outputs.push_back(pup::global_pool().intern(path_sv)); + } + } + if (address.outputs.empty()) { + address.key = pup::graph::compute_command_key(state.graph, id, state.path_cache); + } + return address; +} + +auto index_command_address(pup::index::Index const& idx, pup::index::CommandEntry const& cmd) -> CommandAddress +{ + auto address = CommandAddress { .outputs = {}, .key = cmd.key }; + for (auto const* edge : idx.edges_from(cmd.id)) { + auto const* file = idx.find_file_by_id(edge->to); + if (file && file->type == pup::NodeType::Generated && !pup::is_empty(file->path)) { + address.outputs.push_back(file->path); + } + } + return address; +} + +/// Guard-satisfied only, matching the set the index records. +auto graph_command_lookup(pup::graph::BuildGraph const& state) -> CommandLookup +{ + auto lookup = CommandLookup {}; + for (auto id : pup::graph::all_nodes(state.graph)) { + if (!pup::node_id::is_command(id) || !pup::graph::is_guard_satisfied(state.graph, id)) { + continue; + } + auto address = graph_command_address(state, id); + for (auto path : address.outputs) { + lookup.by_output.emplace_back(path, id); + } + if (address.outputs.empty()) { + lookup.by_key.emplace_back(address.key, id); + } + } + sort_lookup(lookup); + return lookup; +} + +auto index_command_lookup(pup::index::Index const& idx) -> CommandLookup +{ + auto lookup = CommandLookup {}; + for (auto const& cmd : idx.commands()) { + auto address = index_command_address(idx, cmd); + for (auto path : address.outputs) { + lookup.by_output.emplace_back(path, cmd.id); + } + if (address.outputs.empty()) { + lookup.by_key.emplace_back(address.key, cmd.id); + } + } + sort_lookup(lookup); + return lookup; +} + +auto find_joined(CommandLookup const& lookup, CommandAddress const& address) -> std::optional +{ + for (auto path : address.outputs) { + auto const* it = std::lower_bound( + lookup.by_output.begin(), lookup.by_output.end(), path, [](auto const& p, StringId k) { return pup::handle_less(p.first, k); } + ); + if (it != lookup.by_output.end() && it->first == path) { + return it->second; + } + } + if (!address.outputs.empty()) { + return std::nullopt; + } + + auto const* it = std::lower_bound( + lookup.by_key.begin(), lookup.by_key.end(), address.key, [](auto const& p, auto const& k) { return pup::hash_less(p.first, k); } + ); + if (it != lookup.by_key.end() && pup::hash_equal(it->first, address.key)) { + return it->second; + } + return std::nullopt; +} + +/// Whether any command in the lookup still produces this path. +auto has_live_producer(CommandLookup const& lookup, StringId path) -> bool +{ + auto const* it = std::lower_bound( + lookup.by_output.begin(), lookup.by_output.end(), path, [](auto const& p, StringId k) { return pup::handle_less(p.first, k); } + ); + return it != lookup.by_output.end() && it->first == path; +} + +auto find_joined_command( + CommandLookup const& lookup, + pup::index::Index const& idx, + pup::index::CommandEntry const& cmd +) -> std::optional +{ + return find_joined(lookup, index_command_address(idx, cmd)); +} + /// Preserve implicit edges from the old index for commands that weren't rebuilt. auto preserve_old_implicit_edges( pup::index::Index const& old_index, @@ -847,34 +977,28 @@ auto preserve_old_implicit_edges( } } - // Map a command's structural identity -> its id in the new index. Command ids are - // positional and shift across builds (e.g. when an earlier-created command is - // removed), so the old edge's `to` id cannot be trusted to mean the same command. - // Identity is stable, so we re-resolve each carried edge's command through it. - auto identity_to_new_id = pup::Vec> {}; - identity_to_new_id.reserve(ctx.index.commands().size()); - for (auto const& cmd : ctx.index.commands()) { - identity_to_new_id.emplace_back(cmd.key, cmd.id); - } - std::sort(identity_to_new_id.begin(), identity_to_new_id.end(), [&](auto const& a, auto const& b) { return pup::hash_less(a.first, b.first); }); + // Command ids are positional and shift across builds (e.g. when an earlier-created + // command is removed), so the old edge's `to` id cannot be trusted to mean the same + // command; re-resolve each carried edge's command through the same join every other + // consumer uses. + auto const new_lookup = index_command_lookup(ctx.index); for (auto const& edge : old_index.edges()) { if (edge.type != pup::LinkType::Implicit) { continue; } - // Re-resolve the old command to its identity-matched counterpart in the new - // index. If it's gone or its definition changed (no identity match), drop the - // edge: the command either no longer exists or rebuilt and rediscovered its deps. + // If the command is gone, drop the edge. If it survived but re-ran, the branch + // below drops it too: it rediscovered its own deps. auto const* old_cmd = old_index.find_command_by_id(edge.to); if (!old_cmd) { continue; } - auto match = std::lower_bound(identity_to_new_id.begin(), identity_to_new_id.end(), old_cmd->key, [&](auto const& p, pup::Hash256 const& key) { return pup::hash_less(p.first, key); }); - if (match == identity_to_new_id.end() || match->first != old_cmd->key) { + auto joined = find_joined(new_lookup, index_command_address(old_index, *old_cmd)); + if (!joined) { continue; } - auto new_to_id = match->second; + auto new_to_id = *joined; if (commands_with_new_deps.contains(new_to_id)) { continue; @@ -902,74 +1026,6 @@ auto preserve_old_implicit_edges( } } -/// Sorted (identity → graph NodeId) map: the cross-build join key for commands. -/// Recognises a command from the previous build in the current graph. A command that -/// produces files is found by the files: output ownership is unique among guard-satisfied -/// commands, enforced where the output edge is created, so a produced path names its -/// producer and survives any edit to the recipe. Output-less commands have nothing else -/// to be named by and fall back to their textual key. -/// -/// Guard-satisfied only, matching the set the index records. -struct CommandJoin { - Vec> by_output; ///< produced path -> producer - Vec> by_key; -}; - -auto build_command_join(pup::graph::BuildGraph const& state) -> CommandJoin -{ - auto const& g = state.graph; - auto join = CommandJoin {}; - - for (auto id : pup::graph::all_nodes(g)) { - if (!pup::node_id::is_command(id) || !pup::graph::is_guard_satisfied(g, id)) { - continue; - } - auto produced_any = false; - for (auto output_id : pup::graph::get_outputs(g, id)) { - auto path_sv = pup::graph::get_full_path(g, output_id, state.path_cache); - if (!path_sv.empty()) { - join.by_output.emplace_back(pup::global_pool().intern(path_sv), id); - produced_any = true; - } - } - if (!produced_any) { - join.by_key.emplace_back(pup::graph::compute_command_key(g, id, state.path_cache), id); - } - } - - std::sort(join.by_output.begin(), join.by_output.end(), [](auto const& a, auto const& b) { return pup::handle_less(a.first, b.first); }); - std::sort(join.by_key.begin(), join.by_key.end(), [](auto const& a, auto const& b) { return pup::hash_less(a.first, b.first); }); - return join; -} - -auto find_joined_command( - CommandJoin const& join, - pup::index::Index const& idx, - pup::index::CommandEntry const& cmd -) -> std::optional -{ - for (auto const* edge : idx.edges_from(cmd.id)) { - auto const* file = idx.find_file_by_id(edge->to); - if (!file || file->type != pup::NodeType::Generated) { - continue; - } - auto const* it = std::lower_bound( - join.by_output.begin(), join.by_output.end(), file->path, [](auto const& p, StringId k) { return pup::handle_less(p.first, k); } - ); - if (it != join.by_output.end() && it->first == file->path) { - return it->second; - } - } - - auto const* it = std::lower_bound( - join.by_key.begin(), join.by_key.end(), cmd.key, [](auto const& p, auto const& k) { return pup::hash_less(p.first, k); } - ); - if (it != join.by_key.end() && pup::hash_equal(it->first, cmd.key)) { - return it->second; - } - return std::nullopt; -} - auto contains_id(pup::Vec const& v, pup::StringId id) -> bool { return !pup::is_empty(id) && std::find(v.begin(), v.end(), id) != v.end(); @@ -1015,7 +1071,7 @@ auto is_dir_authoritative( /// (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. +/// preserve_old_implicit_edges through the same join. auto merge_out_of_scope_commands( pup::index::Index const& old_index, pup::Vec const& parse_scopes, @@ -1028,12 +1084,7 @@ auto merge_out_of_scope_commands( { auto& pool = pup::global_pool(); - auto new_identities = pup::Vec {}; - new_identities.reserve(ctx.index.commands().size()); - for (auto const& cmd : ctx.index.commands()) { - new_identities.push_back(cmd.key); - } - std::sort(new_identities.begin(), new_identities.end(), pup::hash_less); + auto new_lookup = index_command_lookup(ctx.index); // serialize_graph_nodes registers File/Generated/Directory paths but not // Ghosts; without this the merge would duplicate a ghost's entry by path. @@ -1129,7 +1180,7 @@ auto merge_out_of_scope_commands( if (is_dir_authoritative(old_index, cmd.dir_id, parse_scopes, excludes, parsed_dirs, available_dirs, pruned_dirs)) { continue; } - if (std::binary_search(new_identities.begin(), new_identities.end(), cmd.key, pup::hash_less)) { + if (find_joined(new_lookup, index_command_address(old_index, cmd))) { continue; } if (any_dep_changed(cmd)) { @@ -1212,7 +1263,7 @@ auto expand_implicit_deps( pup::Vec const& changed, pup::index::Index const& index, pup::graph::BuildGraph const& state, - CommandJoin const& join + CommandLookup const& join ) -> pup::Vec { auto result = pup::Vec { changed }; @@ -1478,7 +1529,7 @@ auto reconcile_input_set( pup::graph::BuildGraph const& state, pup::index::Index const& idx, pup::Vec const& changed, - CommandJoin const& join, + CommandLookup const& join, std::string_view variant_name, bool verbose ) -> InputSetDelta @@ -1587,7 +1638,7 @@ auto reconcile_input_set( /// Remove stale outputs from removed commands and report them. auto remove_stale_outputs( pup::index::Index const& idx, - CommandJoin const& join, + CommandLookup const& join, pup::Vec const& parse_scopes, pup::parser::IgnoreList const& excludes, pup::Vec const& parsed_dirs, @@ -1606,15 +1657,17 @@ auto remove_stale_outputs( continue; } - if (find_joined_command(join, idx, cmd)) { - continue; - } - + // Staleness is per file, not per command: a rule that drops one of its outputs + // still joins through the ones it kept, so asking only "did this command survive" + // would leave the dropped file owned by nothing and never delete it. for (auto const* edge : idx.edges_from(cmd.id)) { auto const* file = idx.find_file_by_id(edge->to); if (!file || file->type != pup::NodeType::Generated) { continue; } + if (has_live_producer(join, file->path)) { + continue; + } // Paths now include build root (e.g., "build/program") auto file_path_sv = pup::global_pool().get(file->path); @@ -1632,7 +1685,7 @@ auto remove_stale_outputs( } } - if (verbose) { + if (verbose && !find_joined_command(join, idx, cmd)) { vprint(variant_name, " Removed command: {}\n", pup::global_pool().get(cmd.display)); } } @@ -1774,7 +1827,7 @@ auto build_single_variant( // Build the identity → NodeId map: the cross-build join key for commands. // Must happen after parsing (operands set) but before incremental logic. auto cmd_index_start = pup::SteadyClock::now(); - auto const join = build_command_join(bs); + auto const join = graph_command_lookup(bs); auto cmd_index_elapsed = pup::SteadyClock::now() - cmd_index_start; pup::thread_metrics().command_index_time = std::chrono::duration_cast(cmd_index_elapsed); diff --git a/src/graph/builder.cpp b/src/graph/builder.cpp index 74783bf2..bbdfca8f 100644 --- a/src/graph/builder.cpp +++ b/src/graph/builder.cpp @@ -2517,13 +2517,6 @@ auto reject_ambiguous_keys(BuildGraph& build_state) -> Result if (!node_id::is_command(id) || !is_guard_satisfied(g, id) || !get_outputs(g, id).empty()) { continue; } - // The configure pass evaluates before tup.config exists, so every rule gated on an - // unset config var renders empty and collides with the next one. Those commands - // never run: a real build rejects them outright, and configure schedules only the - // config-generating rules. - if (str(expand_instruction(g, id, build_state.path_cache)).find_first_not_of(" \t") == std::string_view::npos) { - continue; - } keys.emplace_back(compute_command_key(g, id, build_state.path_cache), id); } @@ -2677,7 +2670,12 @@ auto finalize_graph( state.deferred_edges.clear(); - // Last: pass 2 rewrites command text, so identity is not final before it. + // The configure pass runs before tup.config exists, so rendered text does not yet + // distinguish anything and a key collision there means nothing; it schedules only the + // config-generating rules anyway. Last, because pass 2 rewrites command text. + if (!state.options.reject_empty_commands) { + return {}; + } return reject_ambiguous_keys(build_state); } diff --git a/test/unit/test_builder.cpp b/test/unit/test_builder.cpp index 33afdcbc..f3a68214 100644 --- a/test/unit/test_builder.cpp +++ b/test/unit/test_builder.cpp @@ -1955,9 +1955,9 @@ TEST_CASE("GraphBuilder accepts empty-rendered duplicates in the configure pass" { auto fixture = BuilderTestFixture {}; - // The configure pass evaluates before tup.config exists, so every rule gated on an - // unset config var renders empty and collides with the next one. Those commands are - // never scheduled -- only the config-generating rules are -- so the collision is inert. + // The configure pass evaluates before tup.config exists, so rendered text does not yet + // distinguish anything. Those commands are never scheduled -- only the config-generating + // rules are -- so no collision among them can matter. auto result = finalize_tupfile( fixture, ": |> @(CC) |> a.out\n: |> @(CC) |> b.out\n", nullptr, false ); @@ -1965,6 +1965,19 @@ TEST_CASE("GraphBuilder accepts empty-rendered duplicates in the configure pass" CHECK(result.has_value()); } +TEST_CASE("GraphBuilder accepts config-distinguished commands in the configure pass", "[builder][identity][configure]") +{ + auto fixture = BuilderTestFixture {}; + + // Distinct once tup.config is read; identical before it exists. Rejecting here would + // fail the very pass that creates the file that tells them apart. + auto result = finalize_tupfile( + fixture, ": |> ./run @(A) |>\n: |> ./run @(B) |>\n", nullptr, false + ); + + CHECK(result.has_value()); +} + TEST_CASE("GraphBuilder accepts identical text under complementary guards", "[builder][identity][phi]") { auto fixture = BuilderTestFixture {}; diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index 0154cfd7..8f33a7ff 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -4522,6 +4522,35 @@ SCENARIO("Editing a rule's recipe does not make it a different rule", "[e2e][ide } } +SCENARIO("Dropping one output of a rule removes that output", "[e2e][identity][join][stale]") +{ + GIVEN("a built rule that declares two outputs") + { + auto f = E2EFixture { "phi_same_output" }; + f.write_file("Tupfile", ": input.txt |> sh -c \"cp input.txt build/a.out && cp input.txt build/b.out\" |> a.out b.out\n"); + f.write_file("input.txt", "test\n"); + f.mkdir("build"); + REQUIRE(f.pup({ "configure", "-B", "build" }).success()); + REQUIRE(f.build({ "-B", "build" }).success()); + REQUIRE(f.exists("build/a.out")); + REQUIRE(f.exists("build/b.out")); + + WHEN("the rule is edited to declare only the first output") + { + f.write_file("Tupfile", ": input.txt |> sh -c \"cp input.txt build/a.out\" |> a.out\n"); + auto result = f.build({ "-B", "build", "-v" }); + + THEN("the output it no longer produces is deleted, and the one it still does survives") + { + INFO("stdout: " << result.stdout_output); + REQUIRE(result.success()); + REQUIRE(f.exists("build/a.out")); + REQUIRE_FALSE(f.exists("build/b.out")); + } + } + } +} + SCENARIO("Complementary branches may render the same command line", "[e2e][duplicate][identity][phi]") { GIVEN("a project whose two conditional branches carry identical rule text")