graph: key putup's own tool fingerprints out of the Tupfile's namespace

include/pup/index/format.hpp

@@ -92,7 +92,7 @@ inline constexpr auto INDEX_MAGIC = std::array<char, 4> { '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

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_<name>`, 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

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<NodeId>;
+auto find_internal_var_node(Builder const& state, std::string_view name) -> std::optional<NodeId>;
+auto ensure_internal_var_node(BuilderContext& ctx, Builder& state, std::string_view name, std::string_view value)
+    -> std::optional<NodeId>;
 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<NodeId> { *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<NodeId>
+{
+    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<NodeId> { *existing } : std::nullopt;
+}
+
+auto ensure_internal_var_node(BuilderContext& ctx, Builder& state, std::string_view name, std::string_view value)
+    -> std::optional<NodeId>
+{
+    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 "<missing>" 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;
             }
         }

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_<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_<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")

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<char>(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<Export>();
+            auto const* imp = result.tupfile.statements[0]->as<Import>();
+            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")