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); }