From dbca43cf7a959414cd8910f5f312b8f9442faa47 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 27 Jul 2026 22:57:27 +0800 Subject: [PATCH 1/2] Resolve glob matches in one path space (fixes #191) expand_glob_pattern merges two sources of matches: a filesystem scan, whose paths are source-relative, and a scan of Generated graph nodes, whose paths carry the build-root prefix. Out-of-tree those are different path spaces, and everything downstream compared them as plain strings -- the sort, the dedup, and apply_exclusions. Three consequences, all reproduced: %f order depended on the name of the build directory, because "../AAA/a/gen.dat" sorts before "keep.dat" and "../zz/a/gen.dat" sorts after: putup -B AAA -> cat ../AAA/a/gen.dat keep.dat all.txt: GEN\nKEEP putup -B zz -> cat keep.dat ../zz/a/gen.dat all.txt: KEEP\nGEN A generated file shadowing a same-named source appeared twice, since the two halves spelled it "a/x.dat" and "build/a/x.dat" and the dedup kept both: cat ../build/a/x.dat ../build/a/x.dat > ../build/a/all.txt An exclusion, normalized source-relative, never matched a build-prefixed generated path, so !skip.gen was honoured in-tree and silently ignored out-of-tree. The loop already computed the build-root-stripped path to run the glob against and then discarded it in favour of the physical one. It now pushes the stripped path, so both halves are source-relative and the existing sort, dedup and exclusion comparison are correct as written. resolve_input_node looks under BuildRoot before SourceRoot at source-relative paths, so the stripped spelling still names the generated node -- and the node is known to exist, because these matches come from iterating the Generated nodes themselves. PR #180 introduced this merge, and a follow-up commit normalized the ../ component lexically. That was one axis of a two-axis problem; this is the other. Three e2e scenarios, each the shape that was never covered -- a glob matching a generated and a source file, out-of-tree: order identical under two differently named build directories, a shadowed file consumed once, an exclusion honoured. Each observed failing first. Also checked by hand, since the previous fix in this area got a related case half-right: a ../ pattern crossing directories out-of-tree resolves to distinct files in source-relative order with correct physical paths rendered. GCC BSP parses (24 Tupfiles, 1853 commands). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA --- src/graph/builder.cpp | 10 ++- .../fixtures/glob_mixed_space/Tupfile.fixture | 2 + .../e2e/fixtures/glob_mixed_space/Tupfile.ini | 1 + test/e2e/fixtures/glob_mixed_space/tup.config | 1 + test/unit/test_e2e.cpp | 87 +++++++++++++++++++ 5 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 test/e2e/fixtures/glob_mixed_space/Tupfile.fixture create mode 100644 test/e2e/fixtures/glob_mixed_space/Tupfile.ini create mode 100644 test/e2e/fixtures/glob_mixed_space/tup.config diff --git a/src/graph/builder.cpp b/src/graph/builder.cpp index bbdfca8f..c16baaeb 100644 --- a/src/graph/builder.cpp +++ b/src/graph/builder.cpp @@ -694,11 +694,17 @@ auto expand_glob_pattern( } auto match_path_sv = pool.get(pup::strip_path_prefix(node_path_sv, build_root_name)); if (glob.matches(match_path_sv)) { - matches.push_back(pool.intern(node_path_sv)); + // The stripped path, not the physical one: out-of-tree the two halves would + // otherwise be in different path spaces, and everything below compares strings. + // resolve_input_node looks under BuildRoot first, so this still names the + // generated node. + matches.push_back(pool.intern(match_path_sv)); } } - // In-tree builds see a generated file from both sources; it is one input. + // Both halves are now source-relative, so a generated file seen from both is one input, + // ordering does not depend on where the build tree lives, and an exclusion written + // source-relative can match. std::ranges::sort(matches, {}, [&pool](StringId id) { return pool.get(id); }); matches.erase(std::unique(matches.begin(), matches.end()), matches.end()); for (auto id : matches) { diff --git a/test/e2e/fixtures/glob_mixed_space/Tupfile.fixture b/test/e2e/fixtures/glob_mixed_space/Tupfile.fixture new file mode 100644 index 00000000..21d376eb --- /dev/null +++ b/test/e2e/fixtures/glob_mixed_space/Tupfile.fixture @@ -0,0 +1,2 @@ +# Placeholder - overwritten by test +: |> true |> diff --git a/test/e2e/fixtures/glob_mixed_space/Tupfile.ini b/test/e2e/fixtures/glob_mixed_space/Tupfile.ini new file mode 100644 index 00000000..05e60154 --- /dev/null +++ b/test/e2e/fixtures/glob_mixed_space/Tupfile.ini @@ -0,0 +1 @@ +# Project root marker diff --git a/test/e2e/fixtures/glob_mixed_space/tup.config b/test/e2e/fixtures/glob_mixed_space/tup.config new file mode 100644 index 00000000..f4bd2fae --- /dev/null +++ b/test/e2e/fixtures/glob_mixed_space/tup.config @@ -0,0 +1 @@ +# Test config diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index 8f33a7ff..355fd297 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -4522,6 +4522,93 @@ SCENARIO("Editing a rule's recipe does not make it a different rule", "[e2e][ide } } +SCENARIO("A glob's %f order does not depend on the build directory's name", "[e2e][glob][pathspace]") +{ + GIVEN("a glob matching one generated and one source file, built out-of-tree") + { + auto f = E2EFixture { "glob_mixed_space" }; + f.mkdir("a"); + f.write_file("a/Tupfile", ": src.in |> cp %f %o |> gen.dat\n: *.dat |> cat %f > %o |> all.txt\n"); + f.write_file("a/src.in", "GEN\n"); + f.write_file("a/keep.dat", "KEEP\n"); + + WHEN("built into two differently-named build directories") + { + f.mkdir("AAA"); + REQUIRE(f.pup({ "configure", "-B", "AAA" }).success()); + REQUIRE(f.build({ "-B", "AAA" }).success()); + auto from_aaa = f.read_file("AAA/a/all.txt"); + + f.mkdir("zz"); + REQUIRE(f.pup({ "configure", "-B", "zz" }).success()); + REQUIRE(f.build({ "-B", "zz" }).success()); + auto from_zz = f.read_file("zz/a/all.txt"); + + THEN("the artifact is byte-identical") + { + INFO("AAA: " << from_aaa); + INFO("zz: " << from_zz); + REQUIRE(from_aaa == from_zz); + } + } + } +} + +SCENARIO("A generated file shadowing a source file is one glob match", "[e2e][glob][pathspace]") +{ + GIVEN("a generated file whose name also exists as a checked-in source") + { + auto f = E2EFixture { "glob_mixed_space" }; + f.mkdir("a"); + f.write_file("a/Tupfile", ": gen.src |> cp %f %o |> x.dat\n: *.dat |> cat %f > %o |> all.txt\n"); + f.write_file("a/gen.src", "FROMRULE\n"); + f.write_file("a/x.dat", "FROMSRC\n"); + f.mkdir("build"); + REQUIRE(f.pup({ "configure", "-B", "build" }).success()); + + WHEN("built out-of-tree") + { + REQUIRE(f.build({ "-B", "build" }).success()); + + THEN("it is consumed once, not twice") + { + auto content = f.read_file("build/a/all.txt"); + INFO("all.txt: " << content); + REQUIRE(content == "FROMRULE\n"); + } + } + } +} + +SCENARIO("An exclusion applies to generated glob matches out-of-tree", "[e2e][glob][pathspace]") +{ + GIVEN("a glob over generated files with one of them excluded") + { + auto f = E2EFixture { "glob_mixed_space" }; + f.write_file("Tupfile", + ": keep.in |> cp %f %o |> keep.gen\n" + ": skip.in |> cp %f %o |> skip.gen\n" + ": *.gen !skip.gen |> echo %f > %o |> out.txt\n"); + f.write_file("keep.in", "k\n"); + f.write_file("skip.in", "s\n"); + f.mkdir("build"); + REQUIRE(f.pup({ "configure", "-B", "build" }).success()); + + WHEN("built out-of-tree") + { + REQUIRE(f.build({ "-B", "build" }).success()); + + THEN("the excluded file is absent from %f") + { + auto content = f.read_file("build/out.txt"); + INFO("out.txt: " << content); + REQUIRE(content.find("keep.gen") != std::string::npos); + REQUIRE(content.find("skip.gen") == std::string::npos); + } + } + } +} + SCENARIO("Dropping one output of a rule removes that output", "[e2e][identity][join][stale]") { GIVEN("a built rule that declares two outputs") From 485efc6cf2ec20524580f117181bbd5dd56192c3 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 27 Jul 2026 23:25:26 +0800 Subject: [PATCH 2/2] Scope the claims to what was fixed; pin the source half of the order test From an adversarial review of this PR. The comment claimed the exclusion axis was closed. It is closed only for literal exclusions: a glob exclusion is expanded against the filesystem, so it never sees generated files and is still ignored out-of-tree, and bin members are still pushed build-prefixed into the same list. Both are pre-existing and now filed as #195. The comments here now say only what this change does. The order scenario asserted that two build directories produce identical bytes, which would still hold if the filesystem half stopped contributing entirely -- both would be "GEN" and both would be equal. It now pins the exact content, which covers the source half, the generated half and the canonical order together. The shadowing scenario asserted putup's silent shadow-wins behaviour as correct. Upstream tup rejects that project outright ("Attempting to insert 'x.dat' as a generated node when it already exists as a different type"), and putup in-tree overwrites the checked-in source file on disk. Filed as #194; the scenario is tagged [deviation] and now states that it asserts only the dedup this PR is about, not that shadowing should be legal. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA --- src/graph/builder.cpp | 10 +++------- test/unit/test_e2e.cpp | 9 ++++++++- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/graph/builder.cpp b/src/graph/builder.cpp index c16baaeb..dbae0563 100644 --- a/src/graph/builder.cpp +++ b/src/graph/builder.cpp @@ -694,17 +694,13 @@ auto expand_glob_pattern( } auto match_path_sv = pool.get(pup::strip_path_prefix(node_path_sv, build_root_name)); if (glob.matches(match_path_sv)) { - // The stripped path, not the physical one: out-of-tree the two halves would - // otherwise be in different path spaces, and everything below compares strings. - // resolve_input_node looks under BuildRoot first, so this still names the - // generated node. + // Stripped, not physical: one path space for both halves (resolve_input_node + // is BuildRoot-first, so this still names the generated node). matches.push_back(pool.intern(match_path_sv)); } } - // Both halves are now source-relative, so a generated file seen from both is one input, - // ordering does not depend on where the build tree lives, and an exclusion written - // source-relative can match. + // Sorting and deduping across two path spaces would order by where the build tree lives. std::ranges::sort(matches, {}, [&pool](StringId id) { return pool.get(id); }); matches.erase(std::unique(matches.begin(), matches.end()), matches.end()); for (auto id : matches) { diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index 355fd297..02004e77 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -4549,13 +4549,20 @@ SCENARIO("A glob's %f order does not depend on the build directory's name", "[e2 INFO("AAA: " << from_aaa); INFO("zz: " << from_zz); REQUIRE(from_aaa == from_zz); + // Pins both halves and the canonical order: equality alone would still + // hold if the filesystem half stopped contributing entirely. + REQUIRE(from_aaa == "GEN\nKEEP\n"); } } } } -SCENARIO("A generated file shadowing a source file is one glob match", "[e2e][glob][pathspace]") +SCENARIO("A generated file shadowing a source file is one glob match", "[e2e][glob][pathspace][deviation]") { + // Upstream tup rejects this project outright ("Attempting to insert 'x.dat' as a generated + // node when it already exists as a different type"). putup accepts it and lets the generated + // node win; issue #194 tracks that deviation. This asserts only the dedup #191 is about -- + // one match rather than two -- and is not a claim that shadowing should be legal. GIVEN("a generated file whose name also exists as a checked-in source") { auto f = E2EFixture { "glob_mixed_space" };