build: centralize warm-build fast-path contract#196
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 46 minutes and 1 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 (9)
📝 WalkthroughWalkthroughThis PR centralizes fast-path warm-build cache management across seven orchestrators (AVR, ESP32, NRF52, Renesas, RP2040, SAM, Teensy) by introducing a shared Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/fbuild-build/src/renesas/orchestrator.rs (1)
62-71: Optional: theexpected_fast_path_artifactshelper and its test are duplicated across orchestrators.
expected_fast_path_artifactsdiffers only in the firmware filename (firmware.binhere,firmware.hexin Teensy, etc.), andtest_expected_fast_path_artifacts_follow_compile_db_location(lines 402-418) appears to be identical between orchestrators. Given the PR's objective to centralize the fast-path contract, this helper could move intobuild_fingerprint::fast_path(e.g. taking the firmware filename(s) as a parameter) to eliminate the per-platform copies and the redundant unit test.Not blocking; pointed out because it's adjacent to the centralization work in this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-build/src/renesas/orchestrator.rs` around lines 62 - 71, The helper expected_fast_path_artifacts and its duplicate test should be centralized: move the logic into build_fingerprint::fast_path as a function that accepts the firmware filename(s) (e.g., "firmware.bin"/"firmware.hex") plus build_dir and project_dir, and return the same tuple (elf, firmware, compile_db) using CompileDatabase::expected_output_path; then update the orchestrator-specific expected_fast_path_artifacts references to call the new build_fingerprint::fast_path with the appropriate filename(s) and remove the duplicated unit test test_expected_fast_path_artifacts_follow_compile_db_location from per-orchestrator modules, keeping a single shared test alongside the new helper.
🤖 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-build/src/build_fingerprint/fast_path.rs`:
- Around line 136-149: add_default_watches currently skips adding the "dep_libs"
root when fast_path_watch returns None, causing missing-root state to be lost;
update add_watch_root (or fast_path_watch) so it always records a watch entry
even when the directory is absent: call fast_path_watch(cache_name,
&self.build_dir, root) and if it returns None, push a placeholder watch that
encodes "missing root" (e.g. Watch::missing_root or similar) into self.watches
instead of skipping; this preserves the root in add_default_watches for
"project" and "dep_libs" and lets the hash/check layer represent the absent-root
state.
In `@crates/fbuild-build/src/esp32/orchestrator.rs`:
- Around line 1256-1264: The call to
crate::build_fingerprint::persist_fast_path_success with FastPathPersistInputs
(using metadata_hash, link_result.size_info, params.watch_set_cache,
compiler_cache) must only run after the entire build (link/convert and any final
artifact writes) has completed successfully; move or gate this call so it
executes only on the successful build path (i.e., after the final link/convert
result is confirmed OK and no errors were returned), rather than unconditionally
where it currently runs.
- Around line 262-275: The fast-path contract currently built by
FastPathContract::for_project_outputs is missing the ESP32 side-output
boot_app0.bin, so add the corresponding fast_app0 (or similarly named artifact
token you already use elsewhere) into the list passed to
FastPathContract::for_project_outputs alongside fast_elf, fast_bin, fast_boot,
fast_parts, and fast_compile_db; ensure the symbol you add matches the variable
used when the cold path copies boot_app0.bin (so the contract and the copy refer
to the same artifact name) so warm fast-path hits will require/own boot_app0.bin
as well.
---
Nitpick comments:
In `@crates/fbuild-build/src/renesas/orchestrator.rs`:
- Around line 62-71: The helper expected_fast_path_artifacts and its duplicate
test should be centralized: move the logic into build_fingerprint::fast_path as
a function that accepts the firmware filename(s) (e.g.,
"firmware.bin"/"firmware.hex") plus build_dir and project_dir, and return the
same tuple (elf, firmware, compile_db) using
CompileDatabase::expected_output_path; then update the orchestrator-specific
expected_fast_path_artifacts references to call the new
build_fingerprint::fast_path with the appropriate filename(s) and remove the
duplicated unit test
test_expected_fast_path_artifacts_follow_compile_db_location from
per-orchestrator modules, keeping a single shared test alongside the new helper.
🪄 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: 5b0167f6-d234-43ca-b981-7870374083d3
📒 Files selected for processing (10)
crates/fbuild-build/src/avr/orchestrator.rscrates/fbuild-build/src/build_fingerprint/README.mdcrates/fbuild-build/src/build_fingerprint/fast_path.rscrates/fbuild-build/src/build_fingerprint/mod.rscrates/fbuild-build/src/esp32/orchestrator.rscrates/fbuild-build/src/nrf52/orchestrator.rscrates/fbuild-build/src/renesas/orchestrator.rscrates/fbuild-build/src/rp2040/orchestrator.rscrates/fbuild-build/src/sam/orchestrator.rscrates/fbuild-build/src/teensy/orchestrator.rs
|
Addressed the review points in Changes:
Validation:
|
1 participant
Summary
Testing
Closes #157.
Summary by CodeRabbit