From f442d435cc0883aef3ea74eeb1d89ae5177291f1 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Sun, 20 Sep 2026 22:28:38 +0800 Subject: [PATCH] graph: fold each command's own tool into its identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 . 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: https://github.com/typeless/putup/issues/482 Co-Authored-By: Claude Opus 5 (1M context) --- docs/reference.md | 19 ++++++++--- spec/requirements/command-record.ears.md | 19 +++++++++-- src/graph/builder.cpp | 30 +++++++++++++++++ test/unit/test_e2e.cpp | 41 ++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 8 deletions(-) diff --git a/docs/reference.md b/docs/reference.md index a9e73edd..02af6f24 100644 --- a/docs/reference.md +++ b/docs/reference.md @@ -1159,10 +1159,16 @@ Only `export`ed variables are folded into command identity, so changing an exported variable's **value** re-runs the affected commands even when the command text is unchanged. The forwarded set above is **not** recorded: a command whose text reads `$PATH`, `$HOME` or `$TMPDIR` sees whatever the -invoking shell had, and changing that value alone re-runs nothing, except that a -bare name in `CONFIG_TRACKED_TOOLS` (§6.1) is resolved through putup's own -`PATH`, so a `PATH` change that resolves it to a different binary re-runs every -command while one that does not re-runs nothing. `export` the +invoking shell had, and changing that value alone re-runs nothing. `PATH` is the +exception, and only by resolution, never by value: putup resolves the first word +of every command — the first that is not a `NAME=value` assignment, and only when +it contains no `/` — and folds that binary's path, size and mtime into the +command's identity, so a `PATH` change that resolves `gcc` to a different +compiler re-runs the commands that name it, while one that moves nothing they +name re-runs nothing. A tool a command reaches only past a shell operator +(`cd build && gcc ...`), through `sh -c`, or through a wrapper script or compiler +driver is not the first word and is not covered — name it in +`CONFIG_TRACKED_TOOLS` (§6.1). `export` the variable if its value must decide whether a command is out of date. **`import`** - Import an environment variable into the Tupfile namespace: @@ -1287,7 +1293,10 @@ containing `/` against the source root) and its toolchain upgrade — same command text, same inputs, different compiler — then triggers a rebuild, closing the gap that command text alone cannot see. A missing tool records ``, so it appearing later also rebuilds. -Unset (the default) disables tracking. The fingerprint trusts size+mtime, +Unset (the default) tracks only each command's own first word (§ `export`); name +a tool here when a rule reaches it past a shell operator, through `sh -c`, or +through a wrapper or driver, or to catch an in-place upgrade of a tool no command +names directly. The fingerprint trusts size+mtime, the same trade-off as the stat cache (§9.1). **CLI Overrides:** diff --git a/spec/requirements/command-record.ears.md b/spec/requirements/command-record.ears.md index b87e6ad7..29cc06c4 100644 --- a/spec/requirements/command-record.ears.md +++ b/spec/requirements/command-record.ears.md @@ -764,14 +764,27 @@ empty. - leg: invariant - conformance: deliberate-deviation -- reference: upstream forwards its default set because it records it - `environ_add_defaults` (environ.c, called per Tupfile from `parse` in parser.c) makes each `default_env[]` name a sticky env node of that Tupfile's commands, `tup_db_check_env` (db.c) marks the node modified when `getenv` disagrees with the stored `VAR=value` (`env_cb`, both-NULL matching), and `tup_db_get_environ` builds the subprocess block from those sticky entries alone plus `CCACHE_NODIRECT=1` - so tup re-runs every command when `PATH` or `HOME` changes, which tup.1's `export` entry states for `PATH` ("if PATH is changed, all commands will run again"), and on Windows also when `TEMP`, `TMP` or a Visual Studio variable changes; upstream forwards no `TMPDIR`, so on that one variable the two systems agree that a change re-runs nothing; `--no-environ-check` (updater.c) skips the comparison, but tup.1 gives the monitor's frozen environment as its purpose rather than the cost of the check; putup forwards the set unrecorded because these values say where the invoking shell put things rather than what a command produces, the axis REQ-ENV-IMPORTED already settled, and because a `PATH` that varies between one shell and the next would otherwise re-run every command in the project, putup's own test runner included; the case a recorded `PATH` exists to catch is reached instead by REQ-ENV-TRACKED-TOOLS, which re-runs on a `PATH` change that resolves a tracked name to a different binary and not on one that does not, a distinction a recorded `PATH` string cannot draw; a project that wants upstream's behaviour writes `export PATH`, which REQ-ENV-SUBPROCESS folds into identity +- reference: upstream forwards its default set because it records it - `environ_add_defaults` (environ.c, called per Tupfile from `parse` in parser.c) makes each `default_env[]` name a sticky env node of that Tupfile's commands, `tup_db_check_env` (db.c) marks the node modified when `getenv` disagrees with the stored `VAR=value` (`env_cb`, both-NULL matching), and `tup_db_get_environ` builds the subprocess block from those sticky entries alone plus `CCACHE_NODIRECT=1` - so tup re-runs every command when `PATH` or `HOME` changes, which tup.1's `export` entry states for `PATH` ("if PATH is changed, all commands will run again"), and on Windows also when `TEMP`, `TMP` or a Visual Studio variable changes; upstream forwards no `TMPDIR`, so on that one variable the two systems agree that a change re-runs nothing; `--no-environ-check` (updater.c) skips the comparison, but tup.1 gives the monitor's frozen environment as its purpose rather than the cost of the check; putup forwards the set unrecorded because these values say where the invoking shell put things rather than what a command produces, the axis REQ-ENV-IMPORTED already settled, and because a `PATH` that varies between one shell and the next would otherwise re-run every command in the project, putup's own test runner included; the case a recorded `PATH` exists to catch is reached instead by REQ-ENV-COMMAND-TOOL in every project and by REQ-ENV-TRACKED-TOOLS where a project names more, each of which re-runs on a `PATH` change that resolves a name to a different binary and not on one that does not, a distinction a recorded `PATH` string cannot draw; a project that wants upstream's behaviour writes `export PATH`, which REQ-ENV-SUBPROCESS folds into identity - discharge: test "Scenario: Changing TMPDIR alone re-runs no command" - discharge: test "Scenario: Changing HOME alone re-runs no command" - discharge: test "Scenario: Changing PATH alone re-runs no command" While a Tupfile neither exports nor imports a variable putup forwards to every command's subprocess -by default, and no `CONFIG_TRACKED_TOOLS` entry resolves through that variable, putup shall exclude -its value from the identity of every command it declares. +by default, and neither a `CONFIG_TRACKED_TOOLS` entry nor a command's own first word resolves +through that variable, putup shall exclude its value from the identity of every command it +declares. + +### REQ-ENV-COMMAND-TOOL + +- leg: invariant +- conformance: deliberate-deviation +- reference: upstream reaches this case through `PATH` itself - `environ_add_defaults` (environ.c) makes `PATH` a sticky env node of every Tupfile's commands and `env_cb` (db.c) marks it modified whenever `getenv` disagrees with the stored value - so tup re-runs every command on any `PATH` change, the cost tup.1's `export` entry states; putup resolves the one name each command actually leads with and folds that binary's stat instead, so a `PATH` change that moves the tool re-runs the commands that name it and one that does not re-runs nothing, which is what REQ-ENV-FORWARDED-IDENTITY's exclusion of `PATH`'s value leaves uncovered; the first word is taken after any leading `NAME=value` words because the shell runs those as assignments rather than as the program, and 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; a tool a command reaches only past a shell operator, through `sh -c`, or through a wrapper or driver is not the first word and is not covered - `CONFIG_TRACKED_TOOLS` is the route for those, and on Windows every bare name records `` because `append_tool_stat` does not consult `PATHEXT` +- discharge: test "Scenario: A PATH change that swaps a tool no config tracks re-runs its commands" +- discharge: test "Scenario: Changing PATH alone re-runs no command" + +putup shall fold the path, size and modification time it resolves each command's first word to - +the first word that is not a `NAME=value` assignment, and only where that word contains no `/` - +into the identity of that command. ### REQ-ENV-TRACKED-TOOLS diff --git a/src/graph/builder.cpp b/src/graph/builder.cpp index f07b5ecc..b9a5e2c6 100644 --- a/src/graph/builder.cpp +++ b/src/graph/builder.cpp @@ -478,6 +478,10 @@ auto resolve_group_operand_node( return get_or_create_group_node(ctx, state, str(parsed->group_dir), parsed->group_name); } +auto ensure_env_var_node(BuilderContext& ctx, Builder& state, std::string_view var_name, std::string_view value) + -> std::optional; +auto append_tool_stat(Buf& out, std::string_view name, std::string_view source_root) -> void; + auto create_command_node( BuilderContext& ctx, Builder& state, @@ -507,6 +511,32 @@ auto create_command_node( (void)add_edge(ctx.state->graph, src_id, cmd_id, LinkType::Sticky); } + auto tool_words = tokenize_command(get(ctx.state->graph, cmd_id)); + auto tool = std::string_view {}; + for (auto word : tool_words.words()) { + if (word.find('=') != std::string_view::npos) { + continue; + } + tool = word; + break; + } + if (!tool.empty() && tool.find('/') == std::string_view::npos) { + auto key = Buf {}; + key += "TUP_TOOL_"; + key += tool; + auto const key_view = key.view(); + auto const* cached = state.imported_env_var_nodes.find(to_underlying(intern(key_view))); + auto tool_node = (cached != nullptr) ? std::optional { *cached } : std::nullopt; + if (!tool_node) { + auto stat = Buf {}; + append_tool_stat(stat, tool, str(state.options.source_root)); + tool_node = ensure_env_var_node(ctx, state, key_view, stat.view()); + } + if (tool_node) { + (void)add_edge(ctx.state->graph, *tool_node, cmd_id, LinkType::Sticky); + } + } + auto const* cv = ctx.used_config_vars.data(); for (std::size_t i = 0, n = ctx.used_config_vars.size(); i < n; ++i) { auto const* node_id = state.config_var_nodes.find(cv[i]); diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index 464f312f..e3c17181 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -9430,6 +9430,47 @@ SCENARIO("A PATH change that shadows a tracked tool re-runs its commands", "[e2e } } +SCENARIO("A PATH change that swaps a tool no config tracks re-runs its commands", "[e2e][incremental][envdep]") +{ + GIVEN("a default-configured project whose rule names a tool found on PATH") + { + auto f = E2EFixture { "tracked_tool_path" }; + f.mkdir("build"); + f.mkdir("first"); + f.mkdir("second"); + f.write_file("first/mytool", "#!/bin/sh\necho v1\n"); + f.write_file("second/mytool", "#!/bin/sh\necho v2\n"); + REQUIRE(f.run("/bin/chmod", { "+x", "first/mytool", "second/mytool" }).exit_code == 0); + + auto const* inherited = std::getenv("PATH"); + auto base = std::string { inherited != nullptr ? inherited : "/usr/bin:/bin" }; + auto first = (f.workdir() / "first").string(); + auto second = (f.workdir() / "second").string(); + { + auto env = EnvGuard { "PATH", first + ":" + base }; + REQUIRE(f.pup({ "configure", "-B", "build" }).success()); + REQUIRE(f.build({ "-B", "build" }).success()); + REQUIRE(f.read_file("build/out.txt") == "v1\n"); + REQUIRE(f.build({ "-B", "build" }).is_noop()); + } + + WHEN("a PATH change resolves the rule's tool to a different binary") + { + auto env = EnvGuard { "PATH", second + ":" + first + ":" + base }; + auto result = f.build({ "-B", "build" }); + + THEN("the command re-runs against the newly resolved tool") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE(result.success()); + REQUIRE_FALSE(result.is_noop()); + REQUIRE(f.read_file("build/out.txt") == "v2\n"); + } + } + } +} + SCENARIO("Content change with preserved size and mtime", "[e2e][incremental]") { GIVEN("a built project whose input has an aged mtime")