From ddd1f948d628ee6b49d9852542470b021ac90274 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 21 Sep 2026 19:05:50 +0800 Subject: [PATCH] graph: keep nodes that name no path out of the path namespace A `tup.config` key that matched a source file's name made that file unreachable. Three shapes, all reproduced with an A/B control differing only in the key's name: CONFIG_LICENSE=y, in-source, rule reads LICENSE -> "Nothing to do (up to date)" for ever, out.txt frozen at its first contents CONFIG_LICENSE=y, out-of-tree -B build, same rule -> "FAILED: cp build/LICENSE build/out.txt", cannot stat CONFIG_dep.h=y, header discovered by gcc -MD -> header edits invisible, on any build shape CONFIG_LICENSE=y, LICENSE reached by a glob rather than named -> same freeze Root cause: `add_file_node` interns `/` into a PathId and registers it in `path_to_node` and the parent's `dir_children` for every node it is handed, and a config variable is such a node - named with the bare key, parented to the config directory. So the variable owns a path. A rule input resolving through `ensure_file_node`, or a discovered dependency joining through the index's `files_by_path`, finds it and binds to it, and nothing ever stats the file behind it. The out-of-tree shape differs only in which path the variable lands on: the config directory is the build directory, and a variant build grounds an ungrounded input there. Path-addressability is now a property of the node's kind rather than of whichever producer created it. `is_path_addressable` answers false for `Variable`, `Condition` and `Phi` - nodes that stand for a value rather than for something on disk - and `add_file_node` and `files_by_path` are the two sites that consult it. The switch is exhaustive, so a new node type is a build error here rather than a silent entry in the path namespace. The gate is on the lookup maps only. A node still interns its PathId, because `get_full_path` reads it and `show graph` labels every non-command node with that; withholding it printed each config var, env var and tool fingerprint as an unlabelled node. Being unfindable by path is the invariant; having no path was an accident of where the gate first went. The config-variable reuse branch in `add_tupfile` is deleted with it. After the gate it can find no Variable, and the only node it could still adopt is a same-named file or directory - which would drop the config value out of command identity, since `compute_command_signature` folds a sticky source only when its type is `Variable`. Upstream reaches the same disjointness a different way: tup parents a `TUP_NODE_VAR` under the `tup.config` node itself rather than under its directory (`tup_db_get_tup_config_tent`, db.c:440-459, through `tup_db_read_vars`, updater.c:358 and :427, to `add_var`, db.c:4979), so its `unique(dir, name)` key separates them. That does not transfer to putup, whose path namespace is lexical: a Tupfile can spell `tup.config/LICENSE`, so a parent alone would relocate the bug rather than close it. Gating on kind does not depend on what a name can be spelled. INDEX_VERSION goes 27 to 28. It is load-bearing, not bookkeeping: without it a project whose index was written before this change stays wedged for ever, because `Index::compute_paths` reconstructs a Variable entry's path from parent and name whatever the writer stored, and `reconcile_input_set` then finds that stale entry at the source's path and never marks the real file as new. Verified both ways - wedged on a v27 index without the bump, repaired with it. `files_by_path`'s contract narrowed from "every entry with a non-empty path" to "every path-addressable entry", so the property test that pinned the old contract is updated to the new one and strengthened to assert that an unaddressable kind is not reachable by path. The version ledger in `format.hpp` gets no entry: it records layout changes that leave an older record unparseable, which is why 23, 24, 25 and 27 have none either. This bump is a wrong-join, so its reason is here instead. Residual: the property test's generator names every entry `n`, so no Variable ever shares a path with a File in it and the new assertion only witnesses re-admission, not shadowing; the four scenarios carry that half. `Group` nodes stay path-addressable, because group resolution goes through `find_by_dir_name`; a path spelled to collide with one is not reachable through today's lexer. #487 is still open underneath all of this - `add_file_node` discards `SortedPairVec::insert`'s duplicate-key return, so a collision that does slip through is still silent. Verified red-then-green on all three shapes, each failing on the parent commit for its own reason. make check: 182488 assertions in 954 test cases across 32 e2e shards. Ref: https://github.com/typeless/putup/issues/486 Co-Authored-By: Claude Opus 5 (1M context) --- include/pup/core/types.hpp | 26 ++++ include/pup/index/format.hpp | 2 +- spec/requirements/variant-builds.ears.md | 12 ++ src/graph/builder.cpp | 5 - src/graph/dag.cpp | 6 +- src/index/entry.cpp | 2 +- .../config_var_shadow/Tupfile.fixture | 1 + test/unit/test_e2e.cpp | 113 ++++++++++++++++++ test/unit/test_index.cpp | 13 +- 9 files changed, 170 insertions(+), 10 deletions(-) create mode 100644 test/e2e/fixtures/config_var_shadow/Tupfile.fixture diff --git a/include/pup/core/types.hpp b/include/pup/core/types.hpp index 01cd3a5d..9d27d281 100644 --- a/include/pup/core/types.hpp +++ b/include/pup/core/types.hpp @@ -186,6 +186,32 @@ constexpr auto names_node_type(std::uint8_t value) -> bool return false; } +/// Whether a node of this type names a path, and so may be reached by resolving one. A node that +/// stands for a value rather than for something on disk must answer false: a rule input, a glob or +/// a discovered dependency that resolved to one would bind to it and never be stat'd, which is a +/// build that goes stale or fails against a path that does not exist (#486). Exhaustive so that a +/// new node type is a build error here rather than a silent entry in the path namespace. +[[nodiscard]] +constexpr auto is_path_addressable(NodeType type) -> bool +{ + switch (type) { + case NodeType::File: + case NodeType::Command: + case NodeType::Directory: + case NodeType::Generated: + case NodeType::Ghost: + case NodeType::Group: + case NodeType::GeneratedDir: + case NodeType::Root: + return true; + case NodeType::Variable: + case NodeType::Condition: + case NodeType::Phi: + return false; + } + return false; +} + [[nodiscard]] constexpr auto names_link_type(std::uint8_t value) -> bool { diff --git a/include/pup/index/format.hpp b/include/pup/index/format.hpp index 7ab6f8d5..227e223a 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 { 27 }; +inline constexpr auto INDEX_VERSION = std::uint32_t { 28 }; /// 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/variant-builds.ears.md b/spec/requirements/variant-builds.ears.md index c6da7560..3b913d98 100644 --- a/spec/requirements/variant-builds.ears.md +++ b/spec/requirements/variant-builds.ears.md @@ -33,6 +33,18 @@ See `README.md` for the format and the rules that apply to every area. Which node a path spelled in a Tupfile names, when more than one spelling reaches the same file. +### REQ-VARIANT-NAME-IS-A-FILE + +- conformance: tup-conformant +- reference: upstream parents a configuration variable's node under the `tup.config` node itself rather than under its directory - `tup_db_get_tup_config_tent` finds or creates that node, `tup_db_read_vars` passes it down, and `add_var` creates the variable under it - so under tup's `unique(dir, name)` key a configuration variable and a source file can never collide; putup reaches the same disjointness by refusing a path to any node type that names none (`is_path_addressable`), because putup's path namespace is lexical and a parent alone would leave the collision spellable +- discharge: test "Scenario: A config variable does not hide a source file of the same name" +- discharge: test "Scenario: A config variable does not capture a rule's input in an out-of-tree build" +- discharge: test "Scenario: A config variable does not hide a discovered header of the same name" +- discharge: test "Scenario: A config variable does not hide a source file a glob matches" + +When a rule names an input, a glob matches a name, or a dep scan discovers one, putup shall +resolve that name to the file on disk rather than to a configuration variable that shares it. + ### REQ-VARIANT-OUTPUT-NODE - conformance: tup-conformant diff --git a/src/graph/builder.cpp b/src/graph/builder.cpp index 7e019d3b..d5dbad46 100644 --- a/src/graph/builder.cpp +++ b/src/graph/builder.cpp @@ -2487,11 +2487,6 @@ auto add_tupfile( auto var_name_id = to_underlying(intern(var_name)); - if (auto existing = find_by_dir_name(build_state.graph, config_dir_id, var_name)) { - state.config_var_nodes.insert(var_name_id, *existing); - continue; - } - auto value = eval.config_vars->get(var_name); auto node = FileNode { .type = NodeType::Variable, diff --git a/src/graph/dag.cpp b/src/graph/dag.cpp index 8ce148ae..b0f0e9b4 100644 --- a/src/graph/dag.cpp +++ b/src/graph/dag.cpp @@ -100,7 +100,9 @@ auto add_file_node(Graph& graph, FileNode node) -> Result } } node.path_id = graph.paths.intern(parent_path, node.name); - graph.path_to_node.insert(to_underlying(node.path_id), id); + if (is_path_addressable(node.type)) { + graph.path_to_node.insert(to_underlying(node.path_id), id); + } } auto const idx = node_id::index(id); @@ -110,7 +112,7 @@ auto add_file_node(Graph& graph, FileNode node) -> Result } graph.files[idx] = node; - if (!is_empty(graph.files[idx].name)) { + if (!is_empty(graph.files[idx].name) && is_path_addressable(graph.files[idx].type)) { auto const parent_idx = node_id::index(graph.files[idx].parent_dir); graph.dir_children[parent_idx].insert(to_underlying(graph.files[idx].name), id); } diff --git a/src/index/entry.cpp b/src/index/entry.cpp index ede50e02..c5e0b2cc 100644 --- a/src/index/entry.cpp +++ b/src/index/entry.cpp @@ -478,7 +478,7 @@ auto files_by_path(Index const& index) -> FilesByPath result.entries.reserve(index.files().size()); for (auto const& file : index.files()) { - if (!is_empty(file.path)) { + if (!is_empty(file.path) && is_path_addressable(file.type)) { result.entries.emplace_back(file.path, &file); } } diff --git a/test/e2e/fixtures/config_var_shadow/Tupfile.fixture b/test/e2e/fixtures/config_var_shadow/Tupfile.fixture new file mode 100644 index 00000000..8c1e134b --- /dev/null +++ b/test/e2e/fixtures/config_var_shadow/Tupfile.fixture @@ -0,0 +1 @@ +: LICENSE |> cp %f %o |> out.txt diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index b145fdf9..aac7da5c 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -9589,6 +9589,119 @@ SCENARIO("A variable named after a tool is still an ordinary exported variable", } } +SCENARIO("A config variable does not hide a source file of the same name", "[e2e][incremental][config]") +{ + GIVEN("an in-source project whose config key matches a rule's input file") + { + auto f = E2EFixture { "config_var_shadow" }; + REQUIRE(f.init().success()); + f.write_file("tup.config", "CONFIG_LICENSE=y\n"); + f.write_file("LICENSE", "MIT\n"); + REQUIRE(f.build().success()); + REQUIRE(f.read_file("out.txt") == "MIT\n"); + REQUIRE(f.build().is_noop()); + + WHEN("the shadowed source file changes") + { + f.write_file("LICENSE", "BSD\n"); + auto result = f.build(); + + THEN("the command re-runs against the new contents") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE(result.success()); + REQUIRE_FALSE(result.is_noop()); + REQUIRE(f.read_file("out.txt") == "BSD\n"); + } + } + } +} + +SCENARIO("A config variable does not capture a rule's input in an out-of-tree build", "[e2e][incremental][config]") +{ + GIVEN("an out-of-tree project whose config key matches a rule's input file") + { + auto f = E2EFixture { "config_var_shadow" }; + f.mkdir("build"); + f.write_file("LICENSE", "MIT\n"); + REQUIRE(f.pup({ "configure", "-B", "build" }).success()); + f.write_file("build/tup.config", "CONFIG_LICENSE=y\n"); + + WHEN("the project is built") + { + auto result = f.build({ "-B", "build" }); + + THEN("the input resolves to the source file rather than the config variable") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE(result.success()); + REQUIRE(f.read_file("build/out.txt") == "MIT\n"); + } + } + } +} + +SCENARIO("A config variable does not hide a discovered header of the same name", "[e2e][incremental][config]") +{ + GIVEN("a project whose config key matches a header the compiler discovers") + { + auto f = E2EFixture { "config_var_shadow" }; + REQUIRE(f.init().success()); + f.write_file("tup.config", "CONFIG_dep.h=y\n"); + f.write_file("dep.h", "#define V 1\n"); + f.write_file("main.c", "#include \"dep.h\"\nint main() { return V; }\n"); + f.write_file("Tupfile", ": main.c |> gcc -MD -c %f -o %o |> main.o\n"); + REQUIRE(f.build().success()); + REQUIRE(f.build().is_noop()); + + WHEN("the shadowed header changes") + { + f.write_file("dep.h", "#define V 2\n"); + auto result = f.build(); + + THEN("the command re-runs") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE(result.success()); + REQUIRE_FALSE(result.is_noop()); + } + } + } +} + +SCENARIO("A config variable does not hide a source file a glob matches", "[e2e][incremental][config]") +{ + GIVEN("a project whose config key matches a file a glob brings into the graph") + { + auto f = E2EFixture { "config_var_shadow" }; + f.write_file("LICENSE", "MIT\n"); + f.write_file("Tupfile", ": foreach *E |> cp %f %o |> %b.out\n"); + REQUIRE(f.init().success()); + f.write_file("tup.config", "CONFIG_LICENSE=y\n"); + REQUIRE(f.build().success()); + REQUIRE(f.read_file("LICENSE.out") == "MIT\n"); + REQUIRE(f.build().is_noop()); + + WHEN("the shadowed source file changes") + { + f.write_file("LICENSE", "BSD\n"); + auto result = f.build(); + + THEN("the command re-runs against the new contents") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE(result.success()); + REQUIRE_FALSE(result.is_noop()); + REQUIRE(f.read_file("LICENSE.out") == "BSD\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_index.cpp b/test/unit/test_index.cpp index eb36e66a..47084153 100644 --- a/test/unit/test_index.cpp +++ b/test/unit/test_index.cpp @@ -2267,9 +2267,15 @@ TEST_CASE("Keying a record's file table by path keeps every addressable entry an auto const by_path = files_by_path(index); auto expected = std::vector {}; + auto unaddressable = std::vector {}; for (auto const& file : index.files()) { - if (!pup::is_empty(file.path)) { + if (pup::is_empty(file.path)) { + continue; + } + if (pup::is_path_addressable(file.type)) { expected.push_back(&file); + } else { + unaddressable.push_back(&file); } } @@ -2293,6 +2299,11 @@ TEST_CASE("Keying a record's file table by path keeps every addressable entry an REQUIRE(found->path == file->path); } + for (auto const* file : unaddressable) { + auto const* found = by_path.find(file->path); + REQUIRE((found == nullptr || pup::is_path_addressable(found->type))); + } + REQUIRE(by_path.find(intern("no entry is ever recorded at this path " + std::to_string(seed))) == nullptr); REQUIRE(by_path.find(StringId::Empty) == nullptr); }