feat(clean): add standalone framework cache cleanup#1101
Conversation
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (30)
📝 WalkthroughWalkthroughAdds ChangesClean command and documentation
Build request and cleanup pipeline
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Daemon
participant BuildPipeline
participant FrameworkCoreCache
participant ProjectOutputs
CLI->>Daemon: submit clean request
Daemon->>BuildPipeline: forward clean_only and clean_all
BuildPipeline->>FrameworkCoreCache: remove matching entry for clean all
BuildPipeline->>ProjectOutputs: delete project build directory
BuildPipeline-->>Daemon: return clean result
Daemon-->>CLI: stream operation result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
e9fb604 to
ff58314
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/fbuild-daemon/src/models.rs (1)
561-566: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd positive deserialization coverage for
clean_only: true.The current test proves only that omitted fields default to false; it does not verify the new value that activates clean-only behavior. Add a request containing
"clean_only": trueand assert that it deserializes totrue.As per path instructions: write tests as the specification and cover request-semantics contract boundaries.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-daemon/src/models.rs` around lines 561 - 566, Extend the build_request_defaults test to deserialize a request containing "clean_only": true and assert req.clean_only is true, while preserving the existing omitted-field assertions for false defaults. This should explicitly cover the positive deserialization contract for BuildRequest.clean_only.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agents/docs/commands-reference.md`:
- Line 15: Escape the pipe character in the command name within the Markdown
table row so “sketch|all” remains in a single cell and the “See more” value is
preserved. Update only the affected command-name entry in the documentation
table.
In `@crates/fbuild-build-engine/src/pipeline/sequential.rs`:
- Around line 107-122: Make clean-all fail when exact cache eviction fails
instead of logging a warning and returning success. In sequential.rs, propagate
errors from core_cache.remove() in the clean_all branch; apply the same
propagation to the ESP32 core-cache removal in build.rs and framework-library
cache removal in framework_libs.rs, preserving successful removal handling.
In `@crates/fbuild-build-esp/src/esp32/orchestrator/build.rs`:
- Around line 628-648: Gate the fast-path reuse and project-as-library
compilation paths in the build orchestration flow on !params.clean_only,
ensuring neither executes for clean-only builds. Let clean-only requests reach
the existing params.clean_only branch, including cache eviction for clean all,
without compiling sources.
---
Nitpick comments:
In `@crates/fbuild-daemon/src/models.rs`:
- Around line 561-566: Extend the build_request_defaults test to deserialize a
request containing "clean_only": true and assert req.clean_only is true, while
preserving the existing omitted-field assertions for false defaults. This should
explicitly cover the positive deserialization contract for
BuildRequest.clean_only.
🪄 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: 8c2ffec7-484f-4f9a-956b-ec373403475e
📒 Files selected for processing (30)
agents/docs/commands-reference.mdcrates/fbuild-build-engine/src/framework_core_cache.rscrates/fbuild-build-engine/src/lib.rscrates/fbuild-build-engine/src/pipeline/sequential.rscrates/fbuild-build-esp/src/esp32/orchestrator/build.rscrates/fbuild-build-esp/src/esp32/orchestrator/framework_libs.rscrates/fbuild-build/src/compile_many.rscrates/fbuild-build/tests/avr_build.rscrates/fbuild-build/tests/eh_frame_strip_esp32.rscrates/fbuild-build/tests/esp32_build.rscrates/fbuild-build/tests/nxplpc_build_flags.rscrates/fbuild-build/tests/nxplpc_core_compile_commands.rscrates/fbuild-build/tests/stm32_acceptance.rscrates/fbuild-build/tests/teensy30_acceptance.rscrates/fbuild-build/tests/teensy_build.rscrates/fbuild-build/tests/teensylc_acceptance.rscrates/fbuild-cli/src/cli/args.rscrates/fbuild-cli/src/cli/build.rscrates/fbuild-cli/src/cli/clean.rscrates/fbuild-cli/src/cli/dispatch.rscrates/fbuild-cli/src/cli/mod.rscrates/fbuild-cli/src/cli/tests.rscrates/fbuild-cli/src/daemon_client/types.rscrates/fbuild-cli/src/mcp/tools.rscrates/fbuild-daemon/src/handlers/emulator/select.rscrates/fbuild-daemon/src/handlers/emulator/tests_process.rscrates/fbuild-daemon/src/handlers/operations/build.rscrates/fbuild-daemon/src/handlers/operations/deploy.rscrates/fbuild-daemon/src/models.rsdocs/reference/cli.md
| | Command | Use this when | See more | | ||
| |---|---|---| | ||
| | `fbuild build` | You want to compile firmware for the env specified by `-e <env>` and `<project_dir>`. The default path; cache via daemon. | `fbuild help build` | | ||
| | `fbuild clean sketch|all` | You want to remove one environment/profile's project outputs, optionally including its exact reusable framework-cache entries, without compiling or deploying. | `fbuild help clean`, FastLED/fbuild#1089 | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the pipe in the command name.
sketch|all creates an extra table cell, so the “See more” value is dropped.
Proposed fix
-| `fbuild clean sketch|all` | You want to remove one environment/profile's project outputs, optionally including its exact reusable framework-cache entries, without compiling or deploying. | `fbuild help clean`, FastLED/fbuild#1089 |
+| `fbuild clean sketch\|all` | You want to remove one environment/profile's project outputs, optionally including its exact reusable framework-cache entries, without compiling or deploying. | `fbuild help clean`, FastLED/fbuild#1089 |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `fbuild clean sketch|all` | You want to remove one environment/profile's project outputs, optionally including its exact reusable framework-cache entries, without compiling or deploying. | `fbuild help clean`, FastLED/fbuild#1089 | | |
| | `fbuild clean sketch\|all` | You want to remove one environment/profile's project outputs, optionally including its exact reusable framework-cache entries, without compiling or deploying. | `fbuild help clean`, FastLED/fbuild#1089 | |
🧰 Tools
🪛 LanguageTool
[style] ~15-~15: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ld| |fbuild clean sketch|all` | You want to remove one environment/profile's projec...
(REP_WANT_TO_VB)
🪛 markdownlint-cli2 (0.23.0)
[warning] 15-15: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agents/docs/commands-reference.md` at line 15, Escape the pipe character in
the command name within the Markdown table row so “sketch|all” remains in a
single cell and the “See more” value is preserved. Update only the affected
command-name entry in the documentation table.
Source: Linters/SAST tools
| if params.clean_all { | ||
| let _g = perf.phase("core-cache-remove"); | ||
| match core_cache.remove() { | ||
| Ok(()) => tracing::info!( | ||
| "removed framework core cache key={} at {}", | ||
| core_cache.key(), | ||
| core_cache.path().display() | ||
| ), | ||
| Err(error) => tracing::warn!( | ||
| "failed to remove framework core cache key={} at {}: {}", | ||
| core_cache.key(), | ||
| core_cache.path().display(), | ||
| error | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail clean all when exact cache eviction fails. These paths only warn on removal errors and then report a successful cleanup, leaving reusable cache entries intact.
crates/fbuild-build-engine/src/pipeline/sequential.rs#L107-L122: propagatecore_cache.remove()failures instead of continuing to the successful clean-only result.crates/fbuild-build-esp/src/esp32/orchestrator/build.rs#L613-L627: propagate ESP32 core-cache removal failures.crates/fbuild-build-esp/src/esp32/orchestrator/framework_libs.rs#L105-L112: propagate ESP32 framework-library cache removal failures.
📍 Affects 3 files
crates/fbuild-build-engine/src/pipeline/sequential.rs#L107-L122(this comment)crates/fbuild-build-esp/src/esp32/orchestrator/build.rs#L613-L627crates/fbuild-build-esp/src/esp32/orchestrator/framework_libs.rs#L105-L112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-build-engine/src/pipeline/sequential.rs` around lines 107 -
122, Make clean-all fail when exact cache eviction fails instead of logging a
warning and returning success. In sequential.rs, propagate errors from
core_cache.remove() in the clean_all branch; apply the same propagation to the
ESP32 core-cache removal in build.rs and framework-library cache removal in
framework_libs.rs, preserving successful removal handling.
Summary
Validation
Closes #1089
Summary by CodeRabbit
fbuild cleancommand for removing project outputs without compiling or deploying.sketchandallcleanup scopes, with optional quick or release profiles.allscope.