Repository navigation
graph: fold each command's own tool into its identity - #483
Merged
Merged
Conversation
CONFIG_TRACKED_TOOLS is empty by default, so in a default configuration a PATH change that resolved a command's tool to a different binary produced a stale build and no diagnostic: putup reported the project up to date while the compiler it would run had changed. The reproduction is a project whose only rule is `: |> mytool > %o |> out.txt`, with two directories each holding a different `mytool`; prepending the second to PATH re-ran nothing. #480 decided putup would not follow tup here. tup makes PATH a sticky env node of every command, so any PATH change re-runs everything, a cost tup.1 documents; a recorded PATH string cannot tell a change that shadows a tool from one that does not, and PATH varies between shells on one machine. That left the shadowing case reachable only through CONFIG_TRACKED_TOOLS, which nothing sets by default. This is that residue. create_command_node now resolves each command's first word — the first that is not a NAME=value assignment, skipped entirely when it contains '/' — and folds that binary's path, size and mtime into the command's identity, using the append_tool_stat and ensure_env_var_node machinery CONFIG_TRACKED_TOOLS already uses. The resolution is cached through imported_env_var_nodes, so the PATH walk runs once per distinct tool rather than once per command, and the seeding sits inside create_command_node so both call sites, ordinary rules and generated rules, are covered from one point. A word containing '/' is skipped because append_tool_stat joins such a name to the source root while a command runs in its Tupfile's directory, so the two disagree and the stat would be a wrong, stable <missing>. Words are screened for '=' rather than by is_env_assignment_word: for `VAR=hello; echo $VAR > %o` that predicate rejects the glued `VAR=hello;` as hiding shell syntax and would seed it, where the shell runs `echo`. Rejected: recording PATH as a witness node and forcing commands whose text names a basename the two PATH values resolve differently. It was larger and wrong under scoped builds — out-of-scope commands are carried forward from the old index and are not graph nodes, so nothing could force them, while the witness was refreshed to the new PATH, making the staleness permanent. Folding into identity recovers by construction, because a merged command keeps its recorded signature until it is next in scope. Also rejected: seeding the first word of each invocation rather than of the command. Across all 534 rule bodies in the repo it would additionally seed gcc three times and mv once — all in fixtures written to be compound, or after a `bison` that is already seeded — at the cost of 32 dependencies on /usr/bin/echo, which the shell runs as a builtin. Not covered, and stated in REQ-ENV-COMMAND-TOOL's reference: a tool reached past a shell operator, through `sh -c`, or through a wrapper or driver, and every bare name on Windows, where append_tool_stat does not consult PATHEXT. CONFIG_TRACKED_TOOLS remains the route for those. Verified red-then-green on the new scenario, which failed with "Nothing to do (up to date)" before the change. [envdep] 54 assertions and [incremental] 1250 assertions pass; spec-check reports 201 requirements and 0 gaps. Every existing index re-runs once on the first build after this lands, because each command gains a sticky input; no INDEX_VERSION bump, since nothing in the on-disk format changes. Ref: #482 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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)
Code churn (whole codebase, last 30d)
Of the lines written across the codebase in the last 30 days, how many are already gone — work that was written and then discarded or rewritten inside the same window. This is the state of the tree including this PR, not a measure of the PR itself. Only code we write is counted: tests, examples, vendored and generated files, CI plumbing and prose are excluded. 1592 lines were deleted in the window in total, most of them older than it. Where the churn is
Test coverage (lines)
105 files · 17850/20090 lines covered Deltas vs main@8dd8a5a6b. Updated for f442d43 |
typeless
added a commit
that referenced
this pull request
Sep 21, 2026
A Tupfile line as ordinary as `export TUP_TOOLCHAIN` silently disabled tool-change detection for the whole build: the PATH swap that should have re-run every command reported "Nothing to do (up to date)" and left the stale output in place, with no diagnostic. `export TUP_TOOL_<name>` did the same to the per-command tool stat added in #483, which is to say it reopened #482 through a name. Root cause: `ensure_env_var_node` keys every `NodeType::Variable` node by bare variable name in one map, and a find-by-name hit is treated as "same variable, new value" - it overwrites the node's name and content hash. That is right for a Tupfile variable and wrong for a node putup owns. Four producers share the key space: `process_export` and `process_import` on the user's side, the `TUP_TOOLCHAIN` fingerprint and the `TUP_TOOL_<name>` tool stat on putup's. Whichever runs second wins. putup's two now go through `ensure_internal_var_node`, which prefixes the key with `@`. No Tupfile can spell a name starting with `@`: `parse_export` and `parse_import` accept one Identifier or Text token, and `@` lexes as `TokenType::At` - so the two key spaces are disjoint by construction, not by a reserved-name list. Callers pass the bare name and never see the prefix, so a future synthetic node kind cannot forget to apply it. Two alternatives were measured against this one and rejected. Rejecting a reserved name at parse keeps two lists - the reserved names and the names internal producers actually emit - equal by hand, leaves the same hazard standing for every name not yet on the list, and adds a user-visible error where none is needed. A second map keyed by bare name leaves two further illegal states reachable: `add_file_node` discards `SortedPairVec::insert`'s duplicate-key signal, so a user node whose `NAME=VALUE` equals an internal one silently aliases it in `path_to_node`; and `cached_env_vars` would still serve the fingerprint to a later `import`. Nothing is reserved after this. `export TUP_TOOL_gcc` is an ordinary environment variable with the semantics it should always have had. A third scenario pins that, and passes with or without this change - it is a coexistence pin against a future fix that reserves the names instead, not part of the red. `show index` now prints the fingerprints as `$/@TUP_TOOLCHAIN=...` and `$/@TUP_TOOL_<name>=...`. No test snapshots those names and no document mentions them. Residual: the prefixed nodes still enter `cached_env_vars` in `src/cli/context.cpp`. No `import` can match an unspellable key, so the entries are inert; filtering them would put the prefix in a second file. Node names changed, so INDEX_VERSION goes 26 to 27 and the first build after this re-runs every command once. Verified red-then-green: both reported shapes fail with "Nothing to do (up to date)" and a stale output on a control built from the parent commit with the tests and version bump present and only the builder change reverted, and pass here. The unspellability law is exhaustive over all 255 leading bytes against both keywords, and was mutation-checked - asserting `P` instead of `@` fails it at byte 80. Ref: #484 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #482.
The gap
CONFIG_TRACKED_TOOLSis empty by default, so in a default configuration aPATHchange that resolved a command's tool to a different binary produced a stale build and no diagnostic — putup reported the project up to date while the compiler it would run had changed.#480 decided putup would not follow tup here: tup makes
PATHa sticky env node of every command, so anyPATHchange re-runs everything (a costtup.1documents), and a recordedPATHstring cannot tell a change that shadows a tool from one that does not. That left the shadowing case reachable only throughCONFIG_TRACKED_TOOLS, which nothing sets by default. This PR is that residue.What it does
create_command_noderesolves each command's first word — the first that is not aNAME=valueassignment, skipped entirely when it contains/— and folds that binary'spath:size:mtimeinto the command's identity, reusing theappend_tool_statandensure_env_var_nodemachineryCONFIG_TRACKED_TOOLSalready uses. Resolution is cached throughimported_env_var_nodes, so thePATHwalk runs once per distinct tool rather than once per command, and the seeding sits insidecreate_command_nodeso ordinary rules and generated rules are both covered from one point.On putup's own build this seeds exactly
g++×115,gcc×2,cp,ar, and oneecho.Two designs were killed on evidence, not argument
A
PATHwitness node + forced re-runs. Record thePATHcommands ran under as an edgeless node, and on a difference force every command whose text names a basename the twoPATHvalues resolve differently. Killed: out-of-scope commands in a scoped build are carried forward from the old index and are not graph nodes (cmd_build.cpp:1333-1340), so nothing could force them, while the witness advanced to the newPATH— making the staleness permanent and undetectable. The identity route recovers by construction, because a merged command keeps its recorded signature until it is next in scope.Seeding the first word of each invocation rather than of the command. A census of all 534 rule bodies in the repo shows it would additionally seed
gcc×3 andmv×1 — every one either a fixture written to be compound, or anmvfollowing abisonthat is already seeded — at a cost of 32 commands depending on/usr/bin/echo's mtime, which the shell runs as a builtin and never executes.Not covered
A tool reached past a shell operator (
cd build && gcc ...), throughsh -c, or through a wrapper script or compiler driver is not the first word; and on Windows every bare name records<missing>, becauseappend_tool_statdoes not consultPATHEXT.CONFIG_TRACKED_TOOLSremains the route for those, and all of it is stated inREQ-ENV-COMMAND-TOOL'sreference:rather than left for a reader to discover.Spec
REQ-ENV-COMMAND-TOOLadded tocommand-record.ears.md's env-values group.REQ-ENV-FORWARDED-IDENTITY's guard amended — its condition no longer named the only case in which aPATHchange re-runs a command.spec-check: 21 areas, 201 requirements, 0 gaps.Verification
Red-then-green: the new scenario failed with
Nothing to do (up to date)— the issue's exact symptom — before the change.One cost
The first build after this lands re-runs every command once, because each gains a sticky input. No
INDEX_VERSIONbump — nothing in the on-disk format changes.🤖 Generated with Claude Code
https://claude.ai/code/session_01StgwMENEyfBnEoe4pvAdtQ