chore(paths): settle where .fbuild is declared, and ratchet five more sites - #1385
Conversation
…re sites Second batch for #1349. Allowlist 43 -> 39 legacy sites, and the tail of the ratchet is now unblocked. ## Where the const has to live `fbuild-core` had two raw-literal sites and no way to fix them: `fbuild-paths` owns `FBUILD_DIR_NAME`, but `fbuild-paths` *depends on* `fbuild-core`, so the call sites could not reach it. Left alone, those two would have been permanent exceptions to a ratchet whose stated goal is zero. The declaration moves to `fbuild_core::path::FBUILD_DIR_NAME`, next to `compile_cwd_from_output`, which walks the tree looking for that exact segment. `fbuild-paths` re-exports it, so `fbuild_paths::FBUILD_DIR_NAME` is still the name every other crate reaches for and no consumer changes. ## A second home directory, disagreeing with the first `response_file` resolved the home directory itself, preferring `HOME` over `USERPROFILE`. The neutral facade prefers `USERPROFILE` on Windows — and the difference is exactly the case this module exists for. Under MSYS2 / Git Bash `HOME` is a POSIX path like `/c/Users/you`, which native Windows GCC cannot open, so response files landed where no compiler could read them. That is the failure `windows_temp_dir` was written to avoid, reintroduced by resolving the home directory a second way. It now goes through `platform::host::home_dir()`. The test harness does not inherit Git Bash's `HOME`, so the divergence was invisible until the test set it; on the old code it fails with `left: "/c/Users/msys"`, `right: "C:\Users\native"`. ## Also ratcheted - `fbuild-library`'s `object_hash_key` duplicated the "find the `.fbuild` ancestor" walk; it now compares against the same const. - `fbuild-build-arm`'s linker-CWD test built its fixture paths by hand. - `fbuild-library-select`'s Teensy diagnostic rebuilt `~/.fbuild/prod/cache` literally — hardcoding `prod` made it look in the wrong tree under `FBUILD_DEV_MODE=1` and ignore `FBUILD_CACHE_DIR` entirely. It asks `get_cache_root()` now. `crates/fbuild-core/src/path.rs` joins `fbuild-paths/src/lib.rs` as a *sanctioned* allowlist entry rather than baseline debt: the const has to be spelled somewhere, and that is the somewhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 59 minutes Limit 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. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
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 |
1 participant
Second ratchet batch for #1349. Allowlist 43 → 39 legacy sites, and the
tail of the ratchet is now unblocked.
Where the const has to live
I flagged this on #1349 after batch one:
fbuild-corehad two raw-literalsites and no way to fix them.
fbuild-pathsownedFBUILD_DIR_NAME, butfbuild-pathsdepends onfbuild-core, so those call sites could not reachit. Left alone they would have been permanent exceptions to a ratchet whose
stated goal is zero.
The declaration moves to
fbuild_core::path::FBUILD_DIR_NAME, next tocompile_cwd_from_output, which walks the tree looking for that exact segment.fbuild-pathsre-exports it, sofbuild_paths::FBUILD_DIR_NAMEis still thename every other crate reaches for and no consumer changes.
crates/fbuild-core/src/path.rstherefore joinsfbuild-paths/src/lib.rsas asanctioned entry rather than baseline debt — the const has to be spelled
somewhere, and that is the somewhere. (43 → 38 removed, +1 sanctioned = 39.)
A second home directory, disagreeing with the first
response_fileresolved the home directory itself, preferringHOMEoverUSERPROFILE. The neutral facade prefersUSERPROFILEon Windows — and thedifference is exactly the case this module exists for. Its own docstring:
Under Git Bash
HOMEis a POSIX path like/c/Users/you, which native WindowsGCC cannot open either. So response files landed where no compiler could read
them — the failure
windows_temp_direxists to avoid, reintroduced byresolving the home directory a second way.
The test harness does not inherit Git Bash's
HOME, which is why this wasinvisible: my first attempt at a test passed against the buggy code. Setting
the var explicitly makes it fail on the old code:
Also ratcheted
fbuild-library'sobject_hash_keyduplicated the "find the.fbuildancestor" walk; it compares against the same const now.
fbuild-build-arm's linker-CWD test built its fixture paths by hand.fbuild-library-select's Teensy diagnostic rebuilt~/.fbuild/prod/cacheliterally — the hardcoded
prodmade it look in the wrong tree underFBUILD_DEV_MODE=1and ignoreFBUILD_CACHE_DIRentirely. It asksget_cache_root()now.Verification
dylint --lib ban_raw_fbuild_pathover the five affected crates: exit 0with the entries removed. It fired first on
fbuild-core/src/path.rs, whichis how the sanctioned-vs-baseline distinction above got made rather than
assumed.
soldr cargo teston fbuild-core / fbuild-library / fbuild-build-arm /fbuild-paths: 794 passed, 0 failed.
-D warnings: clean.Remaining
39 entries: fbuild-build 12, fbuild-build-engine 10, fbuild-cli 9,
fbuild-daemon 6, plus the 2 sanctioned definition sites.