Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 21 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdates zccache module to canonicalize workspace paths and introduces utilities for rewriting compiler arguments and paths relative to computed compile working directories. Integrates these utilities into the compilation flow to support stable path handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
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 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.
🧹 Nitpick comments (2)
crates/fbuild-build/src/zccache.rs (1)
287-302: Prefix-ordering note — no bug, but worth a safety check as prefixes grow.Current prefixes (
-I,-isystem,-iquote,-idirafter,-include,-imacros,-isysroot) don't collide because Rust'sstrip_prefixis case-sensitive and-Iuses uppercaseIwhile the rest use lowercasei. If a future prefix is added (e.g., another uppercase-I-starting flag, or a lowercase one that is itself a prefix of another, like hypothetical-ibefore-include), the first-match iteration could mis-split. Consider either sorting by descending length or asserting no-prefix-of-another in a unit test when the list is next modified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-build/src/zccache.rs` around lines 287 - 302, split_joined_path_flag can mis-split if a shorter prefix appears before a longer one that shares the same start; update the prefix handling in split_joined_path_flag by ordering the prefix array by descending length (so longer prefixes like "-isystem" are checked before "-I") or implement a unit test that asserts no prefix is a prefix of another to prevent regressions when modifying the list; reference the prefix array in split_joined_path_flag and add the sorting or test near that function to guarantee safe matching as new prefixes are added.crates/fbuild-build/src/compiler.rs (1)
579-594: Optional: consider documenting the limited flag coverage ofnormalize_flags_for_compile_cwd.
normalize_flags_for_compile_cwdonly rewrites include-family flags and--sysroot(seeflag_takes_path_argument/split_joined_path_flaginzccache.rs). Other path-bearing compile-step flags that can appear inflags/extra_pre_flags/extra_flags— e.g.-B<prefix>,-fprofile-dir=<dir>,-fdebug-prefix-map=<old>=<new>,-specs=<path>,-Wp,-MD,<path>, or-L<dir>/-Wl,...=<path>when a link step reuses this entry point — will still be passed through verbatim and can leak absolute workspace paths into the zccache key. Not a correctness bug for the common compile path today, but worth a short doc comment at the call site (or onnormalize_flags_for_compile_cwd) so future flag additions remember to extend the allow-list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-build/src/compiler.rs` around lines 579 - 594, Call site lacks documentation that normalize_flags_for_compile_cwd only rewrites include-style flags and --sysroot; update the compiler.rs block (where all_flags is built using normalize_flags_for_compile_cwd with compile_cwd) to add a short comment explaining the limited coverage and reference the helper functions that determine this behavior (flag_takes_path_argument and split_joined_path_flag in zccache.rs), so future maintainers know other path-bearing flags (e.g. -B, -fprofile-dir, -fdebug-prefix-map, -specs, -Wp,-MD, -L, -Wl) may still contain absolute paths and must be added to the allow-list if needed. Ensure the comment sits immediately above the compile_cwd conditional (around normalize_flags_for_compile_cwd usage) and mentions compile_cwd, all_flags, and the zccache helpers by name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/fbuild-build/src/compiler.rs`:
- Around line 579-594: Call site lacks documentation that
normalize_flags_for_compile_cwd only rewrites include-style flags and --sysroot;
update the compiler.rs block (where all_flags is built using
normalize_flags_for_compile_cwd with compile_cwd) to add a short comment
explaining the limited coverage and reference the helper functions that
determine this behavior (flag_takes_path_argument and split_joined_path_flag in
zccache.rs), so future maintainers know other path-bearing flags (e.g. -B,
-fprofile-dir, -fdebug-prefix-map, -specs, -Wp,-MD, -L, -Wl) may still contain
absolute paths and must be added to the allow-list if needed. Ensure the comment
sits immediately above the compile_cwd conditional (around
normalize_flags_for_compile_cwd usage) and mentions compile_cwd, all_flags, and
the zccache helpers by name.
In `@crates/fbuild-build/src/zccache.rs`:
- Around line 287-302: split_joined_path_flag can mis-split if a shorter prefix
appears before a longer one that shares the same start; update the prefix
handling in split_joined_path_flag by ordering the prefix array by descending
length (so longer prefixes like "-isystem" are checked before "-I") or implement
a unit test that asserts no prefix is a prefix of another to prevent regressions
when modifying the list; reference the prefix array in split_joined_path_flag
and add the sorting or test near that function to guarantee safe matching as new
prefixes are added.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71a9364b-df0e-433e-a151-39a6cba55978
📒 Files selected for processing (3)
crates/fbuild-build/src/compiler.rscrates/fbuild-build/src/zccache.rscrates/fbuild-build/tests/zccache_hit_across_workspace_rename.rs
1 participant
Summary
Investigation
The failing run was https://github.com/FastLED/fbuild/actions/runs/24805459290. The macOS test failed because the runner temp path was created as
/var/..., but the child process reported its CWD as/private/var/.... The fake zccache wrapper then received absolute source/include args with the non-canonical spelling and could not relativize them against the canonical CWD, producing two different cache keys.This belongs in fbuild: once fbuild chooses a zccache CWD, it should pass zccache path arguments that are stable relative to that CWD. No upstream zccache issue filed.
Testing
uv run soldr cargo fmt --all -- --checkuv run soldr cargo check -p fbuild-builduv run soldr cargo clippy -p fbuild-build -- -D warningsuv run soldr cargo test -p fbuild-builduv run soldr cargo test -p fbuild-build --test zccache_hit_across_workspace_renameuv run soldr cargo test -p fbuild-build zccache::tests::Summary by CodeRabbit
Release Notes
Bug Fixes
Chores