From c02eeebbd5d067fc3f65c608e93e9c54a21e0eec Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 21 Sep 2026 17:20:12 +0800 Subject: [PATCH] graph: key putup's own tool fingerprints out of the Tupfile's namespace 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_` 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_` 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_=...`. 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: https://github.com/typeless/putup/issues/484 Co-Authored-By: Claude Opus 5 (1M context) --- include/pup/index/format.hpp | 2 +- spec/requirements/command-record.ears.md | 13 +++ src/graph/builder.cpp | 34 ++++++- test/unit/test_e2e.cpp | 118 +++++++++++++++++++++++ test/unit/test_parser.cpp | 28 ++++++ 5 files changed, 190 insertions(+), 5 deletions(-) diff --git a/include/pup/index/format.hpp b/include/pup/index/format.hpp index 6fd88fe6..7ab6f8d5 100644 --- a/include/pup/index/format.hpp +++ b/include/pup/index/format.hpp @@ -92,7 +92,7 @@ inline constexpr auto INDEX_MAGIC = std::array { 'P', 'U', 'P', 'I' }; /// `RawFileEntry::name_offset` is semantics-bearing on the same terms: `read_prior_paths` composes /// it into the paths `clean`/`distclean` delete and `reject_shadowed_sources` refuses a build over, /// so a name this reader cannot reproduce fails the record rather than reading as empty (#381). -inline constexpr auto INDEX_VERSION = std::uint32_t { 26 }; +inline constexpr auto INDEX_VERSION = std::uint32_t { 27 }; /// The oldest version whose `RawHeader` and `RawFileEntry` bytes mean what today's mean, so a /// record that old still says which paths it recorded as sources and which as generated even diff --git a/spec/requirements/command-record.ears.md b/spec/requirements/command-record.ears.md index 29cc06c4..b5c91cbf 100644 --- a/spec/requirements/command-record.ears.md +++ b/spec/requirements/command-record.ears.md @@ -786,6 +786,19 @@ putup shall fold the path, size and modification time it resolves each command's 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-INTERNAL-NAMES + +- leg: invariant +- conformance: putup-only +- discharge: test "Scenario: A Tupfile exporting the name of a command's own tool keeps tool-change detection" +- discharge: test "Scenario: A Tupfile exporting TUP_TOOLCHAIN keeps tracked-tool detection" +- discharge: test "Scenario: A variable named after a tool is still an ordinary exported variable" +- discharge: test "No export or import names a variable putup reserves for itself" + +Where a Tupfile exports or imports a variable named `TUP_TOOLCHAIN` or `TUP_TOOL_`, putup +shall track it as the ordinary environment variable it is, keeping the REQ-ENV-TRACKED-TOOLS and +REQ-ENV-COMMAND-TOOL fingerprints under keys outside the names a Tupfile can spell. + ### REQ-ENV-TRACKED-TOOLS - leg: invariant diff --git a/src/graph/builder.cpp b/src/graph/builder.cpp index b9a5e2c6..7e019d3b 100644 --- a/src/graph/builder.cpp +++ b/src/graph/builder.cpp @@ -480,6 +480,9 @@ auto resolve_group_operand_node( auto ensure_env_var_node(BuilderContext& ctx, Builder& state, std::string_view var_name, std::string_view value) -> std::optional; +auto find_internal_var_node(Builder const& state, std::string_view name) -> std::optional; +auto ensure_internal_var_node(BuilderContext& ctx, Builder& state, std::string_view 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( @@ -525,12 +528,11 @@ auto create_command_node( 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; + auto tool_node = find_internal_var_node(state, key_view); 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()); + tool_node = ensure_internal_var_node(ctx, state, key_view, stat.view()); } if (tool_node) { (void)add_edge(ctx.state->graph, *tool_node, cmd_id, LinkType::Sticky); @@ -1331,6 +1333,30 @@ auto ensure_env_var_node( return *result; } +constexpr auto INTERNAL_VAR_PREFIX = std::string_view { "@" }; + +auto internal_var_key(Buf& out, std::string_view name) -> void +{ + out += INTERNAL_VAR_PREFIX; + out += name; +} + +auto find_internal_var_node(Builder const& state, std::string_view name) -> std::optional +{ + auto key = Buf {}; + internal_var_key(key, name); + auto const* existing = state.imported_env_var_nodes.find(to_underlying(intern(key.view()))); + return (existing != nullptr) ? std::optional { *existing } : std::nullopt; +} + +auto ensure_internal_var_node(BuilderContext& ctx, Builder& state, std::string_view name, std::string_view value) + -> std::optional +{ + auto key = Buf {}; + internal_var_key(key, name); + return ensure_env_var_node(ctx, state, key.view(), value); +} + /// Append "path:size:mtime" for a tracked tool, or "" if it cannot be /// resolved. Bare names resolve through PATH (the same PATH build commands get); /// names containing a separator resolve against the source root. @@ -2514,7 +2540,7 @@ auto add_tupfile( append_tool_stat(fingerprint, name, str(state.options.source_root)); fingerprint += ';'; } - if (auto node = ensure_env_var_node(ctx, state, "TUP_TOOLCHAIN", fingerprint.view())) { + if (auto node = ensure_internal_var_node(ctx, state, "TUP_TOOLCHAIN", fingerprint.view())) { state.toolchain_node_id = *node; } } diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index e3c17181..b145fdf9 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -9471,6 +9471,124 @@ SCENARIO("A PATH change that swaps a tool no config tracks re-runs its commands" } } +SCENARIO("A Tupfile exporting the name of a command's own tool keeps tool-change detection", "[e2e][incremental][envdep]") +{ + GIVEN("a project that exports TUP_TOOL_ for the tool its rule leads with") + { + 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); + f.write_file("Tupfile", "export TUP_TOOL_mytool\n: |> mytool > %o |> out.txt\n"); + + 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 exported variable does not displace the tool stat, and the command re-runs") + { + 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("A Tupfile exporting TUP_TOOLCHAIN keeps tracked-tool detection", "[e2e][incremental][envdep]") +{ + GIVEN("a project that exports TUP_TOOLCHAIN and reaches its tracked tool past a shell") + { + 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); + f.write_file("Tupfile", "export TUP_TOOLCHAIN\n: |> sh -c 'mytool > %o' |> out.txt\n"); + f.write_file("build/tup.config", "CONFIG_TRACKED_TOOLS=mytool\n"); + + 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 tracked name to a different binary") + { + auto env = EnvGuard { "PATH", second + ":" + first + ":" + base }; + auto result = f.build({ "-B", "build" }); + + THEN("the exported variable does not displace the fingerprint, and the command re-runs") + { + 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("A variable named after a tool is still an ordinary exported variable", "[e2e][incremental][envdep]") +{ + GIVEN("a built project that exports TUP_TOOL_ and echoes it from the command") + { + auto f = E2EFixture { "tracked_tool_path" }; + f.mkdir("build"); + f.write_file("Tupfile", "export TUP_TOOL_mytool\n: |> echo \"[$TUP_TOOL_mytool]\" > %o |> out.txt\n"); + + { + auto env = EnvGuard { "TUP_TOOL_mytool", "one" }; + REQUIRE(f.pup({ "configure", "-B", "build" }).success()); + REQUIRE(f.build({ "-B", "build" }).success()); + REQUIRE(f.read_file("build/out.txt") == "[one]\n"); + REQUIRE(f.build({ "-B", "build" }).is_noop()); + } + + WHEN("the exported variable's value changes") + { + auto env = EnvGuard { "TUP_TOOL_mytool", "two" }; + auto result = f.build({ "-B", "build" }); + + THEN("the command re-runs and sees the new value") + { + 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") == "[two]\n"); + } + } + } +} + SCENARIO("Content change with preserved size and mtime", "[e2e][incremental]") { GIVEN("a built project whose input has an aged mtime") diff --git a/test/unit/test_parser.cpp b/test/unit/test_parser.cpp index f4505376..c099c4af 100644 --- a/test/unit/test_parser.cpp +++ b/test/unit/test_parser.cpp @@ -179,6 +179,34 @@ TEST_CASE("Parser export/import", "[parser]") } } +TEST_CASE("No export or import names a variable putup reserves for itself", "[parser]") +{ + for (int byte = 1; byte < 256; ++byte) { + auto const lead = static_cast(byte); + + for (auto const* keyword : { "export ", "import " }) { + auto source = std::string { keyword }; + source += lead; + source += "NAME"; + + INFO("keyword: " << keyword << " leading byte: " << byte); + auto result = parse_tupfile(source, "test.tup"); + if (!result.success() || result.tupfile.statements.empty()) { + continue; + } + + auto const* exp = result.tupfile.statements[0]->as(); + auto const* imp = result.tupfile.statements[0]->as(); + if (exp == nullptr && imp == nullptr) { + continue; + } + + auto const name = sv(exp != nullptr ? exp->var_name : imp->var_name); + REQUIRE_FALSE(name.starts_with('@')); + } + } +} + TEST_CASE("Parser error directive", "[parser]") { SECTION("error with message")