diff --git a/crates/fbuild-build/src/lib.rs b/crates/fbuild-build/src/lib.rs index 20a2203a..48d07772 100644 --- a/crates/fbuild-build/src/lib.rs +++ b/crates/fbuild-build/src/lib.rs @@ -25,6 +25,7 @@ pub mod linker; pub mod managed_zccache; pub mod nrf52; pub mod nxplpc; +pub mod package_override; pub mod parallel; pub mod perf_log; pub mod pipeline; diff --git a/crates/fbuild-build/src/managed_zccache.rs b/crates/fbuild-build/src/managed_zccache.rs index abc30168..01a81a49 100644 --- a/crates/fbuild-build/src/managed_zccache.rs +++ b/crates/fbuild-build/src/managed_zccache.rs @@ -678,6 +678,20 @@ mod tests { #[test] fn install_lock_blocks_second_caller_until_released() { + // What this test proves: holding the install lock makes a contending + // acquire wait until the first holder releases. + // + // Earlier shape — `std::thread::sleep(50ms); drop(first); assert waited >= 40ms` — + // was flaky under parallel-test contention: `std::thread::spawn` doesn't + // guarantee the spawned thread is scheduled before the main thread's sleep + // ends. If the waiter wasn't scheduled until after `drop(first)`, it would + // acquire instantly and report `waited ≈ 0ms`, failing the deadline. + // + // New shape: use `JoinHandle::is_finished` to prove the waiter is *still* + // blocked while we hold the lock — that's the actual invariant we care + // about. The wall-clock elapsed assertion becomes a much softer "waiter + // observed at least one polling sleep," which doesn't race with scheduler + // latency. let tmp = tempfile::tempdir().unwrap(); let lock_dir = tmp.path().join("zccache.lock"); let first = acquire_install_lock_at( @@ -700,13 +714,28 @@ mod tests { started.elapsed() }); - std::thread::sleep(Duration::from_millis(50)); + // Give the waiter a generous window to start polling. 100ms is far + // longer than any reasonable scheduler latency, so on a healthy run + // the waiter is definitely inside `acquire_install_lock_at` by now. + std::thread::sleep(Duration::from_millis(100)); + + // Load-bearing assertion: the waiter MUST still be blocked while we + // hold the lock. If it isn't, the lock isn't actually contended. + assert!( + !waiter.is_finished(), + "second lock acquire completed while the first lock was still held — \ + the install lock is not blocking contended callers" + ); + drop(first); let waited = waiter.join().expect("waiter thread"); + // Soft sanity check: at least some elapsed time. Don't pin a tight + // deadline — scheduler jitter on parallel test runs can hide between + // `Instant::now()` and the first `create_dir` call. assert!( - waited >= Duration::from_millis(40), - "second lock should block behind the first, waited {waited:?}" + waited > Duration::from_millis(0), + "waiter should have measured a non-zero acquire duration, got {waited:?}" ); } diff --git a/crates/fbuild-build/src/nxplpc/mod.rs b/crates/fbuild-build/src/nxplpc/mod.rs index aeaa7d15..696478ae 100644 --- a/crates/fbuild-build/src/nxplpc/mod.rs +++ b/crates/fbuild-build/src/nxplpc/mod.rs @@ -14,7 +14,11 @@ pub mod mcu_config; pub mod orchestrator; -pub mod platform_packages; +// `platform_packages` lookup is now shared at the workspace level +// (FastLED/fbuild#681) — see `crate::package_override`. The per-platform +// parser introduced in #663 has been folded into +// `fbuild_config::platform_packages` so every orchestrator gets the same +// parser without duplication. use std::path::Path; diff --git a/crates/fbuild-build/src/nxplpc/orchestrator.rs b/crates/fbuild-build/src/nxplpc/orchestrator.rs index 23c659f5..61dd1671 100644 --- a/crates/fbuild-build/src/nxplpc/orchestrator.rs +++ b/crates/fbuild-build/src/nxplpc/orchestrator.rs @@ -119,29 +119,28 @@ impl BuildOrchestrator for NxpLpcOrchestrator { // framework owns `main()`, startup + vector table, wiring, // HardwareSerial, SPI, and the device headers. // - // Consumer override (FastLED/fbuild#663): when `platformio.ini` - // carries `platform_packages = framework-arduino-lpc8xx@`, - // fetch and use that commit instead of the const-pinned default - // — needed for bisection workflows like FastLED/FastLED#3325. - let env_config = ctx.config.get_env_config(¶ms.env_name)?; - let core_override = - super::platform_packages::lookup_override(env_config, "framework-arduino-lpc8xx"); - let core = match &core_override { + // Honor `platform_packages = framework-arduino-lpc8xx@#` + // from the env section (FastLED/fbuild#663, #681): if set, the + // override URL replaces the const-pinned default and gets its own + // cache subdir via `PackageBase::with_override`. The parser + // + resolver are shared across every framework orchestrator so + // nxplpc carries no platform-specific platform_packages logic. + let core_override = ctx + .config + .get_env_config(¶ms.env_name) + .ok() + .and_then(|env| { + crate::package_override::resolve_override(env, "framework-arduino-lpc8xx") + }); + let core = match core_override { Some(ovr) => { - let short = &ovr.git_ref[..7.min(ovr.git_ref.len())]; - let version = format!("override+g{}", short); let banner = format!( "ArduinoCore-LPC8xx OVERRIDE: {} (default pinned: {})", - ovr.archive_url, + ovr.url, fbuild_packages::library::ArduinoCoreLpc8xx::commit() ); - tracing::info!("{}", banner); ctx.build_log.push(banner); - fbuild_packages::library::ArduinoCoreLpc8xx::with_override( - ¶ms.project_dir, - &ovr.archive_url, - &version, - ) + fbuild_packages::library::ArduinoCoreLpc8xx::with_override(¶ms.project_dir, ovr) } None => fbuild_packages::library::ArduinoCoreLpc8xx::new(¶ms.project_dir), }; diff --git a/crates/fbuild-build/src/nxplpc/platform_packages.rs b/crates/fbuild-build/src/nxplpc/platform_packages.rs deleted file mode 100644 index 8c87c141..00000000 --- a/crates/fbuild-build/src/nxplpc/platform_packages.rs +++ /dev/null @@ -1,259 +0,0 @@ -//! Parser for PlatformIO `platform_packages` framework overrides -//! (FastLED/fbuild#663). -//! -//! `platform_packages` is a multi-line PIO config option whose value lines -//! look like `@`. For framework packages, the consumer is -//! pinning an upstream source so fbuild can fetch the override commit -//! instead of the const-pinned default. PIO honors several spec forms; -//! the ones relevant here all resolve to a downloadable GitHub archive -//! plus the ref the consumer pinned: -//! -//! - `/#` — GitHub shorthand. -//! - `https://github.com//.git#` — git URL. -//! - `https://.../.tar.gz` — direct archive URL (no `#ref`). -//! -//! The parser is scoped to the lpc8xx fix in #663. Future work -//! (FastLED/fbuild#664) will generalize this to every framework package. - -use std::collections::HashMap; - -/// A resolved framework override: a downloadable archive URL plus the ref -/// (commit / tag) the consumer pinned. The caller turns the ref into a -/// synthetic cache `version` so override installs don't collide with the -/// default pin. -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct PlatformPackageOverride { - pub archive_url: String, - pub git_ref: String, -} - -/// Look up `framework` inside the resolved `[env:*]` config and return -/// the override the consumer pinned, if any. -pub fn lookup_override( - env_config: &HashMap, - framework: &str, -) -> Option { - let raw = env_config.get("platform_packages")?; - parse_platform_packages_entry(raw, framework) -} - -/// Scan a `platform_packages` multi-line value for `@` -/// and resolve `` to an archive URL + ref. -pub fn parse_platform_packages_entry( - raw: &str, - framework: &str, -) -> Option { - for line in raw.lines() { - let line = strip_inline_comment(line.trim()); - if line.is_empty() { - continue; - } - let (name, spec) = match line.split_once('@') { - Some(pair) => pair, - None => continue, - }; - if name.trim() != framework { - continue; - } - if let Some(parsed) = resolve_spec(spec.trim()) { - return Some(parsed); - } - } - None -} - -fn resolve_spec(spec: &str) -> Option { - if let Some((source, git_ref)) = spec.rsplit_once('#') { - let source = source.trim(); - let git_ref = git_ref.trim(); - if git_ref.is_empty() { - return None; - } - - if let Some(rest) = source.strip_prefix("https://github.com/") { - let repo = rest.strip_suffix(".git").unwrap_or(rest); - let repo = repo.trim_end_matches('/'); - if is_owner_repo(repo) { - return Some(PlatformPackageOverride { - archive_url: github_archive_url(repo, git_ref), - git_ref: git_ref.to_string(), - }); - } - } - - if is_owner_repo(source) { - return Some(PlatformPackageOverride { - archive_url: github_archive_url(source, git_ref), - git_ref: git_ref.to_string(), - }); - } - } - - if is_archive_url(spec) { - let git_ref = spec - .rsplit('/') - .next() - .unwrap_or(spec) - .trim_end_matches(".tar.gz") - .trim_end_matches(".tar.bz2") - .trim_end_matches(".zip") - .to_string(); - return Some(PlatformPackageOverride { - archive_url: spec.to_string(), - git_ref, - }); - } - - None -} - -fn is_owner_repo(s: &str) -> bool { - let parts: Vec<&str> = s.split('/').collect(); - parts.len() == 2 - && !parts[0].is_empty() - && !parts[1].is_empty() - && !s.contains("://") - && !s.contains(' ') -} - -fn is_archive_url(s: &str) -> bool { - (s.starts_with("http://") || s.starts_with("https://")) - && (s.ends_with(".tar.gz") || s.ends_with(".tar.bz2") || s.ends_with(".zip")) -} - -fn github_archive_url(owner_repo: &str, git_ref: &str) -> String { - format!( - "https://github.com/{}/archive/{}.tar.gz", - owner_repo, git_ref - ) -} - -fn strip_inline_comment(s: &str) -> &str { - for marker in [" ;", " #"] { - if let Some(idx) = s.find(marker) { - return s[..idx].trim(); - } - } - s -} - -#[cfg(test)] -mod tests { - use super::*; - - fn env(value: &str) -> HashMap { - let mut m = HashMap::new(); - m.insert("platform_packages".to_string(), value.to_string()); - m - } - - #[test] - fn shorthand_owner_repo_resolves_to_archive_url() { - let got = parse_platform_packages_entry( - "framework-arduino-lpc8xx@zackees/ArduinoCore-LPC8xx#195a2ed", - "framework-arduino-lpc8xx", - ) - .unwrap(); - assert_eq!( - got, - PlatformPackageOverride { - archive_url: "https://github.com/zackees/ArduinoCore-LPC8xx/archive/195a2ed.tar.gz" - .to_string(), - git_ref: "195a2ed".to_string(), - } - ); - } - - #[test] - fn git_url_with_ref_resolves_to_archive_url() { - let got = parse_platform_packages_entry( - "framework-arduino-lpc8xx@https://github.com/zackees/ArduinoCore-LPC8xx.git#deadbee", - "framework-arduino-lpc8xx", - ) - .unwrap(); - assert_eq!( - got.archive_url, - "https://github.com/zackees/ArduinoCore-LPC8xx/archive/deadbee.tar.gz" - ); - assert_eq!(got.git_ref, "deadbee"); - } - - #[test] - fn github_https_url_without_dot_git_still_resolves() { - let got = parse_platform_packages_entry( - "framework-arduino-lpc8xx@https://github.com/zackees/ArduinoCore-LPC8xx#abc1234", - "framework-arduino-lpc8xx", - ) - .unwrap(); - assert_eq!( - got.archive_url, - "https://github.com/zackees/ArduinoCore-LPC8xx/archive/abc1234.tar.gz" - ); - assert_eq!(got.git_ref, "abc1234"); - } - - #[test] - fn direct_archive_url_passes_through() { - let got = parse_platform_packages_entry( - "framework-arduino-lpc8xx@https://github.com/zackees/ArduinoCore-LPC8xx/archive/195a2ed.tar.gz", - "framework-arduino-lpc8xx", - ) - .unwrap(); - assert_eq!( - got.archive_url, - "https://github.com/zackees/ArduinoCore-LPC8xx/archive/195a2ed.tar.gz" - ); - assert_eq!(got.git_ref, "195a2ed"); - } - - #[test] - fn multiline_value_finds_the_matching_framework() { - let raw = "\n other-framework@foo/bar#1\n framework-arduino-lpc8xx@zackees/ArduinoCore-LPC8xx#deadbee\n"; - let got = parse_platform_packages_entry(raw, "framework-arduino-lpc8xx").unwrap(); - assert_eq!(got.git_ref, "deadbee"); - } - - #[test] - fn no_match_when_framework_name_differs() { - let got = parse_platform_packages_entry( - "framework-other@zackees/ArduinoCore-LPC8xx#deadbee", - "framework-arduino-lpc8xx", - ); - assert!(got.is_none()); - } - - #[test] - fn returns_none_on_empty_value() { - let got = parse_platform_packages_entry("", "framework-arduino-lpc8xx"); - assert!(got.is_none()); - } - - #[test] - fn lookup_override_reads_from_env_config() { - let cfg = env("framework-arduino-lpc8xx@zackees/ArduinoCore-LPC8xx#deadbee"); - let got = lookup_override(&cfg, "framework-arduino-lpc8xx").unwrap(); - assert_eq!(got.git_ref, "deadbee"); - } - - #[test] - fn lookup_override_returns_none_when_key_missing() { - let cfg: HashMap = HashMap::new(); - assert!(lookup_override(&cfg, "framework-arduino-lpc8xx").is_none()); - } - - #[test] - fn ignores_comments_and_blank_lines() { - let raw = "\n# comment\n ; inline-style\n framework-arduino-lpc8xx@zackees/ArduinoCore-LPC8xx#deadbee\n"; - let got = parse_platform_packages_entry(raw, "framework-arduino-lpc8xx").unwrap(); - assert_eq!(got.git_ref, "deadbee"); - } - - #[test] - fn empty_ref_rejected() { - let got = parse_platform_packages_entry( - "framework-arduino-lpc8xx@zackees/ArduinoCore-LPC8xx#", - "framework-arduino-lpc8xx", - ); - assert!(got.is_none()); - } -} diff --git a/crates/fbuild-build/src/package_override.rs b/crates/fbuild-build/src/package_override.rs new file mode 100644 index 00000000..9bc0c99a --- /dev/null +++ b/crates/fbuild-build/src/package_override.rs @@ -0,0 +1,91 @@ +//! Shared `platform_packages` override resolver for every framework orchestrator. +//! +//! PlatformIO honors `platform_packages = framework-x@#` on the env +//! section; fbuild's framework packages used to ignore it (audit: #664; first +//! report: #663). The per-orchestrator wiring is now a uniform three-line delta: +//! +//! ```ignore +//! let core = match package_override::resolve_override(env_config, "framework-arduino-lpc8xx") { +//! Some(ovr) => ArduinoCoreLpc8xx::with_override(¶ms.project_dir, ovr), +//! None => ArduinoCoreLpc8xx::new(¶ms.project_dir), +//! }; +//! ``` +//! +//! Centralizing the lookup here means every framework orchestrator gets the +//! same parsing behavior — there is no per-platform place to forget multi-line +//! handling, owner/repo expansion, or empty-value tolerance. + +use std::collections::HashMap; + +use fbuild_config::PackageOverride; + +/// Look up a `platform_packages` override for `package_name` in the resolved +/// env config (`PlatformIOConfig::get_env_config(env)`). +/// +/// Returns `None` when the env has no `platform_packages` key, or when no entry +/// in that value matches `package_name`. Multi-line values are scanned in order +/// and the first match wins (PlatformIO semantics). +pub fn resolve_override( + env_config: &HashMap, + package_name: &str, +) -> Option { + let raw = env_config.get("platform_packages")?; + fbuild_config::parse_platform_packages_value(raw, package_name) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn env(pairs: &[(&str, &str)]) -> HashMap { + pairs + .iter() + .map(|(k, v)| ((*k).to_string(), (*v).to_string())) + .collect() + } + + #[test] + fn returns_override_when_env_has_matching_entry() { + let env = env(&[( + "platform_packages", + "framework-arduino-lpc8xx@https://github.com/zackees/ArduinoCore-LPC8xx/archive/aaaabbbbccccddddeeeeffff0000111122223333.tar.gz#aaaabbbbccccddddeeeeffff0000111122223333", + )]); + let ovr = resolve_override(&env, "framework-arduino-lpc8xx").expect("override resolved"); + assert!(ovr.url.contains("ArduinoCore-LPC8xx")); + assert_eq!(ovr.version, "0.0.0+gaaaabbb"); + } + + #[test] + fn returns_none_when_platform_packages_key_absent() { + let env = env(&[("build_flags", "-DFOO=1")]); + assert!(resolve_override(&env, "framework-arduino-lpc8xx").is_none()); + } + + #[test] + fn returns_none_when_no_entry_matches_package_name() { + let env = env(&[( + "platform_packages", + "framework-some-other-thing@https://example.com/archive/abc.tar.gz#abc", + )]); + assert!(resolve_override(&env, "framework-arduino-lpc8xx").is_none()); + } + + #[test] + fn multi_line_value_picks_first_matching_entry() { + // PlatformIO INI parsing joins continuation lines with `\n`; the + // helper must scan all of them and return the first match. + let env = env(&[( + "platform_packages", + "framework-arduino-lpc8xx@zackees/ArduinoCore-LPC8xx#deadbeefdeadbeefdeadbeefdeadbeefdeadbeef\nframework-arduino-lpc8xx@zackees/ArduinoCore-LPC8xx#cafef00dcafef00dcafef00dcafef00dcafef00d", + )]); + let ovr = resolve_override(&env, "framework-arduino-lpc8xx").unwrap(); + assert_eq!(ovr.version, "0.0.0+gdeadbee"); + } + + #[test] + fn version_pin_only_returns_none() { + // `name @ 1.2.3` is a registry version pin, not a URL override. + let env = env(&[("platform_packages", "framework-arduino-lpc8xx @ 1.2.3")]); + assert!(resolve_override(&env, "framework-arduino-lpc8xx").is_none()); + } +} diff --git a/crates/fbuild-config/src/lib.rs b/crates/fbuild-config/src/lib.rs index 1e02d01c..1bb82d64 100644 --- a/crates/fbuild-config/src/lib.rs +++ b/crates/fbuild-config/src/lib.rs @@ -10,6 +10,7 @@ pub mod board; pub mod ini_parser; pub mod mcu; pub mod pio_env; +pub mod platform_packages; pub mod sdkconfig; pub use board::{BoardConfig, DebugToolMeta, Esp32QemuPsramConfig}; @@ -19,3 +20,6 @@ pub use pio_env::{ scan_unsupported, scan_warn_only, PioEnvOverrides, SUPPORTED_PIO_ENV_VARS, WARN_ONLY_PIO_ENV_VARS, }; +pub use platform_packages::{ + parse_platform_packages_entry, parse_platform_packages_value, PackageOverride, +}; diff --git a/crates/fbuild-config/src/platform_packages.rs b/crates/fbuild-config/src/platform_packages.rs new file mode 100644 index 00000000..b0ec522c --- /dev/null +++ b/crates/fbuild-config/src/platform_packages.rs @@ -0,0 +1,258 @@ +//! Parsing for the PlatformIO `platform_packages` directive. +//! +//! PlatformIO lets a consumer override a framework package's source by pinning +//! a URL + commit on the env's `platform_packages` line: +//! +//! ```ini +//! [env:lpc845brk] +//! platform_packages = +//! framework-arduino-lpc8xx@https://github.com/owner/repo/archive/.tar.gz +//! framework-arduino-lpc8xx@owner/repo# +//! ``` +//! +//! fbuild's framework packages used to ignore this line entirely (every package +//! pinned its URL + commit + sha256 as `const &str`). FastLED/fbuild#664 +//! audited the gap across all 16 framework packages and #681 is the +//! implementation follow-up: parsing lives here, the `PackageBase` abstraction +//! consumes the parsed `PackageOverride`, and per-orchestrator wiring is a +//! uniform 3-line delta. +//! +//! The original LPC8xx bisection workflow that motivated this — see +//! FastLED/fbuild#663 and FastLED/FastLED#3325 — is unblocked once the +//! consumer's `platform_packages` line reaches `PackageBase::with_override`. + +/// A consumer-supplied override for a `PackageBase`. +/// +/// Constructed by [`parse_platform_packages_entry`] (or by tests). Applied +/// via `PackageBase::with_override`, which derives a distinct cache subdir +/// from `url` so the override never collides with the default-pinned cache. +/// +/// `checksum` is `None` for overrides parsed from `platform_packages`: the +/// consumer is explicitly overriding for development / bisection and we don't +/// expect them to recompute sha256 per pin. The cache key derivation +/// (URL → stem + sha256-prefix in `fbuild_packages::Cache`) is what guarantees +/// uniqueness — two different commit URLs hash to two different directories. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PackageOverride { + /// Full download URL (typically `https://github.com///archive/.tar.gz`). + pub url: String, + /// Cache-subdir-discriminating version string. Conventionally `+g` + /// so the override's cache path doesn't collide with the default pin. + pub version: String, + /// Optional SHA-256 of the archive. `None` skips checksum verification — + /// the consumer is explicitly trusting the pin. + pub checksum: Option, +} + +impl PackageOverride { + /// Convenience constructor with no checksum (the common case for parsed + /// `platform_packages` lines). + pub fn new(url: impl Into, version: impl Into) -> Self { + Self { + url: url.into(), + version: version.into(), + checksum: None, + } + } +} + +/// Parse a single `platform_packages` line for an entry matching `package_name`. +/// +/// PlatformIO syntaxes accepted: +/// +/// `name@#` → uses `` as the download URL and `` as the version discriminator +/// `name@` (no `#sha`) → uses `` as the download URL; version derives from the URL tail +/// `name@/#` → expands to the GitHub archive tarball URL for that sha +/// `name @ ` → returns `None` (version pin, not a URL override) +/// anything else → returns `None` +/// +/// Returns `None` when `package_name` does not match the entry's name. +/// +/// Whitespace around the `@` separator and around the line itself is tolerated. +pub fn parse_platform_packages_entry(line: &str, package_name: &str) -> Option { + let line = line.trim().trim_end_matches([',', ';']).trim(); + if line.is_empty() { + return None; + } + + // Split on the first `@`. The `name` side may have whitespace around it + // (e.g. `name @ value`). + let (name, rest) = line.split_once('@')?; + let name = name.trim(); + if name != package_name { + return None; + } + let rest = rest.trim(); + if rest.is_empty() { + return None; + } + + // Plain `name @ ` (no URL, no slash, no `#sha`) is a registry + // version pin, not an override we can act on. + let looks_like_url = rest.starts_with("http://") || rest.starts_with("https://"); + let looks_like_owner_repo = rest.contains('/') && rest.contains('#'); + if !looks_like_url && !looks_like_owner_repo { + return None; + } + + // Split off `#` if present. + let (target, sha) = match rest.split_once('#') { + Some((t, s)) => (t.trim(), Some(s.trim())), + None => (rest, None), + }; + + let (url, version) = if looks_like_url { + let url = target.to_string(); + let version = match sha { + Some(s) if !s.is_empty() => version_string(s), + _ => version_from_url_tail(target), + }; + (url, version) + } else { + // owner/repo#sha → GitHub archive URL + let sha = sha?; + if sha.is_empty() { + return None; + } + let owner_repo = target; + if owner_repo.matches('/').count() != 1 { + return None; + } + let url = format!("https://github.com/{}/archive/{}.tar.gz", owner_repo, sha); + (url, version_string(sha)) + }; + + Some(PackageOverride { + url, + version, + checksum: None, + }) +} + +/// Scan a multi-line `platform_packages` value and return the first override +/// matching `package_name`, if any. +/// +/// `value` is the raw INI value as returned by `PlatformIOConfig::get_env_config` +/// (multi-line PlatformIO values are joined with `\n` by the parser). +pub fn parse_platform_packages_value(value: &str, package_name: &str) -> Option { + value + .lines() + .filter_map(|line| parse_platform_packages_entry(line, package_name)) + .next() +} + +fn version_string(sha: &str) -> String { + // `0.0.0+g` — keeps the cache-subdir distinct from the default + // pin (which uses its own `+g` pattern). The `0.0.0+` base + // is a placeholder; the `+g` build-metadata is what differentiates. + let short = sha.get(..7).unwrap_or(sha); + format!("0.0.0+g{}", short) +} + +fn version_from_url_tail(url: &str) -> String { + // For `https://.../archive/.tar.gz` (no explicit `#sha`), pull the + // sha out of the URL tail. Falls back to a generic `override` tag if the + // tail isn't a recognizable sha-shaped token. + let last = url.rsplit('/').next().unwrap_or(""); + let stripped = last + .strip_suffix(".tar.gz") + .or_else(|| last.strip_suffix(".zip")) + .or_else(|| last.strip_suffix(".tar.bz2")) + .or_else(|| last.strip_suffix(".tar.xz")) + .unwrap_or(last); + if !stripped.is_empty() && stripped.chars().all(|c| c.is_ascii_hexdigit()) { + version_string(stripped) + } else { + "0.0.0+override".to_string() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn url_with_sha_returns_override() { + let line = "framework-arduino-lpc8xx@https://github.com/zackees/ArduinoCore-LPC8xx/archive/aaaabbbbccccddddeeeeffff0000111122223333.tar.gz#aaaabbbbccccddddeeeeffff0000111122223333"; + let got = parse_platform_packages_entry(line, "framework-arduino-lpc8xx").unwrap(); + assert_eq!( + got.url, + "https://github.com/zackees/ArduinoCore-LPC8xx/archive/aaaabbbbccccddddeeeeffff0000111122223333.tar.gz" + ); + assert_eq!(got.version, "0.0.0+gaaaabbb"); + assert_eq!(got.checksum, None); + } + + #[test] + fn url_without_sha_derives_version_from_url_tail() { + let line = "framework-arduino-lpc8xx@https://github.com/zackees/ArduinoCore-LPC8xx/archive/abcdef1234567890abcdef1234567890abcdef12.tar.gz"; + let got = parse_platform_packages_entry(line, "framework-arduino-lpc8xx").unwrap(); + assert_eq!(got.version, "0.0.0+gabcdef1"); + } + + #[test] + fn owner_repo_with_sha_expands_to_github_archive() { + let line = + "framework-arduino-lpc8xx@zackees/ArduinoCore-LPC8xx#1234567890abcdef1234567890abcdef12345678"; + let got = parse_platform_packages_entry(line, "framework-arduino-lpc8xx").unwrap(); + assert_eq!( + got.url, + "https://github.com/zackees/ArduinoCore-LPC8xx/archive/1234567890abcdef1234567890abcdef12345678.tar.gz" + ); + assert_eq!(got.version, "0.0.0+g1234567"); + } + + #[test] + fn version_only_returns_none() { + // `name @ 1.2.3` — registry version pin, not a URL override + assert_eq!( + parse_platform_packages_entry( + "framework-arduino-lpc8xx @ 1.2.3", + "framework-arduino-lpc8xx" + ), + None + ); + // `name@1.2.3` (no space, no URL, no slash, no `#`) + assert_eq!( + parse_platform_packages_entry( + "framework-arduino-lpc8xx@1.2.3", + "framework-arduino-lpc8xx" + ), + None + ); + } + + #[test] + fn non_matching_package_name_returns_none() { + let line = "some-other-package@https://example.com/archive/abc.tar.gz#abc"; + assert_eq!( + parse_platform_packages_entry(line, "framework-arduino-lpc8xx"), + None + ); + } + + #[test] + fn empty_or_blank_line_returns_none() { + assert_eq!(parse_platform_packages_entry("", "x"), None); + assert_eq!(parse_platform_packages_entry(" ", "x"), None); + } + + #[test] + fn multi_line_value_returns_first_match() { + let value = " + framework-other@https://example.com/archive/aaa.tar.gz#aaa + framework-arduino-lpc8xx@https://github.com/zackees/ArduinoCore-LPC8xx/archive/deadbeefdeadbeefdeadbeefdeadbeefdeadbeef.tar.gz#deadbeefdeadbeefdeadbeefdeadbeefdeadbeef + framework-arduino-lpc8xx@https://example.com/archive/should_not_win.tar.gz#1111111111111111111111111111111111111111 + "; + let got = parse_platform_packages_value(value, "framework-arduino-lpc8xx").unwrap(); + assert_eq!(got.version, "0.0.0+gdeadbee"); + assert!(got.url.contains("ArduinoCore-LPC8xx")); + } + + #[test] + fn trailing_comma_or_semicolon_tolerated() { + let line = "framework-arduino-lpc8xx@zackees/ArduinoCore-LPC8xx#abc,"; + let got = parse_platform_packages_entry(line, "framework-arduino-lpc8xx").unwrap(); + assert!(got.url.contains("archive/abc.tar.gz")); + } +} diff --git a/crates/fbuild-packages/src/lib.rs b/crates/fbuild-packages/src/lib.rs index 91e56068..fe3d8ab1 100644 --- a/crates/fbuild-packages/src/lib.rs +++ b/crates/fbuild-packages/src/lib.rs @@ -251,6 +251,30 @@ impl PackageBase { } } + /// Apply a consumer-provided override (e.g. parsed from `platform_packages` + /// in `platformio.ini`). + /// + /// Replaces `url`, `cache_key` (← override URL), `version`, and `checksum`. + /// Preserves `name` and `cache_subdir`. The cache-key swap is what gives the + /// override its own subdir under `~/.fbuild//cache/////`, + /// so two different commit URLs hash to two different directories and a + /// bisection workflow doesn't fight the default cache. + /// + /// `checksum: None` skips sha256 verification — consumer-trusted, which is + /// the right policy for `platform_packages` overrides (#681, sibling of #663). + /// + /// Emits a single INFO log so the override is visible in build scrollback + /// across every framework package, without each orchestrator having to + /// remember to log it themselves. + pub fn with_override(mut self, ovr: fbuild_config::PackageOverride) -> Self { + tracing::info!("{} OVERRIDE: {} (was {})", self.name, ovr.url, self.url); + self.url = ovr.url.clone(); + self.cache_key = ovr.url; + self.version = ovr.version; + self.checksum = ovr.checksum; + self + } + /// Get the install path in the cache. pub fn install_path(&self) -> PathBuf { match self.cache_subdir { @@ -681,3 +705,126 @@ mod toolchain_gcc_ar_tests { assert!(disk_cache::paths::install_complete_sentinel(&installed).exists()); } } + +#[cfg(test)] +mod package_override_tests { + //! Cache-key uniqueness tests for `PackageBase::with_override`. + //! + //! The contract that FastLED/fbuild#681 promises every framework + //! orchestrator is: when a consumer writes + //! `platform_packages = framework-x@#` in `platformio.ini`, + //! the resulting cache dir is **distinct** from the default pin's cache + //! dir AND distinct from any other override at a different commit on the + //! same upstream URL. Without that, bisection workflows like + //! FastLED/FastLED#3325 silently reuse the wrong vendored sources. + //! + //! These tests pin the contract at the `PackageBase` level so the + //! invariant holds for every framework package (16 of them at audit + //! time) without each package needing to re-prove it. + use super::*; + use fbuild_config::PackageOverride; + + const DEFAULT_URL: &str = "https://github.com/example/repo/archive/default.tar.gz"; + + fn make_base(tmp: &Path, cache_root: &Path) -> PackageBase { + PackageBase::with_cache_root( + "framework-test", + "0.1.0+gdefault", + DEFAULT_URL, + DEFAULT_URL, + Some("0000000000000000000000000000000000000000000000000000000000000000"), + CacheSubdir::Platforms, + tmp, + cache_root, + ) + } + + #[test] + fn override_changes_install_path() { + // This is the unit test FastLED/fbuild#681 calls out by name: + // 1. Default pin → some install_path P_default. + // 2. Override URL A → install_path P_A, must differ from P_default. + // 3. Override URL B (same repo, different commit) → install_path P_B, + // must differ from both P_default and P_A. + // + // If any two of these collide, a bisection step that swaps the URL + // would silently reuse the previous commit's vendored sources — the + // exact failure mode the override is meant to prevent. + let tmp = tempfile::TempDir::new().unwrap(); + let cache_root = tmp.path().join("cache"); + + let default_base = make_base(tmp.path(), &cache_root); + let p_default = default_base.install_path(); + + let ovr_a = PackageOverride { + url: "https://github.com/example/repo/archive/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.tar.gz" + .to_string(), + version: "0.0.0+gaaaaaaa".to_string(), + checksum: None, + }; + let p_a = make_base(tmp.path(), &cache_root) + .with_override(ovr_a.clone()) + .install_path(); + + let ovr_b = PackageOverride { + url: "https://github.com/example/repo/archive/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb.tar.gz" + .to_string(), + version: "0.0.0+gbbbbbbb".to_string(), + checksum: None, + }; + let p_b = make_base(tmp.path(), &cache_root) + .with_override(ovr_b) + .install_path(); + + assert_ne!( + p_default, p_a, + "override URL must produce a different cache dir from the default pin" + ); + assert_ne!( + p_default, p_b, + "second override URL must also differ from the default pin" + ); + assert_ne!( + p_a, p_b, + "same upstream repo at different commits MUST hash to different cache dirs \ + — otherwise a bisection step silently reuses the previous commit's sources" + ); + + // Round-trip sanity: applying the same override twice yields the same path + // (the hash is deterministic, not session-dependent). + let p_a_again = make_base(tmp.path(), &cache_root) + .with_override(ovr_a) + .install_path(); + assert_eq!(p_a, p_a_again, "override hash must be deterministic"); + } + + #[test] + fn override_replaces_url_cache_key_version_and_checksum() { + // The cache-key swap is the load-bearing field — assert it directly so a + // future refactor can't accidentally preserve the default `cache_key` + // while updating only `url`. + let tmp = tempfile::TempDir::new().unwrap(); + let cache_root = tmp.path().join("cache"); + let base = make_base(tmp.path(), &cache_root); + assert_eq!(base.cache_key, DEFAULT_URL); + assert!(base.checksum.is_some()); + + let ovr = PackageOverride { + url: "https://example.com/override/archive/cafef00d.tar.gz".to_string(), + version: "0.0.0+gcafef00".to_string(), + checksum: None, + }; + let overridden = base.with_override(ovr.clone()); + assert_eq!(overridden.url, ovr.url); + assert_eq!( + overridden.cache_key, ovr.url, + "cache_key MUST be set from the override URL — install_path() uses cache_key, not url" + ); + assert_eq!(overridden.version, ovr.version); + assert_eq!(overridden.checksum, None); + assert_eq!( + overridden.name, "framework-test", + "name is preserved across override" + ); + } +} diff --git a/crates/fbuild-packages/src/library/arduino_core_lpc8xx.rs b/crates/fbuild-packages/src/library/arduino_core_lpc8xx.rs index 6a35704a..a05f5046 100644 --- a/crates/fbuild-packages/src/library/arduino_core_lpc8xx.rs +++ b/crates/fbuild-packages/src/library/arduino_core_lpc8xx.rs @@ -53,30 +53,23 @@ impl ArduinoCoreLpc8xx { } } - /// Consumer-overridden core source (FastLED/fbuild#663). - /// - /// Used when `platformio.ini` carries a - /// `platform_packages = framework-arduino-lpc8xx@#` line so - /// the consumer can swap in an arbitrary upstream commit for - /// development or bisection (e.g. FastLED/FastLED#3325). The cache - /// subdir keys on `url` + `version`, so an override never collides - /// with the default-pinned install. - /// - /// `url` must resolve to a downloadable archive (`.tar.gz` / `.zip`); - /// `version` is the synthetic cache-version string (e.g. - /// `override+g`). No sha256 check is performed — the - /// consumer is explicitly pinning their own commit. - pub fn with_override(project_dir: &Path, url: &str, version: &str) -> Self { + /// Construct with a consumer-supplied override (parsed from the env's + /// `platform_packages` line in `platformio.ini`). The default const-pinned + /// URL / version / checksum are replaced; `cache_subdir` and `name` are + /// preserved. See `PackageBase::with_override` and FastLED/fbuild#681 + /// (the consolidation of the LPC8xx-specific path introduced in #663). + pub fn with_override(project_dir: &Path, ovr: fbuild_config::PackageOverride) -> Self { Self { base: PackageBase::new( "framework-arduino-lpc8xx", - version, - url, - url, - None, + ACLPC_VERSION, + ACLPC_URL, + ACLPC_URL, + Some(ACLPC_CHECKSUM), CacheSubdir::Platforms, project_dir, - ), + ) + .with_override(ovr), } } @@ -96,27 +89,6 @@ impl ArduinoCoreLpc8xx { } } - #[cfg(test)] - fn with_override_and_cache_root( - project_dir: &Path, - url: &str, - version: &str, - cache_root: &Path, - ) -> Self { - Self { - base: PackageBase::with_cache_root( - "framework-arduino-lpc8xx", - version, - url, - url, - None, - CacheSubdir::Platforms, - project_dir, - cache_root, - ), - } - } - /// The pinned upstream commit SHA. pub fn commit() -> &'static str { ACLPC_COMMIT @@ -255,20 +227,7 @@ mod tests { assert_eq!(ArduinoCoreLpc8xx::commit().len(), 40); } - #[test] - fn override_uses_distinct_cache_path() { - // FastLED/fbuild#663: an override must cache to a different - // subdir than the default pin so the two installs don't collide. - let tmp = tempfile::TempDir::new().unwrap(); - let cache_root = tmp.path().join("cache"); - let default_pkg = ArduinoCoreLpc8xx::with_cache_root(tmp.path(), &cache_root); - let override_pkg = ArduinoCoreLpc8xx::with_override_and_cache_root( - tmp.path(), - "https://github.com/zackees/ArduinoCore-LPC8xx/archive/deadbeefdeadbeefdeadbeefdeadbeefdeadbeef.tar.gz", - "override+gdeadbee", - &cache_root, - ); - assert_ne!(default_pkg.install_path(), override_pkg.install_path()); - assert!(!override_pkg.is_installed()); - } + // Override cache-key uniqueness is asserted at the abstraction level in + // `crates/fbuild-packages/src/lib.rs::package_override_tests` so the + // invariant doesn't need to be re-proved per-package. }