Resolve glob matches in one path space (fixes #191) - #193
Open
typeless wants to merge 2 commits into
Open
Conversation
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
PR metricsPerformance (gcc example, Linux)
Deterministic signals: instructions (cachegrind-simulated instruction reads — exact across runs, no PMU needed), page faults, peak RSS, and the cachegrind D1/LL miss rates. CPU time is user+sys from time(1). Internal statistics (gcc example, up-to-date dry run)
Counters from Binary size (Linux)
Test coverage (lines)
103 files · 15164/17494 lines covered Deltas vs main@fc14f89de. Updated for e1f4f60 |
…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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
expand_glob_patternmerges two sources of matches: a filesystem scan, whose paths are source-relative, and a scan ofGeneratedgraph 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, andapply_exclusions.Three symptoms, one cause
%forder depended on the name of the build directory."../AAA/a/gen.dat"sorts before"keep.dat";"../zz/a/gen.dat"sorts after:A generated file shadowing a same-named source appeared twice — the halves spelled it
a/x.datandbuild/a/x.dat, so the dedup kept both and the checked-in source was never consumed:Exclusions silently stopped applying. Normalized source-relative, compared against a build-prefixed path, so
!skip.genheld in-tree and was ignored out-of-tree.The fix
The loop already computed the build-root-stripped path to run the glob against, 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 literal exclusion comparison are correct as written.
resolve_input_nodechecksBuildRootbeforeSourceRootat source-relative paths, so the stripped spelling still names the generated node — and the node is known to exist, since these matches come from iterating theGeneratednodes themselves.PR #180 introduced this merge and a follow-up normalized the
../component lexically. That was one axis of a two-axis problem; this is the other.Tests
Three e2e scenarios, each the shape that was never covered — a glob matching a generated and a source file, out-of-tree. Each observed failing first; the shadowing case showed
FROMRULEtwice.Note the test-suite gap was narrower than I first wrote in the issue: out-of-tree coverage is extensive (265 scenarios use
-B build). What was missing was specifically a glob spanning both node kinds, which is the only shape where the two spaces meet.Also checked by hand, because 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.Verification
make test: 142404 assertions, 637 cases, 32 e2e shards.make tidy,make iwyuclean. GCC BSP parses (24 Tupfiles, 1853 commands).🤖 Generated with Claude Code
https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
Review corrections
An adversarial review found the claims here were broader than the fix, and one test asserted the wrong thing.
The exclusion axis is only half closed. A glob exclusion (
!s*.gen) is expanded against the filesystem, so it never sees generated files and is still ignored out-of-tree; in-tree it is unstable, renderingecho keep.gen skip.genon build 1 andecho keep.genon build 2. Bin{group}members are still pushed build-prefixed into the same list. Both pre-existing, both verified onmaintoo, now filed as #195. The comments in this PR no longer claim more than it does.The order test had a blind spot. Asserting only that two build directories agree would still pass if the filesystem half stopped contributing entirely — both would be
GENand both equal. It now pins"GEN\nKEEP\n", covering source half, generated half and canonical order together.The shadowing test canonicalized a divergence. Real tup rejects that project outright:
and putup in-tree overwrites the checked-in source file on disk (
FROMSRC→FROMRULE). Filed as #194. The scenario is now tagged[deviation], states that it asserts only the dedup this PR is about, and points at the issue. Consequence worth noting: #191's second symptom is only reachable in a project #194 would reject, so that half of this fix is defensive rather than user-facing.Behaviour change for existing projects. A Tupfile that worked around #191 by spelling an exclusion build-prefixed (
!build/skip.gen) silently stops excluding. The source-relative spelling is the tup-conformant one, so the break is intended, but it is a break.The review also verified independently, by execution, what I had argued: in-tree output is byte-identical to
main(including a../cross-directory glob), the 3-tree GCC BSP dry run is byte-identical over 3530 commands, every consumer of these strings resolves BuildRoot-first before anyexists()probe, an index written bymainupgrades cleanly in one re-run, and all three new tests fail when the one-line change is reverted.