fix(ldf): root framework selection at sketch#1097
Conversation
|
Warning Review limit reached
Next review available in: 4 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 (9)
📝 WalkthroughWalkthroughThe PR adds active-preprocessor branch scanning, translation-unit-based library seeding, defines-aware library resolution and caching, and MCU-specific integration for STM32 and Teensy builds. CLI diagnostics, benchmarks, regression tests, and architecture documentation are updated accordingly. ChangesActive library selection
Process documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BuildOrchestrator
participant FrameworkResolver
participant LibrarySelector
participant IncludeWalker
participant ActiveScanner
BuildOrchestrator->>FrameworkResolver: resolve framework sources with MCU defines
FrameworkResolver->>LibrarySelector: resolve_active(seeds, search_paths, defines)
LibrarySelector->>IncludeWalker: walk_with_state_active(...)
IncludeWalker->>ActiveScanner: scan_active(file, defines)
ActiveScanner-->>IncludeWalker: active includes
IncludeWalker-->>LibrarySelector: resolved graph
LibrarySelector-->>FrameworkResolver: selected source files
FrameworkResolver-->>BuildOrchestrator: framework library sources
Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
crates/fbuild-build-arm/src/teensy/orchestrator.rs (1)
188-191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid computing defines twice.
The
ldf_definescomputed here are identical to thedefinesmap computed later in the function (around line 238). Consider computing the complete map once before library selection and reusing it for the compiler configuration to reduce redundancy.🤖 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-arm/src/teensy/orchestrator.rs` around lines 188 - 191, Update the function around ldf_defines and the later compiler configuration to compute the complete defines map once, including ldf_mcu_config.defines_map(), before library selection. Reuse that same map for both library selection and compiler configuration, removing the duplicate later computation while preserving existing behavior.
🤖 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 `@crates/fbuild-build-arm/src/stm32/orchestrator/mod.rs`:
- Around line 149-152: Update the STM32 orchestration flow around ldf_defines
and the later compiler defines construction so all STM32-specific defines,
including family, USE_HAL_DRIVER, and VARIANT_H, are added before library
selection. Reuse this fully populated defines map when invoking the resolver,
eliminating the split between the initial ldf_defines setup and later defines
augmentation.
In `@crates/fbuild-build-engine/src/framework_libs.rs`:
- Around line 398-412: Update the shared seed/path-resolution logic around
collect_project_seeds and is_library_root so translation units in active local
lib directories are scanned for framework dependencies instead of being pruned
wholesale; move these corrected helpers into a public build-engine module. In
crates/fbuild-build-engine/src/framework_libs.rs#L398-L412, apply the shared
implementation; in crates/fbuild-cli/src/lib_select.rs#L317-L337, remove the
duplicate logic and import the public helpers.
In `@crates/fbuild-header-scan/src/scanner.rs`:
- Around line 217-225: Update active_source to parse logical preprocessing lines
while tracking lexical state, so # tokens inside multiline comments or raw
strings are not treated as directives and backslash-continued conditions are
evaluated as one directive. Add regression tests covering both cases, verifying
that reachable includes remain preserved.
- Around line 276-280: Replace the current macro-value handling around
split_directive, first_token, and the conditional evaluation paths with complete
or conservative tri-state expression evaluation. Preserve aliases, parenthesized
values, arithmetic, bitwise operators, and out-of-range literals without
treating unsupported valid expressions as false; ensure unknown conditions do
not incorrectly include or exclude libraries. Add regression tests covering each
case and verify scanner behavior against real preprocessing scenarios.
In `@crates/fbuild-header-scan/src/walker.rs`:
- Around line 127-133: Update walk_with_state_active and the underlying include
traversal so preprocessing macro state is carried forward in include order
instead of rescanning every file with the original defines. Ensure scan_cache
keys or invalidation account for the active macro state, and preserve branches
affected by macros introduced by earlier headers; add integration tests covering
a header-defined feature flag controlling a later conditional include.
In `@docs/architecture/library-selection.md`:
- Around line 88-95: Reconcile the seed descriptions in this document and the
referenced library-selection documentation with the rooted-at-the-sketch policy:
clarify that only project translation units seed collect_project_seeds, while
lib/ and include/ headers are discovered only through the transitive include
graph. Update or scope any wording that currently presents lib/ trees as
independent seeds so callers do not reintroduce the old root behavior.
---
Nitpick comments:
In `@crates/fbuild-build-arm/src/teensy/orchestrator.rs`:
- Around line 188-191: Update the function around ldf_defines and the later
compiler configuration to compute the complete defines map once, including
ldf_mcu_config.defines_map(), before library selection. Reuse that same map for
both library selection and compiler configuration, removing the duplicate later
computation while preserving existing behavior.
🪄 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: 75a19cad-a0d0-4440-b6bd-d5edf17510b3
📒 Files selected for processing (13)
bench/fastled-examples/src/main.rscrates/fbuild-build-arm/src/stm32/orchestrator/mod.rscrates/fbuild-build-arm/src/teensy/orchestrator.rscrates/fbuild-build-engine/src/framework_libs.rscrates/fbuild-cli/src/lib_select.rscrates/fbuild-header-scan/src/lib.rscrates/fbuild-header-scan/src/scanner.rscrates/fbuild-header-scan/src/walker.rscrates/fbuild-library-select/benches/resolve_warm.rscrates/fbuild-library-select/src/cache.rscrates/fbuild-library-select/src/lib.rsdocs/architecture/library-selection.mdtasks/lessons.md
| if is_translation_unit(entry.path()) { | ||
| seeds.push(entry.path().to_path_buf()); | ||
| } | ||
| } | ||
| } | ||
| seeds | ||
| } | ||
|
|
||
| fn is_library_root(path: &Path) -> bool { | ||
| path.file_name() | ||
| .and_then(|name| name.to_str()) | ||
| .map(|name| name.eq_ignore_ascii_case("lib")) | ||
| .unwrap_or(false) | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Local library .cpp dependencies are silently ignored (and logic is duplicated).
Both files duplicate collect_project_seeds, which completely skips lib/ roots (is_library_root). By entirely excluding the lib/ directory, translation units (e.g., .cpp files) inside local libraries are dropped from the walker seeds. If an active local library includes a framework header like <SPI.h> in its .cpp file but not in its .h file, the framework library will never be evaluated or selected by the walker, resulting in linker errors (undefined references).
Additionally, the path and seed resolution logic is exactly duplicated between the build engine and the CLI.
crates/fbuild-build-engine/src/framework_libs.rs#L398-L412: Fix the LDF logic to ensure.cppfiles of active local libraries are correctly scanned for dependencies (e.g., by seeding dynamically from discovered local libraries, or avoiding wholesale pruning oflib/translation units), and extract these path/seed helpers into a shared, public module.crates/fbuild-cli/src/lib_select.rs#L317-L337: Import the shared, corrected helpers from the build engine instead of duplicating the logic.
📍 Affects 2 files
crates/fbuild-build-engine/src/framework_libs.rs#L398-L412(this comment)crates/fbuild-cli/src/lib_select.rs#L317-L337
🤖 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/framework_libs.rs` around lines 398 - 412,
Update the shared seed/path-resolution logic around collect_project_seeds and
is_library_root so translation units in active local lib directories are scanned
for framework dependencies instead of being pruned wholesale; move these
corrected helpers into a public build-engine module. In
crates/fbuild-build-engine/src/framework_libs.rs#L398-L412, apply the shared
implementation; in crates/fbuild-cli/src/lib_select.rs#L317-L337, remove the
duplicate logic and import the public helpers.
| fn active_source(src: &str, macros: &mut HashMap<String, String>) -> String { | ||
| let mut stack = Vec::new(); | ||
| let mut active = true; | ||
| let mut output = String::with_capacity(src.len()); | ||
| for line in src.split_inclusive('\n') { | ||
| let directive = line.trim_start().strip_prefix('#').map(str::trim_start); | ||
| let mut keep = active; | ||
| if let Some(directive) = directive { | ||
| let (name, rest) = split_directive(directive); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Honor lexical state and line continuations before evaluating directives.
This treats any trimmed # line as a directive, including lines inside multiline comments/raw strings, and evaluates backslash-continued conditions one physical line at a time. Either case can suppress reachable includes. Parse logical preprocessing lines with comment/string awareness and add regressions for both cases.
As per coding guidelines, “address root causes, verify correctness with tests” and “Treat tests as the specification and test real behavior.”
🤖 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-header-scan/src/scanner.rs` around lines 217 - 225, Update
active_source to parse logical preprocessing lines while tracking lexical state,
so # tokens inside multiline comments or raw strings are not treated as
directives and backslash-continued conditions are evaluated as one directive.
Add regression tests covering both cases, verifying that reachable includes
remain preserved.
Source: Coding guidelines
| "define" if active => { | ||
| let (name, value) = split_directive(rest); | ||
| if !name.is_empty() && !name.contains('(') { | ||
| macros.insert(name.to_string(), first_token(value).to_string()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not silently evaluate unsupported valid expressions as false.
Macro aliases, parenthesized values such as #define FEATURE (1), arithmetic, bitwise operators, and out-of-range literals are mis-evaluated or ignored. This can both omit required libraries and retain inactive ones. Use complete or conservative tri-state evaluation rather than defaulting unsupported syntax to 0, with regression tests.
As per coding guidelines, “address root causes, verify correctness with tests” and “Treat tests as the specification and test real behavior.”
Also applies to: 313-320, 377-400, 430-439
🤖 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-header-scan/src/scanner.rs` around lines 276 - 280, Replace the
current macro-value handling around split_directive, first_token, and the
conditional evaluation paths with complete or conservative tri-state expression
evaluation. Preserve aliases, parenthesized values, arithmetic, bitwise
operators, and out-of-range literals without treating unsupported valid
expressions as false; ensure unknown conditions do not incorrectly include or
exclude libraries. Add regression tests covering each case and verify scanner
behavior against real preprocessing scenarios.
Source: Coding guidelines
| pub fn walk_with_state_active( | ||
| seeds: &[PathBuf], | ||
| search_paths: &[PathBuf], | ||
| defines: &HashMap<String, String>, | ||
| state: &mut WalkState, | ||
| ) -> WalkResult { | ||
| walk_with_state_scanner(seeds, search_paths, state, &|src| scan_active(src, defines)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve macro state across the include graph.
Every file is rescanned from the same initial defines, so a macro introduced by config.h cannot affect conditional includes later in its parent or another included header. scan_cache also cannot distinguish the same path under different macro states. Carry preprocessing state through include order—or conservatively retain branches whose state may depend on prior includes—and test header-defined feature flags.
As per coding guidelines, “address root causes, verify correctness with tests” and “Treat tests as the specification and test real behavior.”
🤖 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-header-scan/src/walker.rs` around lines 127 - 133, Update
walk_with_state_active and the underlying include traversal so preprocessing
macro state is carried forward in include order instead of rescanning every file
with the original defines. Ensure scan_cache keys or invalidation account for
the active macro state, and preserve branches affected by macros introduced by
earlier headers; add integration tests covering a header-defined feature flag
controlling a later conditional include.
Source: Coding guidelines
1 participant
Summary
lib/<name>/headers searchable, but never treat every header in that tree as an independent root.<FastLED.h>headers still resolve normally.fbuild lib-selectdiagnostic and invalidate the selection cache.Root cause
The resolver recursively seeded every source and header under the generated project
lib/directory. In FastLED, that made dormant platform headers independent roots, letting framework libraries select themselves even though Blink could not reach their headers.Validation
soldr cargo fmt --all -- --checksoldr cargo test -p fbuild-build-engine inactive_local_library_header_cannot_select_framework_librarysoldr cargo test -p fbuild-cli --test lib_selectsoldr cargo clippy -p fbuild-build-engine -p fbuild-cli --all-targets -- -D warningsfbuild lib-selectagainst FastLED Teensy Blink: Audio, SD, SdFat, and SerialFlash are no longer selected.Coordination
Coordinated with FastLED/FastLED#3710. This must land before the FastLED migration can prove the final linked Blink bloat reduction and close FastLED/FastLED#3696.
Refs #1094; follow-up #1098 evaluates inactive conditional branches.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation