Skip to content

Order glob matches by path, not by interning order (fixes #171) - #174

Merged
typeless merged 1 commit into
mainfrom
fix/171-glob-order
Jul 27, 2026
Merged

Order glob matches by path, not by interning order (fixes #171)#174
typeless merged 1 commit into
mainfrom
fix/171-glob-order

Conversation

@typeless

Copy link
Copy Markdown
Owner

glob_expand sorted its Vec<StringId> results with std::ranges::sort(results). StringId is an enum class over the interning handle, and handles are issued sequentially on first intern, so that sorted by readdir order, not by path. The sort read as a canonicalization and was not one.

That order reaches cmd->inputs unchanged, %f concatenates in it, and compute_command_identity hashes the expanded text — so identical trees on two machines produced different command lines (different binaries), and command identity, the cross-build join key, varied with filesystem enumeration for every glob rule.

Sorting by the interned string matches what the identity hash already assumes, and matches tup: its glob query walks node_dir_index on (dir, name), so matches arrive in name order.

Measured

Directory enumerating (ls -U) as bravo mike zeta yankee alpha charlie, rule : d/*.txt |> echo %f > %o |> order.txt:

before: d/bravo.txt d/mike.txt d/zeta.txt d/yankee.txt d/alpha.txt d/charlie.txt
after:  d/alpha.txt d/bravo.txt d/charlie.txt d/mike.txt d/yankee.txt d/zeta.txt

Index version

15 → 16. Every glob-fed command's identity changes, so v15 identities no longer join. Rejecting them costs the rebuild those commands need anyway, and avoids a scoped build merging the stale twin back in beside the new one.

Test

The new scenario fixes interning order instead of trusting the filesystem to enumerate out of order: it interns the paths in reverse-lexicographic order before globbing, so handle order is deterministically the reverse of path order. It covers both the flat listing and the ** walk. Observed failing first:

test_glob.cpp:312: FAILED:
  REQUIRE( expanded("*.txt") == std::vector<std::string_view>{ paths[0], paths[1], paths[4] } )
with expansion:
  { "gord_zeta.txt", "gord_bravo.txt", "gord_alpha.txt" }
  ==
  { "gord_alpha.txt", "gord_bravo.txt", "gord_zeta.txt" }

make test green (142309 assertions, 616 cases, 32 e2e shards); make tidy and make iwyu clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA

glob_expand collected matches into a Vec<StringId> and finished with
std::ranges::sort(results). StringId is an enum class over the interning handle,
so that sorted by handle: the pool issues handles sequentially on first intern
and the glob interns names in read_directory order, i.e. raw readdir order. The
sort read as a canonicalization and was not one.

That order reaches cmd->inputs unchanged, %f concatenates in it, and
compute_command_identity hashes the expanded text. So the same commit produced
different link command lines -- different binaries -- on two machines whose
filesystems enumerate differently, and command identity, the cross-build join
key for expand_implicit_deps, remove_stale_outputs, merge_out_of_scope_commands
and reconcile_input_set, varied with readdir order for every glob rule. On one
machine it is a fixpoint (the index re-interns in the previous run's order),
which is why no local build-to-build churn exposed it.

Sorting by the interned string makes %f a function of project content, which is
what the identity hash already assumes, and matches tup: its glob query walks
node_dir_index on (dir, name), so matches arrive in name order.

Measured on a directory enumerating as bravo mike zeta yankee alpha charlie:

  before: d/bravo.txt d/mike.txt d/zeta.txt d/yankee.txt d/alpha.txt d/charlie.txt
  after:  d/alpha.txt d/bravo.txt d/charlie.txt d/mike.txt d/yankee.txt d/zeta.txt

INDEX_VERSION 15 -> 16: every glob-fed command's identity changes, so v15
identities no longer join. Rejecting them costs the rebuild those commands need
anyway and avoids a scoped build merging the stale twin back in beside the new
one.

The test fixes interning order rather than trusting the filesystem to enumerate
out of order: it interns the paths in reverse-lexicographic order before
globbing, so handle order is deterministically the reverse of path order.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
@github-actions

Copy link
Copy Markdown

PR metrics

Performance (gcc example, Linux)

Workload Instructions CPU time Page faults D1 miss LL miss Wall Peak RSS
parse 1073 M 0.5 s 14.1 k 0.8% 0.1% 0.517 s 30.8 MB (+0.1MB)
dry-run 1786 M (+0.1%) 0.6 s 16.6 k 0.9% 0.1% 0.617 s 40.1 MB

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)

Metric Value
Tupfiles parsed 24
Commands 3545
Commands scheduled 0
Files checked 5818
Files changed 0
Files in index 6087
Graph edges 367913
Index size (bytes) 7264970
Implicit deps 330624
Hash computations 0
Hashes skipped (stat cache) 5818
Stat calls 5869
Parse time (ms) 493.4
Total time (ms) 601.7
Runner CPU AMD EPYC 9V74 80-Core Processor

Counters from putup -n --stat on the fully-built gcc example (up-to-date dry run): deterministic work measures — a jump in commands scheduled, hash computations, or stat calls is a real behavior change, not noise. Timings are the minimum over repeated runs, compared only against a baseline from the same CPU model; the counters are the regression signal.
Timing deltas suppressed: baseline ran on different hardware (AMD EPYC 7763 64-Core Processor).

Binary size (Linux)

Binary .text .data .bss File
putup 494 KB (+0.7%) 1.7 KB 98.7 KB 582.8 KB (+0.9%)

Test coverage (lines)

Overall Median file Min file Max file
86.6% (+0.1pp) 94.9% 0.0% src/platform/path-posix.cpp 100.0% include/pup/core/arena.hpp

103 files · 14986/17313 lines covered

Deltas vs main@fa93fb08c.

Updated for a65ea19

@typeless
typeless merged commit 1738aa7 into main Jul 27, 2026
12 checks passed
typeless added a commit that referenced this pull request Jul 27, 2026
The merge in the previous commit deduped on raw strings from two different path
spaces. pup::path::join does not normalize, so for a pattern spelled ../b/*.q the
filesystem half pushed "z/../b/gen.q" while the generated half pushed the graph's
"b/gen.q". std::unique compares StringId handles, so both survived and the rule
received the same file twice:

  build 1:  cat ../b/gen.q > out.txt              (generated half only)
  build 2:  cat ../b/gen.q ../b/gen.q > out.txt   (both halves, two spellings)

With gcc %f or ld %f that is a multiple-definition failure. Normalizing the
filesystem half conforms to expand_inputs, which already normalizes the same way
one call up (builder.cpp:955).

Found by adversarial review; no fixture used a ../ or ./ glob, so CI stayed green.

INDEX_VERSION 16 -> 17: the merge changes both the membership and the order of %f
for any pattern a generated file matches, so v16 identities no longer join for
those commands. PR #174 set this rule three commits back for the same class of
change; leaving it at 16 was a silent divergence from it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
typeless added a commit that referenced this pull request Jul 27, 2026
fixes #177, fixes #178)

expand_glob_pattern expanded a pattern two ways and treated the second as a
fallback: if the filesystem glob matched anything it returned early, so one file
on disk discarded every generated match for that pattern. The generated scan
also pushed matches in nodes_of_type order -- arena order, i.e. node creation
order -- with no sort, while #174 had just made the filesystem branch
path-ordered.

Both defects are visible in one build. With putup's default in-tree layout the
generated files land beside the sources, so the branch flips between the first
and second build of an unchanged project:

  : foreach zeta.in mike.in alpha.in |> cp %f %o |> %B.gen
  : *.gen |> echo %f > %o |> matches.txt

  build 1 (clean):     zeta.gen mike.gen alpha.gen   graph branch, creation order
  build 2 (no change): alpha.gen mike.gen zeta.gen   filesystem branch, path order
                       and the command re-runs to produce it

%f is hashed by compute_command_identity, so that is command identity moving
under an unchanged project -- the #166/#167 failure class, reached through the
input set rather than the hash.

Now both sources feed one list that is sorted by path and deduped (an in-tree
build sees a generated file from both sides; it is one input). This matches tup,
which serves file and generated nodes from a single query over
node_dir_index (dir, name) -- measured: for the fixture above, tup emits
alpha.o mike.o zeta.o.

request_demand_driven_parse becomes unconditional. It ran only on the fallback
path, so with a merge the generated half of the match set would otherwise depend
on how far the parse fixpoint had progressed.

Cost: the generated-node scan now runs for every glob rather than only for
patterns that match nothing on disk. It is a linear scan of the file arena per
pattern; tup answers the same question from an index.

Both scenarios were observed failing first: matches.txt read
"zeta.gen mike.gen alpha.gen" for the ordering fixture and "kept.gen" alone
for the mixed source/generated one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
typeless added a commit that referenced this pull request Jul 27, 2026
The merge in the previous commit deduped on raw strings from two different path
spaces. pup::path::join does not normalize, so for a pattern spelled ../b/*.q the
filesystem half pushed "z/../b/gen.q" while the generated half pushed the graph's
"b/gen.q". std::unique compares StringId handles, so both survived and the rule
received the same file twice:

  build 1:  cat ../b/gen.q > out.txt              (generated half only)
  build 2:  cat ../b/gen.q ../b/gen.q > out.txt   (both halves, two spellings)

With gcc %f or ld %f that is a multiple-definition failure. Normalizing the
filesystem half conforms to expand_inputs, which already normalizes the same way
one call up (builder.cpp:955).

Found by adversarial review; no fixture used a ../ or ./ glob, so CI stayed green.

INDEX_VERSION 16 -> 17: the merge changes both the membership and the order of %f
for any pattern a generated file matches, so v16 identities no longer join for
those commands. PR #174 set this rule three commits back for the same class of
change; leaving it at 16 was a silent divergence from it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
typeless added a commit that referenced this pull request Jul 27, 2026
Three findings from an adversarial review of the enumeration-order PRs (#174,
#176, #181, #186).

compose_nested_project_subtree binary-searched `available` while push_back-ing
into it, so after the first append the range was unsorted and lower_bound's
precondition was violated: a directory already present could be reported absent
and appended again, and the final sort had no unique to remove it. A duplicate
there means sort_dirs_by_depth parses that Tupfile twice in one run, registering
its rules twice -- which now dies at the duplicate-key rejection with an error
naming a project that is perfectly valid. Which projects hit it was decided by
interning-order accidents, i.e. exactly what this PR series was removing. The
search now covers only the sorted prefix, which is sufficient because the rel_ids
within one call are distinct, and the sort dedups so the invariant no longer rests
on that being true.

PR #186 deleted <, >, <= and >= on StringId so that every comparison has to state
whether it means handle order or name order. It left two routes open, both
verified by compilation against the real header:

  auto x = (a <=> b) < 0;                                  // compiled
  struct Node { StringId name; int x;                      // compiled: member-wise
    auto operator<=>(Node const&) const = default; };      // built-in enum <=>

The second is the dangerous one: a defaulted spaceship on any struct holding a
StringId sorts by handle without a single character at the call site saying so.
Deleting operator<=> rejects both; equality is untouched. Neither route is used
anywhere in the tree today, so this closes the class before it grows a member.

prepare_job_launch emitted a job's exported variables in interning-handle order.
create_env_block (process-win32.cpp:102) writes that sequence into the environment
block verbatim, and Win32 expects it sorted alphabetically. Now emitted by name.

Not claimed: the review reported observing handle order in a child's `env` output.
That evidence is confounded -- commands run under sh, which re-exports its own
environment in its own order, so the block order putup passes is not observable
that way. The Win32 block requirement stands on its own and is why this changed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant