From 10643424c4f90dea5e982e9dff959fd9bd330b25 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Tue, 29 Sep 2026 23:01:27 +0800 Subject: [PATCH 1/2] graph: return the aliased node, not the entry that slides into its slot ensure_file_node's alias branches found a node through path_to_node.find, inserted the alias into the same SortedPairVec, and then read the id back through the pointer find had returned. The insert shifts every entry with a larger key, so when the alias key sorted below the target, the pointer read the neighbouring entry. The in-tree branch reaches it when a build path is interned before its source twin. #495's parse-time stat does exactly that: for an output d/f/x.o over a source file d/f inside an unbound directory d, the parser interns build d/f first, the stat binds source d/f as a File, and the alias for build d/f then returned the directory d. The output landed at d/x.o, a path no rule named, and the build succeeded. All three alias branches now copy the id before inserting. A sweep of every pointer taken from a container's find and read after a mutation of that container in src/ found no other site: the rest return before mutating or never read the pointer again. Verified red-then-green: the new unit test got node 2 (d) where the file is node 3. Ref: https://github.com/typeless/putup/issues/495 Co-Authored-By: Claude Opus 5.5 (1M context) --- src/graph/dag.cpp | 21 ++++++++++++--------- test/unit/test_graph.cpp | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/graph/dag.cpp b/src/graph/dag.cpp index e2e8d9d8..98281506 100644 --- a/src/graph/dag.cpp +++ b/src/graph/dag.cpp @@ -173,13 +173,14 @@ auto ensure_file_node(Graph& graph, PathId path_id, NodeType type) -> Resulttype == NodeType::Ghost || node->type == NodeType::File)) { - graph.path_to_node.insert(to_underlying(path_id), *hit); + graph.path_to_node.insert(to_underlying(path_id), hit_id); if (type == NodeType::Generated) { node->type = NodeType::Generated; } - return *hit; + return hit_id; } } } @@ -188,26 +189,28 @@ auto ensure_file_node(Graph& graph, PathId path_id, NodeType type) -> Resulttype == NodeType::Ghost || node->type == NodeType::File)) { node->type = NodeType::Generated; } } - return *hit; + return hit_id; } auto source_id = graph.paths.ground(path_id, PathId::SourceRoot); if (auto const* hit = graph.path_to_node.find(to_underlying(source_id))) { - graph.path_to_node.insert(to_underlying(path_id), *hit); + auto const hit_id = NodeId { *hit }; + graph.path_to_node.insert(to_underlying(path_id), hit_id); if (type == NodeType::Generated) { - auto* node = get_file_node(graph, *hit); + auto* node = get_file_node(graph, hit_id); if (node && (node->type == NodeType::Ghost || node->type == NodeType::File)) { node->type = NodeType::Generated; } } - return *hit; + return hit_id; } auto root = (type == NodeType::File || type == NodeType::Directory) diff --git a/test/unit/test_graph.cpp b/test/unit/test_graph.cpp index d1213389..7d322cc4 100644 --- a/test/unit/test_graph.cpp +++ b/test/unit/test_graph.cpp @@ -1793,6 +1793,24 @@ TEST_CASE("FileNode path_id populated by add_file_node", "[graph][path_pool]") } } +TEST_CASE("ensure_file_node returns the source node an in-tree build path aliases", "[graph]") +{ + auto bs = make_build_graph(); + auto& graph = bs.graph; + auto& pool = pup::global_pool(); + + auto build_twin = graph.paths.intern_path("d/f", pool, pup::PathId::BuildRoot); + auto source_file = graph.paths.intern_path("d/f", pool, pup::PathId::SourceRoot); + auto file_id = ensure_file_node(graph, source_file, NodeType::File); + REQUIRE(file_id.has_value()); + + auto aliased = ensure_file_node(graph, build_twin, NodeType::Directory); + + REQUIRE(aliased.has_value()); + REQUIRE(*aliased == *file_id); + REQUIRE(*graph.path_to_node.find(pup::to_underlying(build_twin)) == *file_id); +} + TEST_CASE("ensure_file_node creates nodes from PathId", "[graph]") { auto bs = make_build_graph(); From 0dc8b53521b6c7c39569a39245981e62156a8962 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Tue, 29 Sep 2026 23:01:27 +0800 Subject: [PATCH 2/2] graph: refuse an output under a file or onto a directory at parse time putup refused an output under the source file sub only when a rule had already read sub. Named first, the output minted a Directory node for sub, the later input bound to it, and the command failed in the shell ("Directory nonexistent"). And an output could claim a path an earlier output had made a directory (gen/x, then gen), failing in the shell with "Is a directory". Upstream refuses both at parse time: the first in find_dir_tupid_dt_pg, which knows the file from its scan, the second in validate_output. In an in-tree build, before binding an output or creating a group, putup now stats each component of the directory that no node holds yet, outermost first, and binds a regular file as a File node, so add_file_node's parent check (#494) refuses it. The graph stays free of disk access; the stat sits in the builder next to resolve_input_node, which already types source paths from the disk. Variant builds skip it: real tup accepts both an output and a group under a source file there, because both land in the variant tree. Outputs into directories that do not exist yet are unaffected. After binding, an output whose node is not a file, a generated file or a ghost is refused with upstream's message, naming the type. It fires under an unsatisfied guard too, as REQ-OUTPUT-INSIDE-HIERARCHY does. The output-group site discarded get_or_create_group_node's error, so a group under a file was refused only when some consumer happened to resolve the same group. It now returns the error. Not covered, recorded in the requirements: an output onto a source directory or a group's directory is not refused at parse time (tup refuses it; in-tree putup refuses the source directory later as a file the build does not own), and a group under an earlier rule's generated output is accepted. In a variant build, an input sub named after an output sub/x.o still binds to the output's directory node. Verified red-then-green: the new cases ran the command and failed in the shell, or built, before the change; removing the group-site stat turns its case red again. Real tup was run on the variant and group cases to settle the in-tree-only scope. The new stats run at parse time, once per directory component with no node yet (a Tupfile's own directory already has one); the stat-call metric counts only change detection (cmd_build.cpp), so it does not see them. Ref: https://github.com/typeless/putup/issues/495 Co-Authored-By: Claude Opus 5.5 (1M context) --- spec/requirements/output-paths.ears.md | 18 ++- src/graph/builder.cpp | 62 +++++++++- test/unit/test_e2e.cpp | 162 +++++++++++++++++++++++++ 3 files changed, 237 insertions(+), 5 deletions(-) diff --git a/spec/requirements/output-paths.ears.md b/spec/requirements/output-paths.ears.md index 2e1b2121..2f5d4734 100644 --- a/spec/requirements/output-paths.ears.md +++ b/spec/requirements/output-paths.ears.md @@ -57,12 +57,24 @@ putup shall reject the Tupfile and name that path, whether or not the rule's gua ### REQ-OUTPUT-UNDER-DIRECTORY - conformance: tup-conformant -- reference: upstream refuses the same class in `find_dir_tupid_dt_pg` (`src/tup/create_name_file.c`), which accepts only a directory or a generated directory as a path component of an output or a group and reports "Unable to output to a different directory"; upstream knows a source path's type from its scan, where putup learns it only once a rule names the path, so an output named before any rule reads the file is not refused +- reference: upstream refuses the same class in `find_dir_tupid_dt_pg` (`src/tup/create_name_file.c`), which accepts only a directory or a generated directory as a path component of an output or a group and reports "Unable to output to a different directory"; upstream knows a source path's type from its scan, where putup stats each component of the output's or group's directory that no node holds yet, so in an in-tree build the refusal does not depend on whether a rule reads the file before or after; in a variant build upstream places both in the variant tree and accepts them, and so does putup; putup does not yet refuse a group under an earlier rule's generated output, because the group's directory is a source path and the output is a build path, and in-tree putup aliases a build path to its source twin but not the reverse - discharge: test "add_file_node refuses a parent that is not a directory node" - discharge: test "Scenario: A rule cannot put an output or a group under a path that is not a directory" -If a rule names an output or a group under a path that the graph holds as a file, a generated file -or a group, then putup shall reject the Tupfile and name that path and what it is. +If a rule names an output under a path that the graph holds as a file, a generated file or a group, +or a group under a path that the graph holds as a file or a group, or, in an in-tree build, either +under a path that is a file in the source tree, then putup shall reject the Tupfile and name that +path and what it is. + +### REQ-OUTPUT-NOT-A-DIRECTORY + +- conformance: deliberate-deviation +- reference: upstream refuses the same class in `validate_output` (`src/tup/parser.c`) with "Attempting to insert '' as a generated node when it already exists as a different type"; putup differs three times: it names the type "directory" where upstream says "generated directory", because putup never creates a generated-directory node; it refuses a rule under an unsatisfied guard, which upstream never evaluates, so that whether the Tupfile is accepted does not depend on the configuration, as REQ-OUTPUT-INSIDE-HIERARCHY does; and it does not refuse at parse time an output onto a directory in the source tree or onto a group's directory, which upstream refuses (in-tree putup refuses the source directory later, as a file the build does not own) +- discharge: test "Scenario: A rule cannot output to a path another rule uses as a directory" + +If a rule declares an output at a path under which another rule has declared an output, then putup +shall reject the Tupfile and name that path and what it holds, whether or not the rule's guards are +satisfied. ## Group: canonicality diff --git a/src/graph/builder.cpp b/src/graph/builder.cpp index cf5d72c5..cc59f5db 100644 --- a/src/graph/builder.cpp +++ b/src/graph/builder.cpp @@ -408,6 +408,37 @@ auto resolve_input_node( return ensure_file_node(ctx.state->graph, path_id, NodeType::Ghost); } +auto bind_source_files_along(BuilderContext& ctx, PathId dir) -> Result +{ + auto& graph = ctx.state->graph; + if (!get_build_root_name(graph).empty() || is_root(dir) || graph.path_to_node.find(to_underlying(dir))) { + return {}; + } + auto& pool = global_pool(); + auto rel = pool.get(graph.paths.to_string(dir, pool)); + if (auto source_id = graph.paths.find_path(rel, pool, PathId::SourceRoot); + source_id && graph.path_to_node.find(to_underlying(*source_id))) { + return {}; + } + if (auto outer = bind_source_files_along(ctx, graph.paths.parent(dir)); !outer) { + return outer; + } + auto source_path = pool.get(pup::path::join(str(ctx.options.source_root), rel)); + auto is_source_file = pup::platform::is_file(source_path); + if (!is_source_file && !pup::platform::exists(source_path) && !is_empty(ctx.options.config_root) + && str(ctx.options.config_root) != str(ctx.options.source_root)) { + is_source_file = pup::platform::is_file(pool.get(pup::path::join(str(ctx.options.config_root), rel))); + } + if (!is_source_file) { + return {}; + } + auto file_id = ensure_file_node(graph, graph.paths.intern_path(rel, pool, PathId::SourceRoot), NodeType::File); + if (!file_id) { + return pup::unexpected(file_id.error()); + } + return {}; +} + /// Get all files that are members of a group (via file → group edges) /// Returns file NodeIds by finding all input edges to the group node auto get_group_members(Graph const& graph, NodeId group_id) -> Vec @@ -429,6 +460,9 @@ auto get_or_create_group_node( } auto dir_path_id = ctx.state->graph.paths.intern_path(directory, global_pool(), PathId::SourceRoot); + if (auto bound = bind_source_files_along(ctx, dir_path_id); !bound) { + return pup::unexpected(bound.error()); + } auto parent_id_result = ensure_file_node(ctx.state->graph, dir_path_id, NodeType::Directory); if (!parent_id_result) { return parent_id_result; @@ -2194,6 +2228,9 @@ auto expand_rule( for (auto i = std::size_t { 0 }; i < all_declared.size(); ++i) { auto output_path = all_declared[i]; auto const is_extra = i >= primary_count; + if (auto bound = bind_source_files_along(ctx, ctx.state->graph.paths.parent(output_path)); !bound) { + return pup::unexpected(bound.error()); + } auto output_id = ensure_file_node( ctx.state->graph, output_path, is_context_active(ctx) ? NodeType::Generated : NodeType::Ghost ); @@ -2201,6 +2238,26 @@ auto expand_rule( return pup::unexpected(output_id.error()); } + auto const bound_type = get_file_node(ctx.state->graph, *output_id)->type; + switch (bound_type) { + case NodeType::File: + case NodeType::Generated: + case NodeType::Ghost: + break; + case NodeType::Command: + case NodeType::Directory: + case NodeType::Variable: + case NodeType::Group: + case NodeType::GeneratedDir: + case NodeType::Root: + case NodeType::Condition: + case NodeType::Phi: { + auto err = Buf {}; + err.fmt("Attempting to insert '{}' as a generated node when it already exists as a different type ({})", get_full_path(ctx.state->graph, *output_id, ctx.state->path_cache), node_type_name(bound_type)); + return make_error(ErrorCode::DuplicateNode, err.view()); + } + } + auto output_inputs = get_inputs(ctx.state->graph, *output_id); if (!output_inputs.empty()) { for (auto existing_id : output_inputs) { @@ -2251,9 +2308,10 @@ auto expand_rule( } auto group_id_result = get_or_create_group_node(ctx, state, str(dir), str(*eff_output_oo_group)); - if (group_id_result) { - (void)add_edge(ctx.state->graph, *output_id, *group_id_result, LinkType::Group); + if (!group_id_result) { + return pup::unexpected(group_id_result.error()); } + (void)add_edge(ctx.state->graph, *output_id, *group_id_result, LinkType::Group); } } diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index 77b48fdc..66c75988 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -15929,6 +15929,85 @@ SCENARIO("A rule cannot put an output or a group under a path that is not a dire } } + WHEN("a rule outputs into the source file before another rule reads it") + { + f.write_file("Tupfile", ": |> echo hi > %o |> sub/x.o\n: sub |> cat %f > %o |> a.txt\n"); + auto result = f.build(); + + THEN("the Tupfile is refused before any command runs") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(result.success()); + REQUIRE(result.stderr_output.find("the file 'sub' is not a directory") != std::string::npos); + REQUIRE(result.stdout_output.find("echo hi") == std::string::npos); + } + } + + WHEN("a rule outputs two levels under the source file and no rule names the file") + { + f.write_file("Tupfile", ": |> echo hi > %o |> sub/deep/x.o\n"); + auto result = f.build(); + + THEN("the Tupfile is refused and names the file") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(result.success()); + REQUIRE(result.stderr_output.find("the file 'sub' is not a directory") != std::string::npos); + REQUIRE(result.stdout_output.find("echo hi") == std::string::npos); + } + } + + WHEN("a rule outputs under a source file inside a source directory no rule names") + { + f.mkdir("d"); + f.write_file("d/f", "x\n"); + f.write_file("Tupfile", ": |> echo hi > %o |> d/f/x.o\n"); + auto result = f.build(); + + THEN("the Tupfile is refused, names the file, and nothing is written in its directory") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(result.success()); + REQUIRE(result.stderr_output.find("the file 'd/f' is not a directory") != std::string::npos); + REQUIRE_FALSE(f.exists("d/x.o")); + } + } + + WHEN("a rule outputs into directories that do not exist yet") + { + f.write_file("Tupfile", ": |> echo hi > %o |> newdir/deep/x.o\n"); + auto result = f.build(); + + THEN("the build succeeds and writes the output") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE(result.success()); + REQUIRE(f.read_file("newdir/deep/x.o") == "hi\n"); + } + } + + WHEN("a refused output into the source file is retried after the file becomes a directory") + { + f.write_file("Tupfile", ": |> echo hi > %o |> sub/x.o\n"); + auto refused = f.build(); + f.remove_file("sub"); + f.mkdir("sub"); + auto result = f.build(); + + THEN("the first build is refused and the second writes the output") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(refused.success()); + REQUIRE(result.success()); + REQUIRE(f.read_file("sub/x.o") == "hi\n"); + } + } + WHEN("a rule outputs under another rule's output") { f.write_file("Tupfile", ": |> echo a > %o |> gen\n: |> echo b > %o |> gen/x\n"); @@ -15956,5 +16035,88 @@ SCENARIO("A rule cannot put an output or a group under a path that is not a dire REQUIRE(result.stderr_output.find("the file 'sub' is not a directory") != std::string::npos); } } + + WHEN("a variant build puts an output and a group under the source file") + { + f.mkdir("build"); + f.write_file("build/tup.config", ""); + f.write_file("Tupfile", ": |> echo hi > %o |> sub/x.o sub/\n"); + auto result = f.build({ "-B", "build" }); + + THEN("the build succeeds, because the variant tree holds no file named sub") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE(result.success()); + REQUIRE(f.read_file("build/sub/x.o") == "hi\n"); + } + } + + WHEN("a rule puts a group under the source file and no other rule names the file") + { + f.write_file("Tupfile", ": |> echo a > %o |> a.txt sub/\n"); + auto result = f.build(); + + THEN("the Tupfile is refused and names the file") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(result.success()); + REQUIRE(result.stderr_output.find("the file 'sub' is not a directory") != std::string::npos); + } + } + } +} + +SCENARIO("A rule cannot output to a path another rule uses as a directory", "[e2e][parse]") +{ + GIVEN("a project whose source tree holds a file named sub and a directory named srcdir") + { + auto f = E2EFixture { "output_under_non_directory" }; + + WHEN("a rule outputs to a path an earlier rule wrote under") + { + f.write_file("Tupfile", ": |> echo b > %o |> gen/x\n: |> echo a > %o |> gen\n"); + auto result = f.build(); + + THEN("the Tupfile is refused before any command runs and names the directory") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(result.success()); + REQUIRE(result.stderr_output.find("Attempting to insert 'gen' as a generated node when it already exists as a different type (directory)") != std::string::npos); + REQUIRE(result.stdout_output.find("echo b") == std::string::npos); + } + } + + WHEN("the same two rules build as a variant") + { + f.mkdir("build"); + f.write_file("build/tup.config", ""); + f.write_file("Tupfile", ": |> echo b > %o |> gen/x\n: |> echo a > %o |> gen\n"); + auto result = f.build({ "-B", "build" }); + + THEN("the Tupfile is refused and names the directory") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(result.success()); + REQUIRE(result.stderr_output.find("as a different type (directory)") != std::string::npos); + } + } + + WHEN("the rule outputting to the directory's path is under an unsatisfied guard") + { + f.write_file("Tupfile", ": |> echo b > %o |> gen/x\nifeq (@(FOO),y)\n: |> echo a > %o |> gen\nendif\n"); + auto result = f.build(); + + THEN("the Tupfile is still refused") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(result.success()); + REQUIRE(result.stderr_output.find("as a different type (directory)") != std::string::npos); + } + } } }