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/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_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); + } + } } } 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();