Skip to content

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

Description

@zackees

TL;DR

#664 audited all 16 framework packages and confirmed the bug is universal: every package pins its URL + commit + checksum as const &str, ships only a default constructor, and every orchestrator throws away platform_packages from env_config. PlatformIO honors the consumer's platform_packages line; fbuild silently ignores it.

The audit's closing note already flagged this: "Folding the override-construction into PackageBase so adding it to a new package is a one-line constructor variant — instead of N hand-written with_overrides — is worth considering before implementation begins." This issue is that follow-up. Fix the abstract class once, get correct behavior on every platform for free.

Why hoist into PackageBase instead of patching 16 orchestrators

Per CLAUDE.md (monocrate policy + "Never add a new crate") and per the code-review skill rule about "code that belongs in core instead of platform crates", the correct shape is:

  1. Override support is a property of the package abstraction, not a property of the orchestrator. Every consumer of PackageBase has the same need; duplicating per-package with_override constructors is exactly the per-platform-bespoke wiring that turns a one-line bug fix into a 16-PR sweep, and rots the moment someone adds package ci: pin PyO3 extension glibc to 2.17 on linux-x86_64 #17.
  2. The platform_packages parser is identical for every framework. Sixteen orchestrators each grepping env_config for their framework name is a duplication smell.
  3. Cache subdir semantics already live in PackageBase. The override path (distinct cache key, optional checksum, INFO log) belongs there too — it touches the same name / version / url / cache_key / checksum fields the base already owns (see crates/fbuild-packages/src/lib.rs:172–252).

Proposed shape

1. PackageBase learns a single override path

Reference: crates/fbuild-packages/src/lib.rs:172–252 (pub struct PackageBase + new / with_cache_root).

// crates/fbuild-packages/src/lib.rs
pub struct PackageOverride {
    pub url: String,
    pub version: String,
    // None ⇒ skip checksum (consumer-trusted override). Same policy
    // discussed in #663 §3(a).
    pub checksum: Option<String>,
}

impl PackageBase {
    pub fn new(/* unchanged */) -> Self { /* default consts path */ }

    /// Apply a consumer-provided override. Replaces url/version/checksum;
    /// preserves name + cache_subdir. Cache key derives from the override
    /// URL so the override never collides with the default-pinned cache
    /// (acceptance criterion in #663).
    pub fn with_override(mut self, ovr: PackageOverride) -> Self {
        self.url = ovr.url.clone();
        self.cache_key = ovr.url;       // override URL is the new cache key
        self.version = ovr.version;
        self.checksum = ovr.checksum;
        self
    }
}

Every framework package then becomes the trivial pattern:

// e.g. arduino_core_lpc8xx.rs
pub fn new(project_dir: &Path) -> Self {
    Self { base: PackageBase::new(/* default const-pins */) }
}

pub fn with_override(project_dir: &Path, ovr: PackageOverride) -> Self {
    Self { base: PackageBase::new(/* defaults */).with_override(ovr) }
}

— one builder-style line per package, not a parallel const-pin path.

2. Shared platform_packages parser in fbuild-config

One helper, used by every orchestrator:

// crates/fbuild-config/src/platformio.rs (or new submodule)
/// Parse a `platform_packages` line and find an entry for `package_name`.
///
/// Accepts the PlatformIO syntaxes documented at
/// https://docs.platformio.org/en/latest/projectconf/sections/env/options/platform/platform_packages.html
///   name@<URL>#<sha>         → (url, sha)
///   name@<owner/repo>#<sha>  → (github-archive-url, sha)
///   name @ <version>         → returns None (version pins aren't an override)
pub fn parse_platform_packages_entry(
    line: &str,
    package_name: &str,
) -> Option<PackageOverride> { ... }

3. Shared orchestrator helper, one call per platform

Hoist the lookup into a small helper so each orchestrator's wiring is a 3-line delta, not a per-platform code path:

// crates/fbuild-build/src/package_override.rs
pub fn resolve_override(
    env_config: &EnvConfig,
    package_name: &str,
) -> Option<PackageOverride> {
    env_config
        .raw_value("platform_packages")
        .and_then(|raw| raw.lines()
            .filter_map(|line| parse_platform_packages_entry(line, package_name))
            .next())
}

Per-orchestrator change becomes:

// crates/fbuild-build/src/nxplpc/orchestrator.rs
let core = match resolve_override(&env_config, "framework-arduino-lpc8xx") {
    Some(ovr) => ArduinoCoreLpc8xx::with_override(&params.project_dir, ovr),
    None      => ArduinoCoreLpc8xx::new(&params.project_dir),
};

— that same 3-line delta repeats 16 times, but each line is identical except for the package name and the type name. A short macro could collapse it further; not required for correctness.

4. INFO log lives in PackageBase, not per-orchestrator

When with_override is applied, log once at install time:

ArduinoCore-LPC8xx OVERRIDE: <override-url> (default pinned: <default-url>)

This means consumers get the override-visibility acceptance criterion (#663) on every platform automatically — not just the one whose orchestrator the patch author remembered to update.

Why this matters for #664

The audit subtasks (#665#680) are audit tasks — they confirm the bug exists, they don't ship the fix. If implementation goes platform-by-platform we ship 16 PRs and 16 chances to miss the override visibility log or get the cache-key derivation subtly different. Hoisting into PackageBase:

Acceptance criteria

  • PackageBase::with_override(PackageOverride) exists in crates/fbuild-packages/src/lib.rs and is the only place override semantics (cache-key derivation, optional checksum, INFO log) live.
  • parse_platform_packages_entry exists once in fbuild-config and is unit-tested for the three PIO syntax shapes.
  • A shared resolve_override helper in fbuild-build is used by all 16 orchestrators with a uniform 3-line delta.
  • All 16 framework packages expose with_override(project_dir, PackageOverride) that delegates to the base. No per-package URL-parsing, no per-package cache-key derivation, no per-package logging.
  • Integration test (the LPC8xx bisection case from nxplpc: framework-arduino-lpc8xx pin is hardcoded — platform_packages override in platformio.ini is silently ignored (blocks FastLED#3325 bisection) #663) passes: platform_packages = framework-arduino-lpc8xx@<URL>#<sha> in [env:lpc845brk] causes fbuild to fetch and use the override.
  • Default behavior (no override) is unchanged — same pin, same checksum, same cache subdir — verified by existing per-package tests still passing.
  • Override cache subdir is distinct from default (no collision), verified by a PackageBase-level unit test on install_path() divergence.
  • No new crate added (per CLAUDE.md monocrate policy + crate-gate.yml). All changes land in fbuild-packages, fbuild-config, fbuild-build.

Out of scope

Refs

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Status
    Triage

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions