feat(library-selection): #205 Phase 6 acceptance gates + Phase 8 CLI/docs#208
Merged
feat(library-selection): #205 Phase 6 acceptance gates + Phase 8 CLI/docs#208
Conversation
…docs Continues #205 on top of the foundation merged in PR #207. No-op for runtime behaviour; adds gates that *prove* the foundation behaves as specified, plus a diagnostic CLI and architecture docs. ## Phase 6 — acceptance gates (#[ignore]'d, CI-only) - `crates/fbuild-build/tests/teensylc_acceptance.rs` — builds teensyLC Blink end-to-end and asserts AC#1: * `.bss` <= 3 KB * No `fnet_` / `snooze_` / `RadioHead` / `mbedtls` symbol substrings (#204 regression guard) * `setup` / `loop` symbols present * `compile_commands.json` TU count <= 250 * No FNET / Snooze / RadioHead / mbedtls entries in compile DB - `crates/fbuild-build/tests/stm32_acceptance.rs` — builds an stm32f103c8 sketch that `#include`s `<SPI.h>` and asserts AC#4: * `SPIClass` symbol present in ELF * compile_commands.json contains an SPI library entry * No manual allowlist needed (closes #202) Both gates use `fbuild_test_support::{ElfProbe, CompileDb}` from PR #207 and are gated behind `#[ignore]` because they download real toolchains and Teensyduino / STM32duino frameworks. CI runs them via `cargo test -- --ignored`. ## Phase 8 — diagnostic CLI - `fbuild lib-select <project> -e <env>` — drives the resolver in-process and prints the selected library set. - `--explain` shows per-library trigger header + unresolved includes. - `--json` emits machine-readable output (`{ selected, unresolved, included_files, seeds, framework }`). - Mutually-exclusive flags (`conflicts_with`). - New `crates/fbuild-cli/src/lib_select.rs` (~420 LoC) owns the diagnostic flow without depending on `fbuild-build` — uses `fbuild-packages::library` directly to keep the CLI a leaf binary. - 3 new tests in `crates/fbuild-cli/tests/lib_select.rs` (help-output, missing-project exit code, flag-conflict). ## Phase 8 — architecture docs - `docs/architecture/library-selection.md` — new subsystem doc covering scanner / walker / resolver, path-prefix attribution, two-pass convergence, future Phase 4 cache. - `docs/CLAUDE.md` table updated. - `docs/INDEX.md` FAQ entries added. - `docs/architecture/README.md` lists the new doc. - `tasks/lessons.md` appended with the LDF-semantics lesson. ## Verification - `uv run soldr cargo check --workspace --all-targets` — green. - `uv run soldr cargo clippy --workspace --all-targets -- -D warnings` — green (only pre-existing `clippy.toml` MSRV info note). - `uv run soldr cargo fmt --all --check` — clean. - `RUSTDOCFLAGS="-D warnings" uv run soldr cargo doc --workspace --no-deps` — green. - `uv run soldr cargo test -p fbuild-cli` — 13 passed, 0 failed (3 new lib_select + existing). - Phase 6 acceptance tests `#[ignore]`'d; CI runs them with `--ignored`. ## Out of scope (still tracked) - Phase 4 — zccache K/V memoization (gated on zackees/zccache#130). - Phase 7 — perf gates wired into `bench/fastled-examples`. - Baseline numeric capture (`tasks/baseline-205.md` placeholder). Refs: #202, #204, #205, zackees/zccache#130 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces a library-selection subsystem comprising a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as fbuild lib-select<br/>CLI Command
participant Cfg as platformio.ini<br/>& Project Resolver
participant Cache as Framework<br/>Libraries Cache
participant Scanner as Header Scanner &<br/>Include Walker
participant Resolver as Library<br/>Selection Resolver
participant Output as Result<br/>Formatter
CLI->>Cfg: Load platformio.ini,<br/>resolve environment
Cfg->>Cache: Resolve framework<br/>library directory
Cache-->>Scanner: Framework libs path
CLI->>Scanner: Discover framework<br/>libraries
Scanner-->>Resolver: Library catalog
CLI->>Scanner: Scan project<br/>src/include/lib
Scanner-->>Resolver: Header includes &<br/>seed files
Resolver->>Resolver: Resolve library<br/>selection (BFS +<br/>reconciliation)
Resolver-->>Output: Selected libraries &<br/>attribution
Output->>Output: Format output<br/>(plain/explain/json)
Output-->>CLI: Return exit code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 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: 5
🧹 Nitpick comments (8)
crates/fbuild-cli/src/lib_select.rs (4)
252-261: Dead dedupe check.
["src", "include", "lib"]are all distinct, andproject_dir.join(sub)is therefore unique on every iteration, soroots.iter().any(...)never returnstrue. The dedupe is harmless but signals "this might add the same root twice" when it can't. Drop the predicate to simplify.♻️ Suggested simplification
fn project_scan_roots(project_dir: &Path) -> Vec<PathBuf> { let mut roots = Vec::new(); for sub in ["src", "include", "lib"] { let p = project_dir.join(sub); - if p.exists() && !roots.iter().any(|r: &PathBuf| r == &p) { + if p.exists() { roots.push(p); } } roots }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-cli/src/lib_select.rs` around lines 252 - 261, The function project_scan_roots contains a redundant dedupe check: when iterating over the fixed array ["src", "include", "lib"] each project_dir.join(sub) is unique, so the roots.iter().any(|r: &PathBuf| r == &p) check is dead code; remove that predicate and simply push each existing path into roots (i.e., inside project_scan_roots iterate the subs, if p.exists() then roots.push(p)) to simplify the function and eliminate the needless comparison.
159-165:rootderived aslibraries_dir.parent()may not be the framework's actual root.
get_libraries_dir()on each*Cores/*Frameworktypically returns something like<cache>/<framework>/libraries, whoseparent()is the framework root. That works for the documented Arduino layout, but it's an implicit assumption — if any of the 12 platform helpers ever returns a libraries dir at a different depth (e.g.<cache>/<framework>/cores/<core>/librariesfor AVR alt-cores), the displayedrootwill be wrong. Sinceframework.rootis only used for display inemit_explain/emit_jsonit's not a correctness bug, just a possible misleading diagnostic. Consider asking each framework type for its root explicitly (a smallget_root()method) rather than guessing viaparent().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-cli/src/lib_select.rs` around lines 159 - 165, The current info function derives root by calling parent() on the libraries dir (function info) which can be wrong for frameworks that place libraries at different depths; add an explicit get_root() (or root_dir()) method to each framework helper type (the various *Cores/*Framework implementations that currently expose get_libraries_dir()) and change callers to use that method instead of inferring parent(), update the info function signature to accept the explicit root PathBuf (or retrieve root via the new method where info is invoked), and ensure emit_explain/emit_json use the new framework.root value for display rather than the guessed parent().
263-280:WalkDir::flatten()silently swallows I/O errors.A permission-denied or symlink-loop subtree under
src/,include/, orlib/will be silently skipped, producing a smaller seed set than expected and potentially under-resolving libraries. For a diagnostic CLI whose purpose is exactly to debug "library not found" issues, that's a footgun — the user might misinterpret a missing library as a resolver bug. Consider logging the error to stderr instead of dropping it:♻️ Suggested change
- for root in roots { - for entry in WalkDir::new(root) - .into_iter() - .filter_entry(should_scan_entry) - .flatten() - { + for root in roots { + for entry in WalkDir::new(root) + .into_iter() + .filter_entry(should_scan_entry) + { + let entry = match entry { + Ok(e) => e, + Err(err) => { + eprintln!("lib-select: walk error: {}", err); + continue; + } + }; if !entry.file_type().is_file() { continue; } if is_source_or_header(entry.path()) { seeds.push(entry.path().to_path_buf()); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-cli/src/lib_select.rs` around lines 263 - 280, collect_project_seeds uses WalkDir::new(...).into_iter().filter_entry(should_scan_entry).flatten(), and flatten() hides IO errors (e.g., permission denied or symlink loops), causing files to be silently skipped; change the iterator handling to detect and report WalkDir errors instead of swallowing them: replace the flatten() usage with explicit handling of Result<DirEntry, WalkDirError> (e.g., iterate and match each item, logging errors to stderr via eprintln! with contextual info about the root and the error) while still continuing on error, and keep the existing checks for entry.file_type().is_file() and is_source_or_header to push matching paths; refer to collect_project_seeds, WalkDir::new, and should_scan_entry to locate the code to modify.
316-326:first_reached_underis O(n × m) per library; consider canonicalizing once at call site.This helper is invoked once per selected library (in both
emit_explainandemit_json), and each call canonicalizes everyinclude_dirfrom scratch. For a project with many selected libraries that collectively share canonicalized include_dirs, that re-work is non-trivial andstd::fs::canonicalizeis a syscall. For a one-shot diagnostic this is fine; flagging only as a nice-to-have if you ever extendlib-selectinto a hot path. No action required for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-cli/src/lib_select.rs` around lines 316 - 326, The helper first_reached_under repeatedly canonicalizes lib.include_dirs on every call (used from emit_explain and emit_json), causing O(n×m) syscalls; to fix, canonicalize each library's include_dirs once before calling first_reached_under (e.g., build a Vec<PathBuf> of canonicalized dirs per FrameworkLibrary at the call site or add a new function/overload that accepts pre-canonicalized include_dirs) and then have first_reached_under take those canonicalized paths (or a new helper name) and perform the starts_with checks without calling std::fs::canonicalize repeatedly.crates/fbuild-build/tests/teensylc_acceptance.rs (3)
100-108:repo_fixturewill panic obscurely ifCARGO_MANIFEST_DIRdoesn't have two parents or fixture is missing.The two chained
.parent().unwrap()calls and the join silently rely on the workspace layout (<repo>/crates/fbuild-build). If the crate ever moves (e.g. intocrates/build/fbuild-build) or the file is run outside the workspace, the panic message ("calledOption::unwrap()on aNonevalue") will not point to the cause. Considerexpect("crate must live two levels under the repo root")and asserting the resulting path exists, so test failures self-describe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-build/tests/teensylc_acceptance.rs` around lines 100 - 108, The repo_fixture function currently chains two .parent().unwrap() calls and then joins a test path which will produce an obscure panic if the workspace layout changes or the fixture is missing; update repo_fixture to replace the .unwrap() calls with .expect(...) messages (e.g. expect("crate must live two levels under the repo root")) to document the assumption and then check that the resulting PathBuf (after join) exists, failing with a clear message like "fixture <name> not found at <path>" if it does not; refer to repo_fixture to locate and change the parent unwraps and the final path existence assertion.
110-131: Fallback walk picks the first match — could be non-deterministic across platforms.
walkdir::WalkDirtraversal order is platform-dependent (filesystem inode order on some systems). If the build produces more than onecompile_commands.json(e.g. one per sub-build under<env>/, plus a top-level merged one), the test could pick different files on different runners. The candidate-list short-circuit at lines 115–123 covers the documented layouts, so as long as those land on disk this is moot. Worth a small note here, or consider preferring the deepest or most-recently-modified file as a tiebreaker if the fallback ever fires.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-build/tests/teensylc_acceptance.rs` around lines 110 - 131, The fallback in locate_compile_commands uses walkdir::WalkDir and returns the first compile_commands.json it encounters, which can be non-deterministic; change the fallback to scan all entries from WalkDir::new(build_dir) and deterministically pick a preferred file (e.g., the deepest path or the most recently modified) instead of returning the first match — collect all entries where entry.file_name() == "compile_commands.json", then sort/compare by depth (path.components().count()) or by metadata().modified() and return the best candidate; keep the initial candidate checks (build_dir.join(env)... and build_dir.join("compile_commands.json")) as-is and only apply this deterministic selection in the WalkDir fallback.
110-131: Helper duplicated verbatim withstm32_acceptance.rs.
locate_compile_commandsis identical to the helper incrates/fbuild-build/tests/stm32_acceptance.rs. Both live incrates/fbuild-build/tests/so they can't share amoddirectly, butfbuild-test-supportis already a dev-dependency — consider lifting this (and a futurerepo_fixture) intofbuild-test-supportso additional acceptance tests don't keep copy-pasting the helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-build/tests/teensylc_acceptance.rs` around lines 110 - 131, The helper locate_compile_commands is duplicated across tests; extract it into the existing dev-dependency crate (fbuild-test-support) as a public function (pub fn locate_compile_commands) and remove the local copies; update the tests to import and call fbuild_test_support::locate_compile_commands instead, and add/ensure the dev-dependency is declared in Cargo.toml for the test crate so the shared helper is used by both tests.crates/fbuild-build/tests/stm32_acceptance.rs (1)
97-103:entries_matching("SPI")is a broad substring; consider narrowing.
"SPI"matches any path containing those three letters in sequence —SPIFFS,SPIClass,SerialSPI, even environment paths if the temp dir name happens to contain it. TheSPIClassELF check on line 87–92 is the stronger correctness guard, so this assertion is mainly a complementary signal; tightening to a path-segment match (e.g./SPI/orlibraries/SPI) would make a true negative actually fail this assertion instead of slipping past on an unrelatedSPIsubstring. Optional; not blocking AC#4.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-build/tests/stm32_acceptance.rs` around lines 97 - 103, The assertion uses entries_matching("SPI"), which matches any substring "SPI" (e.g. SPIFFS); tighten the check to a path-segment match so unrelated hits don't pass. Replace the literal "SPI" argument to entries_matching with a path-separator-delimited segment, e.g. entries_matching(&format!("{}SPI{}", std::path::MAIN_SEPARATOR, std::path::MAIN_SEPARATOR)) or a specific directory prefix like entries_matching("libraries/SPI"), so the test only counts true SPI library path entries (refer to entries_matching and the "SPI" argument).
🤖 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/tests/teensylc_acceptance.rs`:
- Around line 74-80: The tests currently fall back to
probe.has_symbol_containing(required), which is too permissive for the short
extern "C" names "setup" and "loop"; remove the substring fallback and assert
only probe.has_symbol(required).expect("symbol query") for each required symbol
(i.e. replace the assert condition that uses has_symbol_containing with a direct
has_symbol check for "setup" and "loop" in the loop that iterates over
required). Ensure the assertion message still references the required variable
correctly.
In `@crates/fbuild-cli/src/lib_select.rs`:
- Line 25: Remove the unused import of the Framework trait by deleting the line
that reads "use fbuild_packages::Framework;" in lib_select.rs; only keep the
specific framework types already used (e.g., FrameworkLibrary, AvrFramework,
Esp32Framework, Esp8266Framework) so the build no longer errors under
RUSTFLAGS="-D warnings".
- Around line 30-131: Update crates/CLAUDE.md to state that diagnostic/utility
subcommands are intentional exceptions to the "thin HTTP client" rule: add a
sentence in the fbuild-cli responsibilities section clarifying that tools like
lib-select, clang-tidy, iwyu, clang-query (and similar in-process commands such
as Mcp and Lnk) run in-process for diagnostics and are not required to be routed
via the HTTP client, while the canonical CLI workflow commands (build, deploy,
test-emu, monitor, purge) remain the primary thin-client targets; ensure you
mention lib-select by name as an example so future contributors understand when
to choose in-process vs HTTP routing.
In `@docs/architecture/library-selection.md`:
- Around line 134-143: The Phase 8 entry is inaccurate because the `fbuild
lib-select --explain` CLI (implemented in crates/fbuild-cli/src/lib_select.rs)
ships in this PR; update the docs/architecture/library-selection.md "Future
work" section to either restrict Phase 8 to only the final deletion of
framework_libs.rs (Phase 8.b) or move the CLI item (Phase 8.a) into a "What's
shipped"/"Now" section so readers aren’t misled; reference the CLI name (`fbuild
lib-select --explain`) and the helper file (`framework_libs.rs`) when making the
change.
- Around line 3-5: Update the status block at the top of library-selection.md to
reflect that Phase 6 acceptance gates (teensylc_acceptance.rs,
stm32_acceptance.rs) and Phase 8.a (fbuild lib-select CLI) are included in this
PR rather than “follow-ups”; edit the sentence that currently reads “Phase 6
acceptance gates and Phase 7 perf gates are follow-ups in `#205`” (and the earlier
Phase statuses line) to accurately list which phases landed in PR `#207`, which
are implemented in this PR (reference teensylc_acceptance.rs,
stm32_acceptance.rs, fbuild lib-select), and which items remain for follow-up,
keeping the status concise and current.
---
Nitpick comments:
In `@crates/fbuild-build/tests/stm32_acceptance.rs`:
- Around line 97-103: The assertion uses entries_matching("SPI"), which matches
any substring "SPI" (e.g. SPIFFS); tighten the check to a path-segment match so
unrelated hits don't pass. Replace the literal "SPI" argument to
entries_matching with a path-separator-delimited segment, e.g.
entries_matching(&format!("{}SPI{}", std::path::MAIN_SEPARATOR,
std::path::MAIN_SEPARATOR)) or a specific directory prefix like
entries_matching("libraries/SPI"), so the test only counts true SPI library path
entries (refer to entries_matching and the "SPI" argument).
In `@crates/fbuild-build/tests/teensylc_acceptance.rs`:
- Around line 100-108: The repo_fixture function currently chains two
.parent().unwrap() calls and then joins a test path which will produce an
obscure panic if the workspace layout changes or the fixture is missing; update
repo_fixture to replace the .unwrap() calls with .expect(...) messages (e.g.
expect("crate must live two levels under the repo root")) to document the
assumption and then check that the resulting PathBuf (after join) exists,
failing with a clear message like "fixture <name> not found at <path>" if it
does not; refer to repo_fixture to locate and change the parent unwraps and the
final path existence assertion.
- Around line 110-131: The fallback in locate_compile_commands uses
walkdir::WalkDir and returns the first compile_commands.json it encounters,
which can be non-deterministic; change the fallback to scan all entries from
WalkDir::new(build_dir) and deterministically pick a preferred file (e.g., the
deepest path or the most recently modified) instead of returning the first match
— collect all entries where entry.file_name() == "compile_commands.json", then
sort/compare by depth (path.components().count()) or by metadata().modified()
and return the best candidate; keep the initial candidate checks
(build_dir.join(env)... and build_dir.join("compile_commands.json")) as-is and
only apply this deterministic selection in the WalkDir fallback.
- Around line 110-131: The helper locate_compile_commands is duplicated across
tests; extract it into the existing dev-dependency crate (fbuild-test-support)
as a public function (pub fn locate_compile_commands) and remove the local
copies; update the tests to import and call
fbuild_test_support::locate_compile_commands instead, and add/ensure the
dev-dependency is declared in Cargo.toml for the test crate so the shared helper
is used by both tests.
In `@crates/fbuild-cli/src/lib_select.rs`:
- Around line 252-261: The function project_scan_roots contains a redundant
dedupe check: when iterating over the fixed array ["src", "include", "lib"] each
project_dir.join(sub) is unique, so the roots.iter().any(|r: &PathBuf| r == &p)
check is dead code; remove that predicate and simply push each existing path
into roots (i.e., inside project_scan_roots iterate the subs, if p.exists() then
roots.push(p)) to simplify the function and eliminate the needless comparison.
- Around line 159-165: The current info function derives root by calling
parent() on the libraries dir (function info) which can be wrong for frameworks
that place libraries at different depths; add an explicit get_root() (or
root_dir()) method to each framework helper type (the various *Cores/*Framework
implementations that currently expose get_libraries_dir()) and change callers to
use that method instead of inferring parent(), update the info function
signature to accept the explicit root PathBuf (or retrieve root via the new
method where info is invoked), and ensure emit_explain/emit_json use the new
framework.root value for display rather than the guessed parent().
- Around line 263-280: collect_project_seeds uses
WalkDir::new(...).into_iter().filter_entry(should_scan_entry).flatten(), and
flatten() hides IO errors (e.g., permission denied or symlink loops), causing
files to be silently skipped; change the iterator handling to detect and report
WalkDir errors instead of swallowing them: replace the flatten() usage with
explicit handling of Result<DirEntry, WalkDirError> (e.g., iterate and match
each item, logging errors to stderr via eprintln! with contextual info about the
root and the error) while still continuing on error, and keep the existing
checks for entry.file_type().is_file() and is_source_or_header to push matching
paths; refer to collect_project_seeds, WalkDir::new, and should_scan_entry to
locate the code to modify.
- Around line 316-326: The helper first_reached_under repeatedly canonicalizes
lib.include_dirs on every call (used from emit_explain and emit_json), causing
O(n×m) syscalls; to fix, canonicalize each library's include_dirs once before
calling first_reached_under (e.g., build a Vec<PathBuf> of canonicalized dirs
per FrameworkLibrary at the call site or add a new function/overload that
accepts pre-canonicalized include_dirs) and then have first_reached_under take
those canonicalized paths (or a new helper name) and perform the starts_with
checks without calling std::fs::canonicalize repeatedly.
🪄 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: 4fe999f2-897f-4b6c-bc2c-35bdd0b85239
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/fbuild-build/Cargo.tomlcrates/fbuild-build/tests/stm32_acceptance.rscrates/fbuild-build/tests/teensylc_acceptance.rscrates/fbuild-cli/Cargo.tomlcrates/fbuild-cli/src/lib_select.rscrates/fbuild-cli/src/main.rscrates/fbuild-cli/tests/lib_select.rsdocs/CLAUDE.mddocs/INDEX.mddocs/architecture/README.mddocs/architecture/library-selection.mdtasks/lessons.md
Comment on lines
+74
to
+80
| for required in ["setup", "loop"] { | ||
| assert!( | ||
| probe.has_symbol(required).expect("symbol query") | ||
| || probe.has_symbol_containing(required).expect("symbol query"), | ||
| "A-11: required symbol '{required}' missing from ELF" | ||
| ); | ||
| } |
There was a problem hiding this comment.
has_symbol_containing("loop") / "setup" is overly permissive.
loop and setup are 4–5-char tokens that appear inside many mangled C++ symbol names (e.g. _ZN6Stream8setupXxx..., anything with loop in its identifier). The fallback substring match almost guarantees this assertion passes even on a build where Arduino's actual setup/loop are missing. Since these are extern "C" from main.cpp / .ino, has_symbol("setup") alone should be sufficient — the substring fallback can be dropped.
♻️ Suggested fix
for required in ["setup", "loop"] {
assert!(
- probe.has_symbol(required).expect("symbol query")
- || probe.has_symbol_containing(required).expect("symbol query"),
+ probe.has_symbol(required).expect("symbol query"),
"A-11: required symbol '{required}' missing from ELF"
);
}📝 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.
Suggested change
| for required in ["setup", "loop"] { | |
| assert!( | |
| probe.has_symbol(required).expect("symbol query") | |
| || probe.has_symbol_containing(required).expect("symbol query"), | |
| "A-11: required symbol '{required}' missing from ELF" | |
| ); | |
| } | |
| for required in ["setup", "loop"] { | |
| assert!( | |
| probe.has_symbol(required).expect("symbol query"), | |
| "A-11: required symbol '{required}' missing from ELF" | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/fbuild-build/tests/teensylc_acceptance.rs` around lines 74 - 80, The
tests currently fall back to probe.has_symbol_containing(required), which is too
permissive for the short extern "C" names "setup" and "loop"; remove the
substring fallback and assert only probe.has_symbol(required).expect("symbol
query") for each required symbol (i.e. replace the assert condition that uses
has_symbol_containing with a direct has_symbol check for "setup" and "loop" in
the loop that iterates over required). Ensure the assertion message still
references the required variable correctly.
| use fbuild_library_select::{resolve, Selection}; | ||
| use fbuild_packages::library::framework_library::discover_framework_libraries; | ||
| use fbuild_packages::library::FrameworkLibrary; | ||
| use fbuild_packages::Framework; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether `Framework` is referenced in lib_select.rs (excluding the import line)
fd -t f 'lib_select.rs' crates/fbuild-cli/src | xargs rg -n '\bFramework\b' --type=rust
# Also check whether the symbol exists in fbuild-packages
rg -nP --type=rust '\bpub\s+(struct|enum|trait|type)\s+Framework\b' crates/fbuild-packagesRepository: FastLED/fbuild
Length of output: 162
Remove unused import fbuild_packages::Framework from line 25.
The Framework trait is imported but never referenced in this file (only specific subtypes like FrameworkLibrary, AvrFramework, Esp32Framework, and Esp8266Framework are used). This will fail the build with RUSTFLAGS="-D warnings".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/fbuild-cli/src/lib_select.rs` at line 25, Remove the unused import of
the Framework trait by deleting the line that reads "use
fbuild_packages::Framework;" in lib_select.rs; only keep the specific framework
types already used (e.g., FrameworkLibrary, AvrFramework, Esp32Framework,
Esp8266Framework) so the build no longer errors under RUSTFLAGS="-D warnings".
Comment on lines
+30
to
+131
| pub fn run(project_dir: &Path, env: Option<&str>, explain: bool, json: bool) -> i32 { | ||
| let project_dir = match std::fs::canonicalize(project_dir) { | ||
| Ok(p) => p, | ||
| Err(err) => { | ||
| eprintln!( | ||
| "lib-select: project '{}' not accessible: {}", | ||
| project_dir.display(), | ||
| err | ||
| ); | ||
| return 2; | ||
| } | ||
| }; | ||
|
|
||
| let ini_path = project_dir.join("platformio.ini"); | ||
| let config = match PlatformIOConfig::from_path(&ini_path) { | ||
| Ok(c) => c, | ||
| Err(err) => { | ||
| eprintln!("lib-select: failed to read {}: {}", ini_path.display(), err); | ||
| return 2; | ||
| } | ||
| }; | ||
|
|
||
| let env_name: String = match env { | ||
| Some(name) => name.to_string(), | ||
| None => match config.get_default_environment() { | ||
| Some(name) => name.to_string(), | ||
| None => { | ||
| eprintln!("lib-select: no environment given and no default in platformio.ini"); | ||
| return 2; | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| let env_cfg = match config.get_env_config(&env_name) { | ||
| Ok(cfg) => cfg, | ||
| Err(err) => { | ||
| eprintln!("lib-select: {}", err); | ||
| return 2; | ||
| } | ||
| }; | ||
|
|
||
| let platform_str = env_cfg.get("platform").cloned().unwrap_or_default(); | ||
| let board = env_cfg | ||
| .get("board") | ||
| .cloned() | ||
| .unwrap_or_else(|| "unknown".to_string()); | ||
| // We deliberately don't read `framework`: every supported `Platform` | ||
| // already implies its (single) framework, and PlatformIO's INI uses | ||
| // `framework = arduino` near-uniformly for the boards we handle. | ||
|
|
||
| let platform = match Platform::from_platform_str(&platform_str) { | ||
| Some(p) => p, | ||
| None => { | ||
| eprintln!( | ||
| "lib-select: unrecognized platform '{}' for env '{}'", | ||
| platform_str, env_name | ||
| ); | ||
| return 2; | ||
| } | ||
| }; | ||
|
|
||
| let framework_info = match resolve_framework(&project_dir, platform, &board) { | ||
| Ok(info) => info, | ||
| Err(msg) => { | ||
| eprintln!("lib-select: {}", msg); | ||
| return 2; | ||
| } | ||
| }; | ||
|
|
||
| let libraries = discover_framework_libraries(&framework_info.libraries_dir); | ||
|
|
||
| let scan_roots = project_scan_roots(&project_dir); | ||
| let seeds = collect_project_seeds(&scan_roots); | ||
|
|
||
| let selection = resolve(&seeds, &scan_roots, &libraries); | ||
|
|
||
| if json { | ||
| emit_json( | ||
| &project_dir, | ||
| &env_name, | ||
| &framework_info, | ||
| &libraries, | ||
| &seeds, | ||
| &selection, | ||
| ); | ||
| } else if explain { | ||
| emit_explain( | ||
| &project_dir, | ||
| &env_name, | ||
| &framework_info, | ||
| &libraries, | ||
| &seeds, | ||
| &selection, | ||
| ); | ||
| } else { | ||
| for name in &selection.required_libraries { | ||
| println!("{}", name); | ||
| } | ||
| } | ||
|
|
||
| 0 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find references to lib_select in the codebase
rg "lib_select|lib-select" --type rs -iRepository: FastLED/fbuild
Length of output: 85
🏁 Script executed:
# Examine the CLI entry point to understand subcommand routing
fd -t f "main.rs" crates/fbuild-cli/src/Repository: FastLED/fbuild
Length of output: 88
🏁 Script executed:
# Check the overall structure of fbuild-cli
tree -L 3 crates/fbuild-cli/src/ --filesfirstRepository: FastLED/fbuild
Length of output: 179
🏁 Script executed:
# Examine the main CLI file
cat -n crates/fbuild-cli/src/main.rsRepository: FastLED/fbuild
Length of output: 50371
🏁 Script executed:
# Check if lib_select is referenced in lib.rs or main module
rg "lib_select" --type rust -A 3 -B 3Repository: FastLED/fbuild
Length of output: 2611
🏁 Script executed:
# Check if crates/CLAUDE.md exists and what it documents about subcommands
cat -n crates/CLAUDE.md | head -100Repository: FastLED/fbuild
Length of output: 4104
🏁 Script executed:
# Look for any existing documentation about diagnostic subcommands
rg "diagnostic|in-process|exception" crates/CLAUDE.md --max-count 20Repository: FastLED/fbuild
Length of output: 40
Add documentation to crates/CLAUDE.md clarifying that diagnostic subcommands (like lib-select, clang-tidy, iwyu, clang-query) are intentional exceptions to the "thin HTTP client" guideline.
The code itself is correct: lib-select follows an established pattern where diagnostic/utility tools run in-process (ClangTidy, Iwyu, ClangQuery, Mcp, and Lnk all do the same). However, crates/CLAUDE.md lists only the core workflow subcommands (build, deploy, test-emu, monitor, purge) as the canonical CLI interface, without noting that diagnostic subcommands are an accepted exception. Update the fbuild-cli responsibilities section to clarify this distinction so future contributors understand when in-process vs. HTTP-client routing is appropriate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/fbuild-cli/src/lib_select.rs` around lines 30 - 131, Update
crates/CLAUDE.md to state that diagnostic/utility subcommands are intentional
exceptions to the "thin HTTP client" rule: add a sentence in the fbuild-cli
responsibilities section clarifying that tools like lib-select, clang-tidy,
iwyu, clang-query (and similar in-process commands such as Mcp and Lnk) run
in-process for diagnostics and are not required to be routed via the HTTP
client, while the canonical CLI workflow commands (build, deploy, test-emu,
monitor, purge) remain the primary thin-client targets; ensure you mention
lib-select by name as an example so future contributors understand when to
choose in-process vs HTTP routing.
Comment on lines
+3
to
+5
| > Status: foundation phases (0–3 + Phase 5 framework_libs delegation) landed | ||
| > in PR #207. Phase 4 (zccache memoization) tracked at zackees/zccache#130. | ||
| > Phase 6 acceptance gates and Phase 7 perf gates are follow-ups in #205. |
There was a problem hiding this comment.
Status note becomes stale on merge.
This PR introduces Phase 6 acceptance gates (teensylc_acceptance.rs, stm32_acceptance.rs) and Phase 8.a (fbuild lib-select CLI). The status block still says these are follow-ups, which contradicts the contents of this very PR. Consider updating to something like:
📝 Suggested status update
-> Status: foundation phases (0–3 + Phase 5 framework_libs delegation) landed
-> in PR `#207`. Phase 4 (zccache memoization) tracked at zackees/zccache#130.
-> Phase 6 acceptance gates and Phase 7 perf gates are follow-ups in `#205`.
+> Status: foundation phases (0–3 + Phase 5 framework_libs delegation) landed
+> in PR `#207`. Phase 6 acceptance gates and Phase 8.a `lib-select` CLI land
+> in PR `#208`. Phase 4 (zccache memoization) tracked at zackees/zccache#130.
+> Phase 7 perf gates and Phase 8.b cleanup remain follow-ups in `#205`.📝 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.
Suggested change
| > Status: foundation phases (0–3 + Phase 5 framework_libs delegation) landed | |
| > in PR #207. Phase 4 (zccache memoization) tracked at zackees/zccache#130. | |
| > Phase 6 acceptance gates and Phase 7 perf gates are follow-ups in #205. | |
| > Status: foundation phases (0–3 + Phase 5 framework_libs delegation) landed | |
| > in PR `#207`. Phase 6 acceptance gates and Phase 8.a `lib-select` CLI land | |
| > in PR `#208`. Phase 4 (zccache memoization) tracked at zackees/zccache#130. | |
| > Phase 7 perf gates and Phase 8.b cleanup remain follow-ups in `#205`. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture/library-selection.md` around lines 3 - 5, Update the status
block at the top of library-selection.md to reflect that Phase 6 acceptance
gates (teensylc_acceptance.rs, stm32_acceptance.rs) and Phase 8.a (fbuild
lib-select CLI) are included in this PR rather than “follow-ups”; edit the
sentence that currently reads “Phase 6 acceptance gates and Phase 7 perf gates
are follow-ups in `#205`” (and the earlier Phase statuses line) to accurately list
which phases landed in PR `#207`, which are implemented in this PR (reference
teensylc_acceptance.rs, stm32_acceptance.rs, fbuild lib-select), and which items
remain for follow-up, keeping the status concise and current.
Comment on lines
+134
to
+143
| ## Future work | ||
|
|
||
| - **Phase 4** — zccache K/V memoization. Gated on zackees/zccache#130 | ||
| shipping a versioned `KvStore` API and a 1.4.0 release; see | ||
| `tasks/zccache-kv-design.md`. | ||
| - **Phase 6** — wire ELF + compile-DB probes through `fbuild-test-support` | ||
| into per-board acceptance tests, gating CI on AC#1..#4 from #205. | ||
| - **Phase 7** — perf gates wired into `bench/fastled-examples`. | ||
| - **Phase 8** — `fbuild lib-select --explain` CLI subcommand and final | ||
| deletion of `framework_libs.rs` helpers. |
There was a problem hiding this comment.
Future-work entry for Phase 8 is inconsistent with this PR.
Phase 8.a (the fbuild lib-select --explain CLI) is being shipped in this PR via crates/fbuild-cli/src/lib_select.rs. Listing it under "Future work" will mislead readers. Suggest scoping the entry to Phase 8.b only (final deletion of framework_libs.rs helpers) or moving Phase 8.a to a "What's shipped" / "Now" section.
🧰 Tools
🪛 LanguageTool
[style] ~142-~142: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ired into bench/fastled-examples. - Phase 8 — fbuild lib-select --explain CLI...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture/library-selection.md` around lines 134 - 143, The Phase 8
entry is inaccurate because the `fbuild lib-select --explain` CLI (implemented
in crates/fbuild-cli/src/lib_select.rs) ships in this PR; update the
docs/architecture/library-selection.md "Future work" section to either restrict
Phase 8 to only the final deletion of framework_libs.rs (Phase 8.b) or move the
CLI item (Phase 8.a) into a "What's shipped"/"Now" section so readers aren’t
misled; reference the CLI name (`fbuild lib-select --explain`) and the helper
file (`framework_libs.rs`) when making the change.
zackees
added a commit
that referenced
this pull request
Apr 25, 2026
Five inline review findings from CodeRabbit on PR #208. All addressed. None changes runtime behaviour. ## Changes - `crates/fbuild-build/tests/teensylc_acceptance.rs` — drop substring fallback for `setup`/`loop` symbol probes. They're `extern "C"` from the sketch and exact `has_symbol(...)` is sufficient; the substring match was overly permissive (4-5-char tokens collide with mangled C++ names like `_ZN6Stream8setupXxx`). - `crates/CLAUDE.md` — add "Diagnostic subcommand exception" pattern to clarify that `clang-tidy` / `clang-query` / `iwyu` / `mcp` / `lnk` / `lib-select` intentionally bypass the daemon. Prevents future readers from concluding `lib-select` violates the "thin HTTP client" rule. - `docs/architecture/library-selection.md` — update top-of-doc status block and "Future work" list to reflect that PR #208 shipped Phase 6 acceptance gates and Phase 8.a `lib-select` CLI; only Phase 4 (zccache#130), Phase 7, and Phase 8.b cleanup remain. The "Tests" section now points at the actual `tests/*_acceptance.rs` files rather than describing them as future work. ## CodeRabbit comment 2 — false positive (no change) CodeRabbit flagged `use fbuild_packages::Framework;` in `crates/fbuild-cli/src/lib_select.rs:25` as unused. Verified: the `Framework` trait is required for method-call dispatch on the 12+ framework objects the file constructs (`get_libraries_dir()` is a trait method). Removing the import produces 12× E0599 errors. Kept. Refs: #205, #208 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Continues #205 on top of the foundation merged in #207. No-op for runtime behaviour; adds gates that prove the foundation behaves as specified, plus a diagnostic CLI and architecture docs.
Summary
Phase 6 — acceptance gates (`#[ignore]`'d, CI-only)
Both use `fbuild_test_support::{ElfProbe, CompileDb}` from #207. CI runs them via `cargo test -- --ignored`.
Phase 8.a — `fbuild lib-select` diagnostic CLI
Phase 8.b — architecture docs
Verification
Test plan
Follow-ups (still tracked, not in this PR)
Refs: #202, #204, #205, zackees/zccache#130
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
fbuild lib-selectdiagnostic command to resolve and display required libraries with plain, detailed (--explain), and JSON (--json) output options.Tests
Documentation