diff --git a/DESIGN.md b/DESIGN.md index f73fc106..c84bf4b5 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -712,7 +712,6 @@ auto get_phi_node(Graph&, id) -> PhiNode*; // nullptr for non-phi ID // Phi-node model helpers auto add_condition_node(Graph&, ConditionNode) -> Result; auto add_phi_node(Graph&, PhiNode) -> Result; -auto resolve_phi_node(Graph const&, phi_id) -> NodeId; // Returns active output auto is_guard_satisfied(Graph const&, NodeId) -> bool; // String access helpers (resolve StringId -> string_view) diff --git a/spec/requirements/glob-expansion.ears.md b/spec/requirements/glob-expansion.ears.md index 0872d123..63d2afa9 100644 --- a/spec/requirements/glob-expansion.ears.md +++ b/spec/requirements/glob-expansion.ears.md @@ -90,6 +90,14 @@ Which paths a pattern names. putup shall expand a glob to both the source files on disk and the files produced by the project's rules. +### REQ-GLOB-FILES-ONLY + +- conformance: tup-conformant +- reference: upstream `nl_add_path` expands a rule's glob through `tup_db_select_node_dir_glob` with directories excluded, so only normal and generated files match +- discharge: test "Scenario: glob expansion matches files and never directories" + +putup shall expand a glob to the files it matches and never to a directory whose name it matches. + ### REQ-GLOB-DEDUP - conformance: unclassified diff --git a/spec/requirements/output-paths.ears.md b/spec/requirements/output-paths.ears.md index ca73c39f..2e1b2121 100644 --- a/spec/requirements/output-paths.ears.md +++ b/spec/requirements/output-paths.ears.md @@ -54,6 +54,16 @@ How far out of the tree a rule may write. If a rule declares an output whose path does not resolve to a location inside the build root, then putup shall reject the Tupfile and name that path, whether or not the rule's guards are satisfied. +### 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 +- 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. + ## Group: canonicality Which of a path's spellings the record carries. diff --git a/src/graph/builder.cpp b/src/graph/builder.cpp index d5dbad46..cf5d72c5 100644 --- a/src/graph/builder.cpp +++ b/src/graph/builder.cpp @@ -387,14 +387,14 @@ auto resolve_input_node( auto source_path = pool.get(pup::path::join(str(ctx.options.source_root), normalized_path)); if (pup::platform::exists(source_path)) { auto path_id = ctx.state->graph.paths.intern_path(normalized_path, pool, PathId::SourceRoot); - return ensure_file_node(ctx.state->graph, path_id, NodeType::File); + return ensure_file_node(ctx.state->graph, path_id, pup::platform::is_directory(source_path) ? NodeType::Directory : NodeType::File); } if (!is_empty(ctx.options.config_root) && str(ctx.options.config_root) != str(ctx.options.source_root)) { auto config_path_sv = pool.get(pup::path::join(str(ctx.options.config_root), normalized_path)); if (pup::platform::exists(config_path_sv)) { auto path_id = ctx.state->graph.paths.intern_path(normalized_path, pool, PathId::SourceRoot); - return ensure_file_node(ctx.state->graph, path_id, NodeType::File); + return ensure_file_node(ctx.state->graph, path_id, pup::platform::is_directory(config_path_sv) ? NodeType::Directory : NodeType::File); } } diff --git a/src/graph/dag.cpp b/src/graph/dag.cpp index 197ea98b..e2e8d9d8 100644 --- a/src/graph/dag.cpp +++ b/src/graph/dag.cpp @@ -88,15 +88,28 @@ auto validate_node_id(Graph const& graph, NodeId id) -> bool auto add_file_node(Graph& graph, FileNode node) -> Result { + auto parent_path = PathId::SourceRoot; + if (node.parent_dir != 0) { + auto const* parent = get_file_node(std::as_const(graph), node.parent_dir); + if (!parent) { + return make_error(ErrorCode::InvalidNodeId, "Invalid parent directory node ID"); + } + if (parent->type != NodeType::Directory && parent->type != NodeType::GeneratedDir) { + auto const parent_spelling = global_pool().get(materialize_path(graph, parent->path_id)); + auto err = Buf {}; + err.fmt( + "Unable to create '{}/{}' because the {} '{}' is not a directory", + parent_spelling, + global_pool().get(node.name), + node_type_name(parent->type), + parent_spelling + ); + return make_error(ErrorCode::InvalidNodeId, err.view()); + } + parent_path = parent->path_id; + } auto const parent_idx = node_id::index(node.parent_dir); if (!is_empty(node.name)) { - auto parent_path = PathId::SourceRoot; - if (node.parent_dir != 0) { - auto const* parent = get_file_node(std::as_const(graph), node.parent_dir); - if (parent) { - parent_path = parent->path_id; - } - } node.path_id = graph.paths.intern(parent_path, node.name); } @@ -494,21 +507,6 @@ auto get_phi_node(Graph& graph, NodeId id) -> PhiNode* return const_cast(get_phi_node(std::as_const(graph), id)); } -auto resolve_phi_node(Graph const& graph, NodeId phi_id) -> NodeId -{ - auto const* phi = get_phi_node(graph, phi_id); - if (!phi) { - return INVALID_NODE_ID; - } - - auto const* cond = get_condition_node(graph, phi->condition); - if (!cond) { - return INVALID_NODE_ID; - } - - return cond->current_value ? phi->then_output : phi->else_output; -} - auto is_guard_satisfied(Graph const& graph, NodeId id) -> bool { auto const* cmd = get_command_node(graph, id); diff --git a/src/parser/glob.cpp b/src/parser/glob.cpp index 1f765d4d..4141a04e 100644 --- a/src/parser/glob.cpp +++ b/src/parser/glob.cpp @@ -232,7 +232,7 @@ auto glob_expand( if (!options.include_hidden && !name_sv.empty() && name_sv[0] == '.') { return false; } - if (glob.matches(rel_path)) { + if (!entry.is_dir && glob.matches(rel_path)) { auto result_id = dir_part.empty() ? pool.intern(rel_path) : pup::path::join(dir_part, rel_path); results.push_back(result_id); } @@ -246,7 +246,7 @@ auto glob_expand( if (!options.include_hidden && !name_sv.empty() && name_sv[0] == '.') { continue; } - if (glob.matches(name_sv)) { + if (!entry.is_dir && glob.matches(name_sv)) { auto result_id = dir_part.empty() ? pool.intern(name_sv) : pup::path::join(dir_part, name_sv); results.push_back(result_id); } diff --git a/test/e2e/fixtures/output_under_non_directory/Tupfile.fixture b/test/e2e/fixtures/output_under_non_directory/Tupfile.fixture new file mode 100644 index 00000000..618ace6f --- /dev/null +++ b/test/e2e/fixtures/output_under_non_directory/Tupfile.fixture @@ -0,0 +1 @@ +: |> echo hi > %o |> ok.txt diff --git a/test/e2e/fixtures/output_under_non_directory/Tupfile.ini b/test/e2e/fixtures/output_under_non_directory/Tupfile.ini new file mode 100644 index 00000000..e69de29b diff --git a/test/e2e/fixtures/output_under_non_directory/srcdir/keep.c b/test/e2e/fixtures/output_under_non_directory/srcdir/keep.c new file mode 100644 index 00000000..b4c38db6 --- /dev/null +++ b/test/e2e/fixtures/output_under_non_directory/srcdir/keep.c @@ -0,0 +1 @@ +int keep; diff --git a/test/e2e/fixtures/output_under_non_directory/sub b/test/e2e/fixtures/output_under_non_directory/sub new file mode 100644 index 00000000..59768066 --- /dev/null +++ b/test/e2e/fixtures/output_under_non_directory/sub @@ -0,0 +1 @@ +not a directory diff --git a/test/e2e/fixtures/output_under_non_directory/tup.config b/test/e2e/fixtures/output_under_non_directory/tup.config new file mode 100644 index 00000000..e69de29b diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index aac7da5c..77b48fdc 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -15893,3 +15893,68 @@ SCENARIO("Numbered input flags name the basename and the basename without extens } } } + +SCENARIO("A rule cannot put an output or a group under a path that is not 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 reads the source directory and others read and write paths inside it") + { + f.write_file("Tupfile", ": srcdir |> ls %f > %o |> list.txt\n: srcdir/keep.c |> cp %f %o |> keep.o\n: |> echo x > %o |> srcdir/x.o\n"); + auto result = f.build(); + + THEN("the build succeeds") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE(result.success()); + REQUIRE(f.read_file("srcdir/x.o") == "x\n"); + } + } + + WHEN("a rule reads the source file and another outputs into it as if it were a directory") + { + f.write_file("Tupfile", ": sub |> cat %f > %o |> a.txt\n: |> echo hi > %o |> sub/x.o\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 under another rule's output") + { + f.write_file("Tupfile", ": |> echo a > %o |> gen\n: |> echo b > %o |> gen/x\n"); + auto result = f.build(); + + THEN("the Tupfile is refused and names the generated file") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(result.success()); + REQUIRE(result.stderr_output.find("the generated file 'gen' is not a directory") != std::string::npos); + } + } + + WHEN("a rule puts a group under the source file") + { + f.write_file("Tupfile", ": sub |> cat %f > %o |> a.txt sub/\n: sub/ |> echo used > %o |> b.txt\n"); + auto result = f.build(); + + THEN("the Tupfile is refused where the build used to succeed") + { + 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); + } + } + } +} diff --git a/test/unit/test_glob.cpp b/test/unit/test_glob.cpp index 6af772ae..7b7e067b 100644 --- a/test/unit/test_glob.cpp +++ b/test/unit/test_glob.cpp @@ -313,3 +313,40 @@ SCENARIO("glob expansion orders matches by path, not by interning order", "[glob } } } + +SCENARIO("glob expansion matches files and never directories", "[glob]") +{ + GIVEN("a directory holding a file and a subdirectory that itself holds a file") + { + auto tmp = TempDir {}; + fs::create_directory(tmp.path() / "gdir_sub"); + tmp.create_file("gdir_top.txt"); + tmp.create_file("gdir_sub/gdir_leaf.txt"); + + auto expanded = [&](std::string_view pattern) { + auto matches = glob_expand(pattern, tmp.path().string()); + REQUIRE(matches.has_value()); + auto result = std::vector {}; + for (auto id : *matches) { + result.push_back(sv(id)); + } + return result; + }; + + WHEN("a pattern that the subdirectory's name also matches is expanded") + { + THEN("only the file is a match") + { + REQUIRE(expanded("gdir_*") == std::vector { "gdir_top.txt" }); + } + } + + WHEN("a recursive pattern is expanded") + { + THEN("the files at every depth match and the subdirectory does not") + { + REQUIRE(expanded("**") == std::vector { "gdir_sub/gdir_leaf.txt", "gdir_top.txt" }); + } + } + } +} diff --git a/test/unit/test_graph.cpp b/test/unit/test_graph.cpp index b9d90361..d1213389 100644 --- a/test/unit/test_graph.cpp +++ b/test/unit/test_graph.cpp @@ -1682,6 +1682,68 @@ TEST_CASE("add_file_node refuses a second node at an occupied path", "[graph][pa } } +TEST_CASE("add_file_node refuses a parent that is not a directory node", "[graph][path_pool]") +{ + auto bs = make_build_graph(); + auto& g = bs.graph; + + auto src_dir = add_file_node(g, FileNode { .type = NodeType::Directory, .name = intern("src") }); + REQUIRE(src_dir.has_value()); + auto root_foo = add_file_node(g, FileNode { .name = intern("foo.c") }); + REQUIRE(root_foo.has_value()); + auto const next_id = g.next_file_id; + + SECTION("an id no node was minted under is an error") + { + auto orphan = add_file_node(g, FileNode { .name = intern("bar.c"), .parent_dir = pup::NodeId { 999 } }); + REQUIRE_FALSE(orphan.has_value()); + REQUIRE(orphan.error().code == pup::ErrorCode::InvalidNodeId); + REQUIRE(g.next_file_id == next_id); + REQUIRE(find_by_dir_name(g, pup::NodeId { 0 }, "bar.c") == std::nullopt); + } + + SECTION("a command id is an error even when its index matches a directory") + { + auto const cmd = pup::node_id::make_command(pup::node_id::index(*src_dir)); + auto orphan = add_file_node(g, FileNode { .name = intern("bar.c"), .parent_dir = cmd }); + REQUIRE_FALSE(orphan.has_value()); + REQUIRE(orphan.error().code == pup::ErrorCode::InvalidNodeId); + REQUIRE(find_by_dir_name(g, *src_dir, "bar.c") == std::nullopt); + } + + SECTION("a parent that is a file rather than a directory is an error naming what it is") + { + auto child = add_file_node(g, FileNode { .name = intern("x.o"), .parent_dir = *root_foo }); + REQUIRE_FALSE(child.has_value()); + REQUIRE(child.error().code == pup::ErrorCode::InvalidNodeId); + REQUIRE(pup::global_pool().get(child.error().message).find("the file 'foo.c' is not a directory") != std::string_view::npos); + REQUIRE(g.next_file_id == next_id); + } + + SECTION("a parent that is a group is an error") + { + auto group = add_file_node(g, FileNode { .type = NodeType::Group, .name = intern(""), .parent_dir = *src_dir }); + REQUIRE(group.has_value()); + auto child = add_file_node(g, FileNode { .name = intern("x.o"), .parent_dir = *group }); + REQUIRE_FALSE(child.has_value()); + REQUIRE(child.error().code == pup::ErrorCode::InvalidNodeId); + } + + SECTION("the build root is a directory a node may sit under") + { + auto child = add_file_node(g, FileNode { .type = NodeType::Generated, .name = intern("x.o"), .parent_dir = pup::BUILD_ROOT_ID }); + REQUIRE(child.has_value()); + } + + SECTION("the refusal leaves the source root's entry of the same name alone") + { + auto orphan = add_file_node(g, FileNode { .name = intern("foo.c"), .parent_dir = pup::NodeId { 999 } }); + REQUIRE_FALSE(orphan.has_value()); + REQUIRE(orphan.error().code == pup::ErrorCode::InvalidNodeId); + REQUIRE(find_by_dir_name(g, pup::NodeId { 0 }, "foo.c") == *root_foo); + } +} + TEST_CASE("FileNode path_id populated by add_file_node", "[graph][path_pool]") { auto bs = make_build_graph();