Split command identity into a join key and a signature (fixes #170, fixes #188) - #190
Conversation
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 %<group> 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
PR metricsPerformance (gcc example, Linux)
Deterministic signals: instructions (cachegrind-simulated instruction reads — exact across runs, no PMU needed), page faults, peak RSS, and the cachegrind D1/LL miss rates. CPU time is user+sys from time(1). Internal statistics (gcc example, up-to-date dry run)
Counters from Binary size (Linux)
Test coverage (lines)
103 files · 15157/17488 lines covered Deltas vs main@b3ffd393f. Updated for 17bf5a2 |
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
…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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
Command identity was one SHA-256 doing 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.
This started as #170's suggested fix — an
adjacent_findto make identity collisions loud — and became the split, because enforcement kept revealing that the invariant being enforced was the wrong one.1ee5c0b90a56b1c08c7ef17bf5a2What was wrong
Editing only a rule's recipe — same inputs, same outputs — made it a different rule:
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. And because the join key is the identity function's output, every improvement to that function invalidated all joins globally — v11, v15, v16, v17, v18 each forced a full rebuild.
Upstream already splits them
tup/src/tup/parser.c:3573:find_existing_commandwalks the rule's outputs and takes each one's incoming link. When that join lands on a command with a different string, tup callstup_db_set_nameand setscommand_modified— the node survives, the string is the change signal. putup had fused what upstream keeps apart.The split
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 load-bearing equivalence — "identity equality ⟺ same command", never proven and violated in five recorded instances (#167, #171, #177/#178, #170, and the dep-scan case) — is replaced by an invariant the graph builder enforces.
Result on the reproducer:
Changed command. The artifact is never deleted.It also got faster: the key is the output path, which both graph and index already hold as edges, so the join needs no hash. Command-index time 0.5ms → 0.0ms on putup's own graph; 3.5ms → 1.6ms on a 5000-command one.
INDEX_VERSION18 → 19;RawCommandEntry48 → 80 bytes.Two review passes, four defects, all mine
CI caught two. A dep-scan command is generated per compile with the parent's
-odropped, so two compiles of one source with equal flags render byte-identical scans — GCC does that, and my duplicate-check rejected it. Not an exemption: the discriminator was genuinely missing from the hash, which is #170's defect one level down. Separately, the configure pass evaluates beforetup.configexists, so rules gated on unset config vars render alike; the check now sits out that pass entirely.An adversarial review (Fable) caught two more, both blockers, both reproduced against a main-built binary:
Dropped outputs leaked permanently.
remove_stale_outputsasked "did this command survive" and skipped the entry when it had. Sound while the join was strict; broken once it was forgiving. A rule that drops one of two outputs joins through the survivor, and the dropped file — absent from the new index, so its ownership record is gone — is never deleted by any later build. Staleness is a property of a file: an output with no live producer is stale, whoever used to own it. Now implemented that way.The check bricked
configure. My empty-render exemption covered only fully empty text;./run @(A)and./run @(B)both render"./run ", so a legal project could not be configured at all — and configure is the step that creates the file distinguishing them.The two smaller findings shared one cause: the join rule existed in three ad-hoc copies (graph→index, index→graph, index→index) that had drifted apart, so the directions disagreed about whether a command survived. All three now go through one
CommandLookup/CommandAddresspair.Verification
make test: 142392 assertions, 634 cases, 32 e2e shards.make tidy,make iwyuclean. All 12 CI checks green.Behaviour checked by execution, each before and after: recipe edit keeps the artifact and reports
Changed command; dropping one of two outputs removes exactly that file; a deleted rule removes its output while its neighbour's survives; a renamed output is a new command with the old artifact removed; an exported-var change re-runs without deleting; scoped and out-of-tree builds unaffected.Real projects: GCC BSP configures, parses (24 Tupfiles, 1853 commands) and builds; busybox parses (31 Tupfiles, 293 commands); coverage configure completes.
Not claimed
preserve_old_implicit_edges/merge_out_of_scope_commandsmatching on bare key) was latent — no reproducer exists. The cause is fixed anyway by collapsing the three joins, but that part is reasoned, not demonstrated.metrics.command_expansionsis printed by--statand never incremented anywhere — noted, not fixed.🤖 Generated with Claude Code
https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA