nxplpc: retire #576's board-JSON build_flags workaround (concrete sub-issue from #574)

[zackees]

Edit history: This body was rewritten on 2026-06-14. The first version of this issue claimed the build_flags propagation gap was already fixed — that claim was wrong. PR #576's note that "build_flags are not propagated by the nxplpc orchestrator today" is still accurate. The body below reflects the corrected scope. See the comment thread for the original-vs-corrected diff.

Goal

Close the [env:*] build_flags propagation gap in the nxplpc orchestrator's library compile path, then retire the board-JSON workaround #576 installed for lpc845brk (the only LPC variant carrying the stopgap defines today).

Background — what's actually wired vs. broken

The body of #574 (the unified namespaced routing design) called out a concrete bug:

currently nxplpc orchestrator does NOT propagate platformio.ini build_flags — surfaced when LPC845 needed -DRELEASE / -DFASTLED_DISABLE_DBG to disable FASTLED_FORCE_DBG to fit a JSON-RPC sketch in 64 KB; had to edit the fbuild-config board JSON instead

PR #576 worked around it by baking those defines into lpc845brk.json's extra_flags field — described in #576 as "a stopgap for the missing build_flags propagation".

Code audit pinpoints the gap precisely:

Path	State
BuildContext::new reads config.get_build_flags(env_name) into ctx.user_flags	✅ works (pipeline/context.rs:155)
Sketch compile chains ctx.user_flags into the overlay	✅ works (pipeline/sequential.rs:57-67)
Library compile applies ctx.user_flags to lib_env's c_flags/cpp_flags	❌ broken — nxplpc/orchestrator.rs:247-257 constructs LibraryBuildEnv with raw compiler.c_flags() / compiler.cpp_flags(), no overlay folded in

That third row is exactly why PR #576 had to put -DRELEASE=1 -DFASTLED_DISABLE_DBG=1 into the board JSON: the defines need to reach FastLED's library compile (where fl/log/log.h lives) to disable FASTLED_FORCE_DBG. The extra_flags field IS applied to libraries; user build_flags are not.

The fix template — what ESP32 does

crates/fbuild-build/src/esp32/orchestrator/build.rs:423-425:

let p_c_flags   = apply_overlay_flags(&p_compiler.c_flags(),   &src_overlay, "dummy.c");
let p_cpp_flags = apply_overlay_flags(&p_compiler.cpp_flags(), &src_overlay, "dummy.cpp");
// ... then ...
let lib_env = crate::pipeline::LibraryBuildEnv {
    c_flags:   &p_c_flags,
    cpp_flags: &p_cpp_flags,
    // ...
};

src_overlay is the combined overlay (incl. ctx.user_flags); apply_overlay_flags folds those into the per-source flag set that ends up on every library compile invocation.

crates/fbuild-build/src/nxplpc/orchestrator.rs:247-257 (current — broken):

let lib_env = pipeline::LibraryBuildEnv {
    c_flags:   &c_flags,        // ← raw compiler flags, no user_flags
    cpp_flags: &cpp_flags,      // ← same
    // ...
};

c_flags / cpp_flags here come from compiler.c_flags() / compiler.cpp_flags() directly. The same gap exists in avr/orchestrator.rs:262.

Scope

1. Add user_flags to LPC library compile. Port the ESP32 pattern: build a src_overlay over lib_env's flag inputs, fold ctx.user_flags in via apply_overlay_flags, store the result in fresh Vec<String> locals, point LibraryBuildEnv::c_flags / cpp_flags at those.
2. Add a regression-proof test fixture. tests/platform/lpc845_build_flags/ with [env:lpc845brk] build_flags = -DFROM_PLATFORMIO_INI=1 and a library source that #errors if the define is missing. One Rust integration test asserts the build succeeds.
3. THEN drop the stopgap from lpc845brk.json. Remove -DRELEASE=1 -DFASTLED_DISABLE_DBG=1 from the extra_flags field. The chip-identification defines (-DLPC845 -DCPU_LPC845M301JBD48 -D__LPC845__ -DARDUINO_LPC845BRK) stay. Note: only lpc845brk.json carries the stopgap; lpc845.json, lpcxpresso804.json, lpcxpresso845max.json do not.
4. Move those two defines into the consuming sketch's platformio.ini so they continue to apply where they're wanted.
5. Post a follow-up comment on PR feat(nxplpc): find_core_root + LPC845 RELEASE defines + pin bump to ACLPC#29 #576 noting the workaround was retired here.

Out of scope

• The broader fbuild: design unified namespaced routing for platform/framework/library artifacts driven by platformio.ini [env:*] #574 design (unified env-namespaced routing across all platforms) — stays in fbuild: design unified namespaced routing for platform/framework/library artifacts driven by platformio.ini [env:*] #574.
• AVR has the identical bug at avr/orchestrator.rs:262. Track separately if it isn't covered by this PR's scope.

Acceptance

• LPC library compile picks up [env:*] build_flags defines — proven by the lpc845_build_flags fixture's #error not firing.
• grep -E '\-DRELEASE|FASTLED_DISABLE_DBG' crates/fbuild-config/assets/boards/json/lpc845brk.json returns no matches.
• LPC845-BRK JSON-RPC sketch still links + boots end-to-end with the defines sourced from its own platformio.ini.
• PR feat(nxplpc): find_core_root + LPC845 RELEASE defines + pin bump to ACLPC#29 #576 has a follow-up comment noting the workaround was retired here.

Refs

• #574 — broader namespaced routing design (parent scope)
• #576 — the original workaround
• #586 — LPC845-BRK burn-down meta (this issue is part of section 2)
• Code paths the fix has to touch:
  • crates/fbuild-build/src/nxplpc/orchestrator.rs:247-257 (current LibraryBuildEnv construction — broken)
  • crates/fbuild-build/src/esp32/orchestrator/build.rs:423-425 (correct pattern to mirror)
  • crates/fbuild-build/src/pipeline/context.rs:155 (where user_flags are populated — already correct)
  • crates/fbuild-build/src/pipeline/sequential.rs:57-67 (where the sketch compile already gets user_flags — also already correct)

[zackees]

Correction — my prior audit was wrong

The body of this issue (and the entry at #586 (comment) announcing it) claimed the build_flags propagation gap was already fixed in nxplpc. It is not. I re-checked the code paths directly and PR #576's explicit note — "build_flags are not propagated by the nxplpc orchestrator today" — is still accurate.

What I got right vs. wrong

	Reality
BuildContext::new populates user_flags from config.get_build_flags()	✅ correct (pipeline/context.rs:155)
user_flags reach the sketch compile via pipeline/sequential.rs:57-67 (chained into user_overlay.common)	✅ correct
user_flags reach the library compile via nxplpc/orchestrator.rs:247 constructing LibraryBuildEnv	❌ wrong — that struct gets c_flags: &compiler.c_flags() raw, with no user_flags overlay applied

The library compile path is exactly where -DRELEASE=1 -DFASTLED_DISABLE_DBG=1 need to land for FastLED's fl/log/log.h (and the rest of the library) to actually disable FASTLED_FORCE_DBG. Until the LPC library compile mirrors the ESP32 fix at esp32/orchestrator/build.rs:423-425, those defines only fire in the sketch translation unit — which is why PR #576 had to bake them into lpc845brk.json's extra_flags instead.

The real fix template — what ESP32 does

crates/fbuild-build/src/esp32/orchestrator/build.rs:423-425:

let p_c_flags = apply_overlay_flags(&p_compiler.c_flags(), &src_overlay, "dummy.c");
let p_cpp_flags = apply_overlay_flags(&p_compiler.cpp_flags(), &src_overlay, "dummy.cpp");
// ...
let lib_env = crate::pipeline::LibraryBuildEnv {
    c_flags: &p_c_flags,
    cpp_flags: &p_cpp_flags,
    // ...
};

src_overlay here is the combined overlay (incl. ctx.user_flags); apply_overlay_flags folds those into the per-source flag set that ends up on every library compile invocation.

crates/fbuild-build/src/nxplpc/orchestrator.rs:247-257 (current — broken):

let lib_env = pipeline::LibraryBuildEnv {
    c_flags: &c_flags,        // ← raw compiler flags, no user_flags
    cpp_flags: &cpp_flags,    // ← same
    // ...
};

c_flags / cpp_flags here come from compiler.c_flags() / compiler.cpp_flags() directly — ctx.user_flags are never folded in. Same bug exists in avr/orchestrator.rs:262.

Revised scope for this issue

1. Add user_flags to LPC library compile — port the ESP32 pattern: build a src_overlay over the lib_env's c_flags / cpp_flags, fold ctx.user_flags in via apply_overlay_flags, store the result in fresh Vec<String> locals, and point LibraryBuildEnv::c_flags / cpp_flags at those.
2. Verify with a test fixture: a tiny tests/platform/lpc845_build_flags/ project where [env:lpc845brk] build_flags = -DFROM_PLATFORMIO_INI=1 and a library source #errors if the define isn't set. Rust integration test asserts the build succeeds — this is the regression-proof for the bug.
3. Only THEN drop the stopgap from lpc845brk.json — remove -DRELEASE=1 -DFASTLED_DISABLE_DBG=1 from the extra_flags field. Note: it's only lpc845brk.json that has these defines today; lpc845.json, lpcxpresso804.json, lpcxpresso845max.json carry just chip-identification defines.
4. Move the LPC845-BRK bring-up defines into the consuming sketch's platformio.ini so they continue to apply where they're actually wanted.
5. Optional same-fix for AVR — avr/orchestrator.rs:262 has the identical pattern; track separately if not in scope.

Updated acceptance

• LPC library compile picks up [env:*] build_flags defines — proven by the lpc845_build_flags fixture's #error not firing
• grep -E '\-DRELEASE|FASTLED_DISABLE_DBG' crates/fbuild-config/assets/boards/json/lpc845brk.json returns no matches
• LPC845-BRK JSON-RPC sketch still links + boots end-to-end with the defines sourced from its own platformio.ini
• PR feat(nxplpc): find_core_root + LPC845 RELEASE defines + pin bump to ACLPC#29 #576 has a follow-up comment noting the workaround was retired here

What stays the same

The link to #574 (broader env-namespaced routing design) and #586 (LPC burn-down meta) are unchanged. The cleanup phase (steps 3-4 above) is the same intent the prior body described — the difference is that step 1 is also required, not optional. The empirical sign-off (LPC845-BRK JSON-RPC bringup still working with the defines moved out of board JSON) is the same end-state.

Sorry for the noise — the previous body misled. The fix is a 5-10 line change in the orchestrator plus the test fixture, modeled directly on the ESP32 pattern.