Skip to content

feat(packages): hoist platform_packages override into PackageBase (foundation + LPC8xx, closes #681)#683

Merged
zackees merged 1 commit into
mainfrom
feat/681-package-override
Jun 20, 2026
Merged

feat(packages): hoist platform_packages override into PackageBase (foundation + LPC8xx, closes #681)#683
zackees merged 1 commit into
mainfrom
feat/681-package-override

Conversation

@zackees

@zackees zackees commented Jun 20, 2026

Copy link
Copy Markdown
Member

Summary

Foundation + LPC8xx wiring for #681 (audit follow-up to #664, originally reported in #663). Hoists platform_packages override semantics into the PackageBase abstract class so the same fix automatically propagates to every framework orchestrator instead of being hand-wired 16 times.

Why the abstract-class approach

#664's audit confirmed the bug is universal (16/16 framework packages). Doing this as 16 parallel patches gives 16 chances to miss the override-visibility log, get the cache-key derivation subtly different, or forget multi-line tolerance. Hoisting once means future framework packages inherit override support by construction — see crates/CLAUDE.md monocrate policy and the code-review skill's "code that belongs in core instead of platform crates" rule.

Test plan

  • package_override_tests::override_changes_install_path — the test the issue calls out by name. Asserts that the default pin, an override at commit A, and an override at commit B (same upstream repo, different SHAs) all hash to distinct cache directories. Without this, a bisection step that swaps the URL would silently reuse the previous commit's vendored sources.
  • package_override_tests::override_replaces_url_cache_key_version_and_checksum — pins the cache_key swap directly so a future refactor can't accidentally preserve the default cache_key while updating only url.
  • platform_packages::tests — 7 parser cases (URL+sha, owner/repo+sha, version-only-returns-None, multi-line first-match, trailing comma/semicolon tolerance, non-matching name, blank lines).
  • package_override::tests — 5 helper cases (env-with-match, missing-key, no-matching-name, multi-line first-match, version-pin-returns-None).
  • soldr cargo clippy -p fbuild-config -p fbuild-packages -p fbuild-build --all-targets -- -D warnings — clean.
  • soldr cargo test -p fbuild-config -p fbuild-packages -p fbuild-build --lib — 728 + 176 + 453 = 1357 tests, 0 failed.

Drive-by

Stabilized managed_zccache::tests::install_lock_blocks_second_caller_until_released. Its 40ms wall-clock deadline was racing std::thread::spawn scheduling latency under parallel-test contention — if the waiter wasn't scheduled before drop(first), it would acquire instantly and the test would fail with waited ≈ 0ms. New shape uses JoinHandle::is_finished() to prove contention directly, with a soft non-zero elapsed sanity check.

Follow-up (not in this PR)

The remaining 15 framework orchestrators (apollo3, arduino_api, avr, attiny, ch32v, esp32, esp8266, nrf52, renesas, rp2040, sam, samd, silabs, stm32, teensy, arduino_mbed) follow the same uniform 3-line delta pattern against the shared resolve_override helper and a per-package with_override constructor. That's a mechanical sweep PR with the abstraction now in place.

Closes #681
Refs #663, #664, FastLED/FastLED#3325

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jun 20, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@zackees, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 25 minutes and 2 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 67d3528b-ba99-4667-8450-ce558bd40fb7

📥 Commits

Reviewing files that changed from the base of the PR and between cd998e0 and 7408bda.

📒 Files selected for processing (10)
  • crates/fbuild-build/src/lib.rs
  • crates/fbuild-build/src/managed_zccache.rs
  • crates/fbuild-build/src/nxplpc/mod.rs
  • crates/fbuild-build/src/nxplpc/orchestrator.rs
  • crates/fbuild-build/src/nxplpc/platform_packages.rs
  • crates/fbuild-build/src/package_override.rs
  • crates/fbuild-config/src/lib.rs
  • crates/fbuild-config/src/platform_packages.rs
  • crates/fbuild-packages/src/lib.rs
  • crates/fbuild-packages/src/library/arduino_core_lpc8xx.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/681-package-override

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Foundation + LPC8xx wiring for #681. The remaining 15
framework orchestrators are a mechanical sweep follow-up — every call
site is now a 3-line delta against the shared resolve_override helper
plus a per-package with_override constructor that delegates to
PackageBase::with_override.

What this PR adds:

- fbuild-config: PackageOverride { url, version, checksum } and
  parse_platform_packages_entry / parse_platform_packages_value. Accepts
  the PlatformIO syntaxes name@<URL>#<sha> and name@<owner/repo>#<sha>;
  registry version pins (name @ 1.2.3) correctly return None.
- fbuild-packages: PackageBase::with_override(self, ovr) replaces url,
  cache_key (← override URL), version, checksum. cache_subdir + name are
  preserved. Emits one INFO log per override so it's visible in build
  scrollback across every framework package, without each orchestrator
  having to remember to log it.
- fbuild-build: package_override::resolve_override(env_config, name)
  scans the multi-line platform_packages value and returns the first
  matching override.
- ArduinoCoreLpc8xx::with_override delegates to PackageBase. nxplpc
  orchestrator consults resolve_override and switches construction.

Unit tests prove the contract the issue calls out by name:

- override_changes_install_path — default pin, override-1, and
  override-2 (same upstream repo, different commit SHAs) all hash to
  distinct cache directories. Without that guarantee, a bisection step
  that swapped the URL would silently reuse the previous commit's
  vendored sources, defeating the override.
- override_replaces_url_cache_key_version_and_checksum — pins the
  cache_key swap directly so a future refactor can't accidentally
  preserve the default cache_key while updating only url.
- parse_platform_packages_entry: URL+sha, owner/repo+sha,
  version-only-returns-None, multi-line first-match, trailing
  comma/semicolon tolerance.
- resolve_override: env-with-match, missing-key, no-matching-name,
  multi-line first-match, version-pin-returns-None.

Drive-by: stabilize managed_zccache::tests::install_lock_blocks_
second_caller_until_released. The 40ms wall-clock deadline was racing
std::thread::spawn scheduling latency under parallel-test contention —
if the waiter wasn't scheduled before drop(first), it would acquire
instantly and report waited ≈ 0ms. New shape uses JoinHandle::
is_finished() to prove contention directly, with a soft "non-zero
elapsed" sanity check.

The remaining 15 orchestrators (apollo3, arduino_api, avr, attiny,
ch32v, esp32, esp8266, nrf52, renesas, rp2040, sam, samd, silabs,
stm32, teensy, arduino_mbed) follow the same 3-line delta pattern and
will be a sweep PR.

Closes #681
Refs #663, #664, FastLED/FastLED#3325

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zackees
zackees force-pushed the feat/681-package-override branch from 8dc0e29 to 7408bda Compare June 20, 2026 11:54
@zackees
zackees merged commit d2ddffe into main Jun 20, 2026
84 of 90 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(packages): hoist platform_packages override into PackageBase so the fix propagates to all 16 orchestrators (impl follow-up to #664)

1 participant