refactor(build): extract resolve_lib_deps() to eliminate duplicated lib_deps wiring across 13 orchestrators - #1294
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds shared ChangesLibrary dependency integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Orchestrator
participant TemporaryCompiler
participant LibraryResolver
participant SequentialBuild
Orchestrator->>TemporaryCompiler: derive compiler flags
Orchestrator->>LibraryResolver: resolve lib_deps and lib_ignore
LibraryResolver-->>Orchestrator: include paths and archives
Orchestrator->>SequentialBuild: compile and link with resolved archives
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
zackees
force-pushed
the
fix/1292-rollout-ensure-lib-deps
branch
from
August 10, 2026 00:27
aca222a to
bc7537a
Compare
…ib_deps wiring across 13 orchestrators Each orchestrator had a ~55-line copy-paste block that read lib_deps / lib_ignore from config, built a temp compiler for c/cxx flags, picked the archiver, called ensure_lib_deps(), extended include_dirs, and passed archives to run_sequential_build_with_libs. The only variation between them was the temp compiler constructor and how they accessed c/cpp flags. Extract the invariant body into pipeline::resolve_lib_deps() in the library module. Each orchestrator now creates a temp compiler (the one varying part), then delegates to resolve_lib_deps() for everything else. Fixes #1292 (roll-out of ensure_lib_deps across all remaining orchestrators, with the duplication refactored into a single site). Co-Authored-By: Claude <noreply@anthropic.com>
Matches the same annotation on the sibling ensure_lib_deps function. Co-Authored-By: Claude <noreply@anthropic.com>
zackees
force-pushed
the
fix/1292-rollout-ensure-lib-deps
branch
from
August 10, 2026 01:04
9d5b8e8 to
c49f75e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/apollo3/orchestrator.rs`:
- Around line 275-302: Temporary compilers passed to resolve_lib_deps do not
apply the project’s build_unflags before their flags are read. In
crates/fbuild-build-arm/src/apollo3/orchestrator.rs:275-302,
crates/fbuild-build-arm/src/nrf52/orchestrator.rs:309-336,
crates/fbuild-build-arm/src/nxplpc/orchestrator.rs:318-345,
crates/fbuild-build-arm/src/rp2040/orchestrator.rs:339-366,
crates/fbuild-build-arm/src/silabs/orchestrator.rs:184-211,
crates/fbuild-build-arm/src/stm32/orchestrator/arduino_mbed.rs:140-167,
crates/fbuild-build-arm/src/stm32/orchestrator/mod.rs:364-391, and
crates/fbuild-build-arm/src/teensy/orchestrator.rs:269-296, apply
ctx.build_unflags.clone() to each temp_compiler before calling Compiler::c_flags
and cpp_flags. Add a failing regression build using tempfile, real filesystem
behavior, a temporary project, and a real dependency fixture.
In `@crates/fbuild-build-engine/src/pipeline/library.rs`:
- Around line 398-437: Add behavioral tests for the public resolve_lib_deps
function using tempfile-backed real filesystem setup: cover empty lib_deps
returning no archives without unnecessary work, and an integration path that
confirms returned include directories are used during compilation and returned
archives during linking. Follow the existing test conventions and create the
failing tests before implementation changes, without replacing filesystem
behavior with mocks.
🪄 Autofix
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 Plus
Run ID: 350307a6-2474-4c15-8fcc-a4094ba2b599
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
crates/fbuild-build-arm/src/apollo3/orchestrator.rscrates/fbuild-build-arm/src/nrf52/orchestrator.rscrates/fbuild-build-arm/src/nxplpc/orchestrator.rscrates/fbuild-build-arm/src/renesas/orchestrator.rscrates/fbuild-build-arm/src/rp2040/orchestrator.rscrates/fbuild-build-arm/src/sam/orchestrator.rscrates/fbuild-build-arm/src/silabs/orchestrator.rscrates/fbuild-build-arm/src/stm32/orchestrator/arduino_mbed.rscrates/fbuild-build-arm/src/stm32/orchestrator/mod.rscrates/fbuild-build-arm/src/teensy/orchestrator.rscrates/fbuild-build-engine/src/pipeline/library.rscrates/fbuild-build-engine/src/pipeline/mod.rscrates/fbuild-build-esp/src/esp8266/orchestrator.rscrates/fbuild-build-mcu/src/avr/orchestrator.rscrates/fbuild-build-mcu/src/ch32v/orchestrator.rs
Comment on lines
+275
to
+302
| let temp_compiler = ArmCompiler::new( | ||
| toolchain.get_gcc_path(), | ||
| toolchain.get_gxx_path(), | ||
| &ctx.board.mcu, | ||
| &ctx.board.f_cpu, | ||
| defines.clone(), | ||
| include_dirs.clone(), | ||
| augmented_config.clone(), | ||
| params.profile, | ||
| params.verbose, | ||
| ); | ||
| pipeline::resolve_lib_deps( | ||
| &lib_deps, | ||
| &lib_ignore, | ||
| ¶ms.project_dir, | ||
| &ctx.build_dir, | ||
| &toolchain.get_gcc_path(), | ||
| &toolchain.get_gxx_path(), | ||
| &toolchain.get_ar_path(), | ||
| &toolchain.get_gcc_ar_path(), | ||
| &crate::compiler::Compiler::c_flags(&temp_compiler), | ||
| &crate::compiler::Compiler::cpp_flags(&temp_compiler), | ||
| &mut include_dirs, | ||
| params.verbose, | ||
| crate::parallel::effective_jobs(params.jobs), | ||
| None, | ||
| ) | ||
| .await? |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply build_unflags to every temporary compiler.
Each final compiler applies with_build_unflags, but each temporary compiler used by resolve_lib_deps does not. A configured build_unflags value can therefore make a dependency archive compile with flags that the project build removes. This can cause dependency compilation failures or incompatible compile settings.
Apply the same unflag configuration before reading temporary compiler flags. Add a failing regression build that uses a temporary project and a real dependency fixture.
crates/fbuild-build-arm/src/apollo3/orchestrator.rs#L275-L302: applyctx.build_unflags.clone()totemp_compiler.crates/fbuild-build-arm/src/nrf52/orchestrator.rs#L309-L336: applyctx.build_unflags.clone()totemp_compiler.crates/fbuild-build-arm/src/nxplpc/orchestrator.rs#L318-L345: applyctx.build_unflags.clone()totemp_compiler.crates/fbuild-build-arm/src/rp2040/orchestrator.rs#L339-L366: applyctx.build_unflags.clone()totemp_compiler.crates/fbuild-build-arm/src/silabs/orchestrator.rs#L184-L211: applyctx.build_unflags.clone()totemp_compiler.crates/fbuild-build-arm/src/stm32/orchestrator/arduino_mbed.rs#L140-L167: applyctx.build_unflags.clone()totemp_compiler.crates/fbuild-build-arm/src/stm32/orchestrator/mod.rs#L364-L391: applyctx.build_unflags.clone()totemp_compiler.crates/fbuild-build-arm/src/teensy/orchestrator.rs#L269-L296: applyctx.build_unflags.clone()totemp_compiler.
As per coding guidelines, “Follow TDD” and “Use tempfile and real filesystem behavior for filesystem tests rather than mocks.”
📍 Affects 8 files
crates/fbuild-build-arm/src/apollo3/orchestrator.rs#L275-L302(this comment)crates/fbuild-build-arm/src/nrf52/orchestrator.rs#L309-L336crates/fbuild-build-arm/src/nxplpc/orchestrator.rs#L318-L345crates/fbuild-build-arm/src/rp2040/orchestrator.rs#L339-L366crates/fbuild-build-arm/src/silabs/orchestrator.rs#L184-L211crates/fbuild-build-arm/src/stm32/orchestrator/arduino_mbed.rs#L140-L167crates/fbuild-build-arm/src/stm32/orchestrator/mod.rs#L364-L391crates/fbuild-build-arm/src/teensy/orchestrator.rs#L269-L296
🤖 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/apollo3/orchestrator.rs` around lines 275 - 302,
Temporary compilers passed to resolve_lib_deps do not apply the project’s
build_unflags before their flags are read. In
crates/fbuild-build-arm/src/apollo3/orchestrator.rs:275-302,
crates/fbuild-build-arm/src/nrf52/orchestrator.rs:309-336,
crates/fbuild-build-arm/src/nxplpc/orchestrator.rs:318-345,
crates/fbuild-build-arm/src/rp2040/orchestrator.rs:339-366,
crates/fbuild-build-arm/src/silabs/orchestrator.rs:184-211,
crates/fbuild-build-arm/src/stm32/orchestrator/arduino_mbed.rs:140-167,
crates/fbuild-build-arm/src/stm32/orchestrator/mod.rs:364-391, and
crates/fbuild-build-arm/src/teensy/orchestrator.rs:269-296, apply
ctx.build_unflags.clone() to each temp_compiler before calling Compiler::c_flags
and cpp_flags. Add a failing regression build using tempfile, real filesystem
behavior, a temporary project, and a real dependency fixture.
Source: Coding guidelines
Comment on lines
+398
to
+437
| pub async fn resolve_lib_deps( | ||
| lib_deps: &[String], | ||
| lib_ignore: &[String], | ||
| project_dir: &Path, | ||
| build_dir: &Path, | ||
| gcc_path: &Path, | ||
| gxx_path: &Path, | ||
| ar_path: &Path, | ||
| gcc_ar_path: &Path, | ||
| c_flags: &[String], | ||
| cpp_flags: &[String], | ||
| include_dirs: &mut Vec<PathBuf>, | ||
| verbose: bool, | ||
| jobs: usize, | ||
| compiler_cache: Option<&Path>, | ||
| ) -> Result<Vec<PathBuf>> { | ||
| if lib_deps.is_empty() { | ||
| return Ok(Vec::new()); | ||
| } | ||
| let dep_lib_ar_path = pick_archiver(ar_path, gcc_ar_path, c_flags, cpp_flags); | ||
| let libs_dir = build_dir.join("libs"); | ||
| let (lib_include_dirs, archives) = ensure_lib_deps( | ||
| lib_deps, | ||
| lib_ignore, | ||
| gcc_path, | ||
| gxx_path, | ||
| dep_lib_ar_path, | ||
| c_flags, | ||
| cpp_flags, | ||
| include_dirs, | ||
| project_dir, | ||
| &libs_dir, | ||
| verbose, | ||
| jobs, | ||
| compiler_cache, | ||
| ) | ||
| .await?; | ||
| include_dirs.extend(lib_include_dirs); | ||
| Ok(archives) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add behavioral tests for resolve_lib_deps.
This public helper changes the compiler and linker input contract. This change adds no test for that contract. Add a tempfile-backed test for the empty dependency path. Add an integration test that verifies include directories reach compilation and archives reach linking.
As per coding guidelines, “Follow TDD: write failing tests first” and “Use tempfile and real filesystem behavior for filesystem tests rather than mocks.”
🤖 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/library.rs` around lines 398 - 437,
Add behavioral tests for the public resolve_lib_deps function using
tempfile-backed real filesystem setup: cover empty lib_deps returning no
archives without unnecessary work, and an integration path that confirms
returned include directories are used during compilation and returned archives
during linking. Follow the existing test conventions and create the failing
tests before implementation changes, without replacing filesystem behavior with
mocks.
Source: Coding guidelines
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.
Summary
Rolls out
ensure_lib_deps()to all 11 remaining build orchestrators (fixes #1292), then refactors the duplicated inline block into a singlepipeline::resolve_lib_deps()function.Before
Each orchestrator had a ~55-line copy-paste block:
After
The invariant body is extracted into
pipeline::resolve_lib_deps():Files changed
pipeline/library.rsresolve_lib_deps()(~50 lines)pipeline/mod.rs55→22 linesNet: +593/−60 across 16 files. ~440 lines of duplicated code eliminated.
Orchestrators wired
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes