Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/fbuild-core/src/response_file.rs (2)
127-135: Run stale cleanup once per process, not on every write.
write_response_fileis invoked for every compile/link command. On large builds (hundreds to thousands of TUs) this re-scanstemp_dirand re-stats every entry on each call, plus causes redundant filesystem churn. Since the staleness window is 7 days, running this a single time per process is sufficient and avoids O(N·M) behavior.♻️ Gate with a std::sync::Once (or OnceLock) per directory
-use std::time::{Duration, SystemTime}; +use std::sync::Once; +use std::time::{Duration, SystemTime}; @@ - let _ = cleanup_stale_response_files(temp_dir, RESPONSE_FILE_STALE_AFTER, SystemTime::now()); + static CLEANUP_ONCE: Once = Once::new(); + CLEANUP_ONCE.call_once(|| { + let _ = cleanup_stale_response_files( + temp_dir, + RESPONSE_FILE_STALE_AFTER, + SystemTime::now(), + ); + });If multiple distinct
temp_dirvalues are expected per process, swapOncefor aMutex<HashSet<PathBuf>>guard keyed on the directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-core/src/response_file.rs` around lines 127 - 135, The stale-file cleanup is currently run on every call to write_response_file (calling cleanup_stale_response_files(temp_dir, RESPONSE_FILE_STALE_AFTER, SystemTime::now())), causing expensive repeated scans; change this so cleanup runs once per process (or once per distinct temp_dir) by gating the call with a one-time guard (e.g. a static std::sync::Once or a OnceLock/Mutex<HashSet<PathBuf>> keyed by temp_dir) and only invoke cleanup_stale_response_files when the guard for that directory has not yet run; leave write_response_file, RESPONSE_FILE_STALE_AFTER, and the cleanup_stale_response_files function intact, only add the one-time registration check around the cleanup invocation.
11-13: Use the new constants when constructing the response-file path.
RESPONSE_FILE_PREFIXandRESPONSE_FILE_SUFFIXwere introduced so the cleanup scanner and the writer agree on the naming scheme, but line 171 still hardcodes"fbuild_"and".rsp". A future rename would silently break cleanup.♻️ Proposed fix
- let path = temp_dir.join(format!("fbuild_{}_{}.rsp", prefix, hash)); + let path = temp_dir.join(format!( + "{}{}_{}{}", + RESPONSE_FILE_PREFIX, prefix, hash, RESPONSE_FILE_SUFFIX + ));Also applies to: 161-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-core/src/response_file.rs` around lines 11 - 13, Replace the hardcoded "fbuild_" and ".rsp" string literals used when constructing response-file paths with the new constants RESPONSE_FILE_PREFIX and RESPONSE_FILE_SUFFIX so the writer and cleanup scanner stay in sync; find the code that builds the response-file name (the block around where the path is constructed—previously using "fbuild_" and ".rsp", referenced in the diff around lines 161–171) and change it to compose the filename using RESPONSE_FILE_PREFIX and RESPONSE_FILE_SUFFIX (e.g., prefix + base + suffix) so any future rename only requires updating the constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fbuild-core/src/response_file.rs`:
- Around line 40-45: The home_dir() function currently falls back to
std::env::temp_dir() which on Windows is the unwanted %LOCALAPPDATA%\Temp;
instead modify home_dir() to avoid returning the raw temp dir by nesting the
fallback under a deterministic fbuild-owned subdirectory (e.g.
std::env::temp_dir().join("fbuild") or ".fbuild") so cleanup/ownership can be
applied; update the implementation in the home_dir function (and any docs/tests)
to return that nested PathBuf rather than the raw temp_dir(), preserving the
current signature to avoid wider API changes.
---
Nitpick comments:
In `@crates/fbuild-core/src/response_file.rs`:
- Around line 127-135: The stale-file cleanup is currently run on every call to
write_response_file (calling cleanup_stale_response_files(temp_dir,
RESPONSE_FILE_STALE_AFTER, SystemTime::now())), causing expensive repeated
scans; change this so cleanup runs once per process (or once per distinct
temp_dir) by gating the call with a one-time guard (e.g. a static
std::sync::Once or a OnceLock/Mutex<HashSet<PathBuf>> keyed by temp_dir) and
only invoke cleanup_stale_response_files when the guard for that directory has
not yet run; leave write_response_file, RESPONSE_FILE_STALE_AFTER, and the
cleanup_stale_response_files function intact, only add the one-time registration
check around the cleanup invocation.
- Around line 11-13: Replace the hardcoded "fbuild_" and ".rsp" string literals
used when constructing response-file paths with the new constants
RESPONSE_FILE_PREFIX and RESPONSE_FILE_SUFFIX so the writer and cleanup scanner
stay in sync; find the code that builds the response-file name (the block around
where the path is constructed—previously using "fbuild_" and ".rsp", referenced
in the diff around lines 161–171) and change it to compose the filename using
RESPONSE_FILE_PREFIX and RESPONSE_FILE_SUFFIX (e.g., prefix + base + suffix) so
any future rename only requires updating the constants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 786478b3-4705-41e7-a765-3621efb2ff54
📒 Files selected for processing (1)
crates/fbuild-core/src/response_file.rs
| fn home_dir() -> PathBuf { | ||
| std::env::var("HOME") | ||
| .or_else(|_| std::env::var("USERPROFILE")) | ||
| .map(PathBuf::from) | ||
| .unwrap_or_else(|_| std::env::temp_dir()) | ||
| } |
There was a problem hiding this comment.
home_dir() silently falls back to the old broken location.
When neither HOME nor USERPROFILE is set, this returns std::env::temp_dir(), which on Windows is exactly %LOCALAPPDATA%\Temp — the location issue #45 is trying to move away from. In practice Windows always sets USERPROFILE, so this is a minor/edge concern, but the fallback defeats the fix if it ever triggers. Consider either erroring out or at least nesting under a deterministic fbuild-owned subdirectory so cleanup still applies.
🛡️ Suggested change
fn home_dir() -> PathBuf {
std::env::var("HOME")
.or_else(|_| std::env::var("USERPROFILE"))
.map(PathBuf::from)
- .unwrap_or_else(|_| std::env::temp_dir())
+ .unwrap_or_else(|_| std::env::temp_dir().join("fbuild-home"))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/fbuild-core/src/response_file.rs` around lines 40 - 45, The home_dir()
function currently falls back to std::env::temp_dir() which on Windows is the
unwanted %LOCALAPPDATA%\Temp; instead modify home_dir() to avoid returning the
raw temp dir by nesting the fallback under a deterministic fbuild-owned
subdirectory (e.g. std::env::temp_dir().join("fbuild") or ".fbuild") so
cleanup/ownership can be applied; update the implementation in the home_dir
function (and any docs/tests) to return that nested PathBuf rather than the raw
temp_dir(), preserving the current signature to avoid wider API changes.
1 participant
Summary
~/.fbuild/{dev|prod}/tmp/response-filesfbuild_*.rspfiles before writing new response filesFixes #45.
Verification
cargo test -p fbuild-corecargo test -p fbuild-build --lib compiler::testscargo fmt --checkSummary by CodeRabbit
Release Notes