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