chore(build-engine): build test fixtures from the canonical path segments - #1394
Merged
Conversation
…ents Sixth ratchet batch for #1349. Allowlist 20 -> 15, and `fbuild-build-engine` is clear. Six files of test fixtures that spelled `<project>/.fbuild/build/...` by hand: `build_info`, the two compile-database suites, `compiler_tests`, `linker`, and `compiler`. Each now assembles its paths from `FBUILD_DIR_NAME` / `BUILD_DIR_NAME`. Fixtures are worth ratcheting for a reason that is easy to miss: a hand-spelled fixture keeps passing after the layout changes. It asserts against the shape the test author typed, not the shape the code produces, so the suite goes on reporting green while describing a directory nothing writes to. That is the same failure the production sites have, minus the symptom. `compiler_tests` had more sites than the first pass surfaced — the framework cache root and three toolchain/include paths under `~/.fbuild/packages/` — all found by the lint rather than by reading. Lint crate 0.1.5 -> 0.1.6, checked against the version this branch starts from rather than against main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 11 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zackees
added a commit
that referenced
this pull request
Aug 24, 2026
Review follow-up on #1349 batch 7. Two call sites reached for `FBUILD_DIR_NAME` and then hardcoded `"build"` as the next segment — half-migrated, and worse than either whole state: it reads as done while still carrying the literal it was supposed to remove. The lint cannot catch this. It bans `.fbuild`, and `"build"` is not that string, so both sites passed a clean workspace sweep. The gap is between the two consts, not in either one. Fixed in `compile_many_two_stage.rs` (this branch) and in `compiler_tests.rs`, which shipped with the same gap in #1394 — swept for the pattern rather than fixing only the one that was reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zackees
added a commit
that referenced
this pull request
Aug 24, 2026
…1395) * chore(build): finish the `.fbuild` ratchet — baseline reaches zero Seventh and final ratchet batch for #1349. Allowlist 15 -> 3, and all three that remain are sanctioned definition sites. `fbuild-build`'s twelve files — `compile_many` plus eleven integration suites — assembled `<root>/.fbuild/build/<env>/<profile>` by hand. They now build it from `FBUILD_DIR_NAME` / `BUILD_DIR_NAME`. Deliberately a literal substitution rather than a switch to `BuildLayout::resolve()`, which would be the more principled fixture. These are `#[ignore]`d toolchain-downloading suites that cannot run on this machine, and `resolve()` also honours `FBUILD_BUILD_DIR` and collapses the `<env>` segment. Changing what the fixtures compute, in tests I cannot execute, is not a trade worth making for elegance. The literal is gone either way; moving them onto the real resolver is a separate change that should be made by someone who can run them. ## The baseline is closed 48 files spelled `.fbuild` by hand when the lint landed. All 48 now route through `fbuild_paths`. What remains: crates/fbuild-paths/src/lib.rs the source of truth crates/fbuild-core/src/path.rs where the const is declared, because fbuild-paths depends on fbuild-core and not the reverse crates/fbuild-cli/src/cli/args.rs clap stringifies `///` help into literals attributed to the `#[derive]`, so no expression edit can clear it Each carries its rationale inline. The baseline section is kept rather than deleted so the next person to reach for it can see it was closed on purpose. Lint crate 0.1.6 -> 0.1.7. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: use BUILD_DIR_NAME for the segment that follows FBUILD_DIR_NAME Review follow-up on #1349 batch 7. Two call sites reached for `FBUILD_DIR_NAME` and then hardcoded `"build"` as the next segment — half-migrated, and worse than either whole state: it reads as done while still carrying the literal it was supposed to remove. The lint cannot catch this. It bans `.fbuild`, and `"build"` is not that string, so both sites passed a clean workspace sweep. The gap is between the two consts, not in either one. Fixed in `compile_many_two_stage.rs` (this branch) and in `compiler_tests.rs`, which shipped with the same gap in #1394 — swept for the pattern rather than fixing only the one that was reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Sixth ratchet batch for #1349. Allowlist 20 → 15, and
fbuild-build-engineis clear.
Six files of test fixtures that spelled
<project>/.fbuild/build/...by hand —build_info, both compile-database suites,compiler_tests,linker, andcompiler— now assemble their paths fromFBUILD_DIR_NAME/BUILD_DIR_NAME.Why fixtures are worth ratcheting
It is easy to read these as cosmetic. They are not, and the reason is specific:
A hand-spelled fixture keeps passing after the layout changes. It asserts
against the shape the test author typed, not the shape the code produces. So
the suite goes on reporting green while describing a directory nothing writes
to — the same drift as the production sites, minus the symptom that would tell
you.
That is not hypothetical in this crate.
compile_cwd_from_outputwalks for the.fbuildcomponent, and #1392 just routed that walk through the const. If theconst changed and these fixtures did not, every one of them would still pass
while testing a layout the walker no longer recognizes.
Found by the lint, not by reading
compiler_tests.rshad four more sites than my first pass surfaced — theframework cache root and three toolchain/include paths under
~/.fbuild/packages/. My initialgrep | head -5had truncated them. The lintlisted all of them; that is the argument for removing allowlist entries and
letting the tool enumerate, rather than grepping and trusting the result.
Verification
dylint --lib ban_raw_fbuild_path -- --workspace --all-targets: 0 findings,re-run after rebasing onto the merged chore(cli): route fbuild-cli through fbuild-paths; document the clap exception #1393 rather than only before
soldr cargo test -p fbuild-build-engine --lib: 405 passed-D warnings: cleanfrom
Remaining
15 entries: fbuild-build 12, plus the 3 sanctioned sites
(
fbuild-paths/lib.rs,fbuild-core/path.rs,fbuild-cli/args.rs). One crateleft.