Skip to content

Commit a28ead6

Browse files
zackeesclaude
andauthored
docs(library-selection): apply CodeRabbit follow-ups from PR #208 (#209)
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>
1 parent 42b732b commit a28ead6

3 files changed

Lines changed: 14 additions & 11 deletions

File tree

crates/CLAUDE.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ fbuild-test-support (test utilities) ──────────────
4848

4949
**HTTP API boundary:** CLI sends JSON requests to daemon over HTTP. Build output streams via WebSocket. Serial monitor data streams via `/ws/serial-monitor`. All endpoints match the Python FastAPI daemon's contract.
5050

51+
**Diagnostic subcommand exception:** A small, growing set of `fbuild-cli` subcommands (`clang-tidy`, `clang-query`, `iwyu`, `mcp`, `lnk`, `lib-select`) run in-process and intentionally bypass the daemon. They are read-only diagnostics that don't need build orchestration, so a round-trip through the HTTP API would only add latency. The "thin HTTP client" rule still applies to every command that touches the build pipeline (`build`, `deploy`, `monitor`, `test-emu`, etc.).
52+
5153
**PyO3 consumer contract:** FastLED imports `SerialMonitor` as a Python context manager with `read_lines()`, `write()`, `write_json_rpc()`. The `fbuild-python` crate must preserve this API exactly.
5254

5355
## Current Status

crates/fbuild-build/tests/teensylc_acceptance.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,7 @@ fn teensylc_blink_meets_205_acceptance_criteria() {
7373
}
7474
for required in ["setup", "loop"] {
7575
assert!(
76-
probe.has_symbol(required).expect("symbol query")
77-
|| probe.has_symbol_containing(required).expect("symbol query"),
76+
probe.has_symbol(required).expect("symbol query"),
7877
"A-11: required symbol '{required}' missing from ELF"
7978
);
8079
}

docs/architecture/library-selection.md

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
# Library selection (LDF-style)
22

33
> Status: foundation phases (0–3 + Phase 5 framework_libs delegation) landed
4-
> in PR #207. Phase 4 (zccache memoization) tracked at zackees/zccache#130.
5-
> Phase 6 acceptance gates and Phase 7 perf gates are follow-ups in #205.
4+
> in PR #207. Phase 6 acceptance gates and Phase 8.a `lib-select` CLI landed
5+
> in PR #208. Phase 4 (zccache memoization) tracked at zackees/zccache#130.
6+
> Phase 7 perf gates and Phase 8.b cleanup remain follow-ups in `#205`.
67
78
## Why
89

@@ -127,20 +128,21 @@ keys safe.
127128
- Resolver tests in `crates/fbuild-library-select/src/lib.rs` including
128129
the #204 regression guard, sort-stability, missing-include-dir
129130
tolerance, and same-basename-different-library disambiguation.
130-
- Acceptance tests for AC#1 (teensyLC), AC#4 (stm32 SPI auto-discovery)
131-
land in Phase 6 via `fbuild-test-support`'s `MiniFramework`,
132-
`ElfProbe`, and `CompileDb` utilities.
131+
- Acceptance gates for AC#1 (teensyLC) and AC#4 (stm32 SPI
132+
auto-discovery) live in `crates/fbuild-build/tests/`
133+
(`teensylc_acceptance.rs`, `stm32_acceptance.rs`). They are
134+
`#[ignore]`'d by default and run by CI with `--ignored`. They use
135+
`fbuild-test-support`'s `ElfProbe` and `CompileDb` utilities to probe
136+
the produced firmware.
133137

134138
## Future work
135139

136140
- **Phase 4** — zccache K/V memoization. Gated on zackees/zccache#130
137141
shipping a versioned `KvStore` API and a 1.4.0 release; see
138142
`tasks/zccache-kv-design.md`.
139-
- **Phase 6** — wire ELF + compile-DB probes through `fbuild-test-support`
140-
into per-board acceptance tests, gating CI on AC#1..#4 from #205.
141143
- **Phase 7** — perf gates wired into `bench/fastled-examples`.
142-
- **Phase 8**`fbuild lib-select --explain` CLI subcommand and final
143-
deletion of `framework_libs.rs` helpers.
144+
- **Phase 8.b**final deletion of any dead helpers in `framework_libs.rs`
145+
once Phase 4 cache lands.
144146

145147
## References
146148

0 commit comments

Comments
 (0)