Skip to content

refactor(build): extract resolve_lib_deps() to eliminate duplicated lib_deps wiring across 13 orchestrators #1294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zackees merged 2 commits into from
Aug 10, 2026
Merged

refactor(build): extract resolve_lib_deps() to eliminate duplicated lib_deps wiring across 13 orchestrators #1294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
513cbb1
refactor(build): extract resolve_lib_deps() to eliminate duplicated l…
zackees Aug 10, 2026
c49f75e
fix(build-engine): allow too_many_arguments on resolve_lib_deps
zackees Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 21 additions & 21 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

43 changes: 42 additions & 1 deletion crates/fbuild-build-arm/src/apollo3/orchestrator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,47 @@ impl BuildOrchestrator for Apollo3Orchestrator {
.extend(extra_common_flags);
augmented_config.compiler_flags.cxx.extend(extra_cpp_flags);

// 6a. Download `lib_deps` from the registry / remote URLs before
// creating the compiler, so the downloaded library include directories
// are available during compilation (FastLED/fbuild#1276).
let lib_deps = ctx.config.get_lib_deps(&params.env_name)?;
let lib_ignore = ctx
.config
.get_lib_ignore(&params.env_name)
.unwrap_or_default();
let lib_archives = if !lib_deps.is_empty() {
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,
&params.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?
Comment on lines +275 to +302

@coderabbitai coderabbitai Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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: apply ctx.build_unflags.clone() to temp_compiler.
  • crates/fbuild-build-arm/src/nrf52/orchestrator.rs#L309-L336: apply ctx.build_unflags.clone() to temp_compiler.
  • crates/fbuild-build-arm/src/nxplpc/orchestrator.rs#L318-L345: apply ctx.build_unflags.clone() to temp_compiler.
  • crates/fbuild-build-arm/src/rp2040/orchestrator.rs#L339-L366: apply ctx.build_unflags.clone() to temp_compiler.
  • crates/fbuild-build-arm/src/silabs/orchestrator.rs#L184-L211: apply ctx.build_unflags.clone() to temp_compiler.
  • crates/fbuild-build-arm/src/stm32/orchestrator/arduino_mbed.rs#L140-L167: apply ctx.build_unflags.clone() to temp_compiler.
  • crates/fbuild-build-arm/src/stm32/orchestrator/mod.rs#L364-L391: apply ctx.build_unflags.clone() to temp_compiler.
  • crates/fbuild-build-arm/src/teensy/orchestrator.rs#L269-L296: apply ctx.build_unflags.clone() to temp_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-L336
  • crates/fbuild-build-arm/src/nxplpc/orchestrator.rs#L318-L345
  • crates/fbuild-build-arm/src/rp2040/orchestrator.rs#L339-L366
  • crates/fbuild-build-arm/src/silabs/orchestrator.rs#L184-L211
  • crates/fbuild-build-arm/src/stm32/orchestrator/arduino_mbed.rs#L140-L167
  • crates/fbuild-build-arm/src/stm32/orchestrator/mod.rs#L364-L391
  • crates/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

} else {
Vec::new()
};

let compiler = ArmCompiler::new(
toolchain.get_gcc_path(),
toolchain.get_gxx_path(),
Expand Down Expand Up @@ -352,7 +393,7 @@ impl BuildOrchestrator for Apollo3Orchestrator {
ctx,
params,
&sources,
&[],
&lib_archives,
Some(&lib_env),
TargetArchitecture::Arm,
"APOLLO3",
Expand Down
47 changes: 44 additions & 3 deletions crates/fbuild-build-arm/src/nrf52/orchestrator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,14 +289,55 @@ impl BuildOrchestrator for Nrf52Orchestrator {
include_dirs.push(tinyusb_src.clone());
}
// Framework library includes (SPI, Wire, etc.)
let libs_dir = framework_dir.join("libraries");
let framework_libs_dir = framework_dir.join("libraries");
for lib_name in &["SPI", "Wire"] {
let lib_dir = libs_dir.join(lib_name);
let lib_dir = framework_libs_dir.join(lib_name);
if lib_dir.exists() {
include_dirs.push(lib_dir);
}
}

// 6a. Download `lib_deps` from the registry / remote URLs before
// creating the compiler, so the downloaded library include directories
// are available during compilation (FastLED/fbuild#1276).
let lib_deps = ctx.config.get_lib_deps(&params.env_name)?;
let lib_ignore = ctx
.config
.get_lib_ignore(&params.env_name)
.unwrap_or_default();
let lib_archives = if !lib_deps.is_empty() {
let temp_compiler = Nrf52Compiler::new(
toolchain.get_gcc_path(),
toolchain.get_gxx_path(),
&ctx.board.mcu,
&ctx.board.f_cpu,
defines.clone(),
include_dirs.clone(),
mcu_config.clone(),
params.profile,
params.verbose,
);
pipeline::resolve_lib_deps(
&lib_deps,
&lib_ignore,
&params.project_dir,
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),
compiler_cache.as_deref(),
)
.await?
} else {
Vec::new()
};

let compiler = Nrf52Compiler::new(
toolchain.get_gcc_path(),
toolchain.get_gxx_path(),
Expand Down Expand Up @@ -359,7 +400,7 @@ impl BuildOrchestrator for Nrf52Orchestrator {
ctx,
params,
&sources,
&[],
&lib_archives,
Some(&lib_env),
TargetArchitecture::Arm,
"NRF52",
Expand Down
44 changes: 43 additions & 1 deletion crates/fbuild-build-arm/src/nxplpc/orchestrator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,47 @@ impl BuildOrchestrator for NxpLpcOrchestrator {
pipeline::add_extra_library_include_dirs(&extra_library_roots, &mut include_dirs);
include_dirs.retain(|dir| !dir.as_os_str().is_empty());

// 6a. Download `lib_deps` from the registry / remote URLs before
// creating the compiler, so the downloaded library include directories
// are available during compilation (FastLED/fbuild#1276).
let lib_deps = ctx.config.get_lib_deps(&params.env_name)?;
let lib_ignore = ctx
.config
.get_lib_ignore(&params.env_name)
.unwrap_or_default();
let lib_archives = if !lib_deps.is_empty() {
let temp_compiler = ArmCompiler::new(
toolchain.get_gcc_path(),
toolchain.get_gxx_path(),
lpc_family,
&ctx.board.f_cpu,
defines.clone(),
include_dirs.clone(),
mcu_config.clone(),
params.profile,
params.verbose,
);
pipeline::resolve_lib_deps(
&lib_deps,
&lib_ignore,
&params.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?
} else {
Vec::new()
};

let compiler = ArmCompiler::new(
toolchain.get_gcc_path(),
toolchain.get_gxx_path(),
Expand Down Expand Up @@ -371,9 +412,10 @@ impl BuildOrchestrator for NxpLpcOrchestrator {
jobs: crate::parallel::effective_jobs(params.jobs),
compiler_cache: None,
};
let extra_link_inputs =
let mut extra_link_inputs =
pipeline::compile_extra_libraries(&extra_library_roots, &ctx.build_dir, &lib_env)
.await?;
extra_link_inputs.extend(lib_archives);

// 11. Run the shared sequential build pipeline.
let result = pipeline::run_sequential_build_with_libs(
Expand Down
Loading
Loading