perf(esp32): download GCC toolchain concurrently with framework + SDK libs (#953)

[zackees]

Summary

The cold pioarduino-resolve phase (#942 baseline ~185–237 s) downloaded the GCC toolchain (~100+ MB) and the arduino-esp32 framework + SDK libs (~hundreds MB) strictly sequentially. Once the platform's platform.json metadata URLs are resolved, those are fully independent packages — different cache dirs, different per-package install locks — so this overlaps their download+extract with tokio::join! instead of summing them.

The framework chain stays internally ordered (framework must be installed before its SDK libs extract into tools/); only the toolchain (independent) is lifted to run alongside it.

before: platform → [framework → SDK-libs] → toolchain
after: platform → [ [framework → SDK-libs] || toolchain ]

Safety / scope

• install_lock.rs keys locks by the package's own install path, so concurrent installs of different packages were always safe — no new locks needed.
• Only the ESP32 resolve path changes; the framework→libs ordering is preserved.
• build.rs's post-resolve ensure_installed(&toolchain) is now an idempotent cache-hit no-op (resolve installed it).

Measurement caveat (honest)

On a bandwidth-rich link (CI runners) the two large downloads overlap and hide the smaller under the larger. On a saturated link, total bytes ÷ bandwidth dominates, so the overlap is neutral. The local #942 Docker harness run was confounded (a second container competing for CPU inflated the cc1plus compile phases, and the single cold run is network-noisy), so I can't cleanly reproduce the delta single-machine — but the change is architecturally correct and non-regressing, and directly implements the "concurrent downloading/installation" part of #942's goal.

Further #953 opportunities (deferred, larger refactors): kick off user lib_deps downloads in the background to overlap the toolchain/SDK fetch, and start the core/fw-libs compile as soon as the toolchain + framework core-sources exist while the SDK libs finish extracting.

Part of #953 / #942.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Performance
  • Improved ESP32 package setup by preparing the toolchain and framework-related components in parallel, reducing overall installation time.
• Bug Fixes
  • Made framework-related downloads and MCU-specific library setup more consistent during provisioning.

[coderabbitai]

📝 Walkthrough

Walkthrough

The resolve_pioarduino_packages function is refactored to precompute framework and MCU skeleton-lib URLs upfront, then run GCC toolchain installation concurrently with framework installation and subsequent libs provisioning using tokio::join!, replacing the prior sequential execution.

Changes

Concurrent Package Installation

Layer / File(s)	Summary
URL derivation and concurrent install flow crates/fbuild-build/src/esp32/orchestrator/packages.rs	Precomputes mcu_suffix and optional platform-derived URLs for framework-libs and skeleton-libs packages, then runs toolchain installation and framework installation (followed by conditional libs/skeleton-libs provisioning) concurrently via tokio::join!, combining and propagating errors from both.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
participant Orchestrator
participant ToolchainInstaller
participant FrameworkInstaller
Orchestrator->>Orchestrator: derive framework-libs and skeleton-lib URLs
par toolchain install
Orchestrator->>ToolchainInstaller: ensure toolchain installed
and framework install
Orchestrator->>FrameworkInstaller: ensure framework installed
FrameworkInstaller->>FrameworkInstaller: ensure_libs / ensure_mcu_libs
end
ToolchainInstaller-->>Orchestrator: result
FrameworkInstaller-->>Orchestrator: result

Loading

Possibly related issues

• perf(packages): overlap download/extract/install in cold pioarduino-resolve (~185-237s serialized) #953: Reworks resolve_pioarduino_packages to overlap ESP32 toolchain and framework/libs installation instead of running them strictly sequentially, matching this PR's change.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main ESP32 performance change: concurrent GCC toolchain downloads with framework and SDK libs.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch perf/953-concurrent-resolve

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/fbuild-build/src/esp32/orchestrator/packages.rs (1)

18-115: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Add coverage for the concurrent package install pathcrates/fbuild-build/src/esp32/orchestrator/packages.rs:97-110
resolve_pioarduino_packages() now depends on tokio::join!, but the ESP32 orchestrator tests only cover CDC/helpers. Add an async tempdir/fixture-based test that exercises the package-resolution path, including failure propagation and the framework-before-libs ordering.

🤖 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/src/esp32/orchestrator/packages.rs` around lines 18 -
115, Add an async test for resolve_pioarduino_packages that uses a
tempdir/fixture setup to exercise the tokio::join! install path, since current
ESP32 orchestrator coverage only hits CDC/helpers. Make the test verify both
successful package resolution and failure propagation from either branch, and
assert the framework install completes before
framework.ensure_libs/ensure_mcu_libs is invoked. Use
resolve_pioarduino_packages, provision_helper_toolchains, and the
framework_fut/toolchain_fut flow as the main targets to locate the logic.

Source: Coding guidelines

🧹 Nitpick comments (1)

crates/fbuild-build/src/esp32/orchestrator/packages.rs (1)

86-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Silent skip on missing libs/skeleton metadata URLs.

libs_url/skeleton_url resolution failures are swallowed via .ok() with no logging, unlike the framework URL resolution just above (lines 64-73) which logs via tracing::info!/tracing::warn!. If get_package_url fails here (e.g., platform.json schema drift, renamed package entry), SDK libs or MCU skeleton libs are silently skipped, likely surfacing later as a confusing missing-header build failure rather than a clear "could not resolve libs URL" message.

♻️ Suggested fix: log on resolution failure

 let libs_url = platform
.get_package_url("framework-arduinoespressif32-libs")
- .ok();+ .inspect_err(|e| tracing::debug!("no split libs package URL: {}", e))+ .ok();
let skeleton_url = if mcu_suffix.is_empty() {
None
} else {
platform
.get_package_url(&format!("framework-arduino-{}-skeleton-lib", mcu_suffix))
+ .inspect_err(|e| tracing::debug!("no skeleton-lib package URL for {}: {}", mcu_suffix, e))
.ok()
};

🤖 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/src/esp32/orchestrator/packages.rs` around lines 86 - 95,
The libs and skeleton package URL lookups in the package resolution logic are
swallowing failures with .ok(), so missing metadata is skipped silently. Update
the URL resolution in the orchestrator/packages.rs path around get_package_url
for framework-arduinoespressif32-libs and framework-arduino-{}-skeleton-lib to
log a tracing::warn! or tracing::info! when resolution fails, similar to the
framework URL handling above, and keep the fallback to None only after emitting
the diagnostic.

🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/fbuild-build/src/esp32/orchestrator/packages.rs`:
- Around line 110-112: The error handling after tokio::join! in the package
orchestration flow only propagates the toolchain result first, so a simultaneous
framework/libs failure can be lost. Update the logic around the join in
packages.rs to preserve and report the framework failure as well when both
futures error, using the existing toolchain_res and framework_res results from
the join. Make sure the implementation surfaces the more relevant error context
instead of discarding the second failure, especially for the
framework/skeleton-libs extraction path.
---
Outside diff comments:
In `@crates/fbuild-build/src/esp32/orchestrator/packages.rs`:
- Around line 18-115: Add an async test for resolve_pioarduino_packages that
uses a tempdir/fixture setup to exercise the tokio::join! install path, since
current ESP32 orchestrator coverage only hits CDC/helpers. Make the test verify
both successful package resolution and failure propagation from either branch,
and assert the framework install completes before
framework.ensure_libs/ensure_mcu_libs is invoked. Use
resolve_pioarduino_packages, provision_helper_toolchains, and the
framework_fut/toolchain_fut flow as the main targets to locate the logic.
---
Nitpick comments:
In `@crates/fbuild-build/src/esp32/orchestrator/packages.rs`:
- Around line 86-95: The libs and skeleton package URL lookups in the package
resolution logic are swallowing failures with .ok(), so missing metadata is
skipped silently. Update the URL resolution in the orchestrator/packages.rs path
around get_package_url for framework-arduinoespressif32-libs and
framework-arduino-{}-skeleton-lib to log a tracing::warn! or tracing::info! when
resolution fails, similar to the framework URL handling above, and keep the
fallback to None only after emitting the diagnostic.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7e3b3dd1-b6e7-4ef0-a9cd-1d7465a81af1

📥 Commits

Reviewing files that changed from the base of the PR and between bfece80 and 7de7664.

📒 Files selected for processing (1)

• crates/fbuild-build/src/esp32/orchestrator/packages.rs

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Partial error propagation: framework error dropped if both futures fail.

tokio::join! runs both futures to completion regardless of errors, then toolchain_res? is checked before framework_res?. If both fail, only the toolchain's error is surfaced and the framework/libs error (which may hold the actually relevant failure, e.g. a skeleton-libs extraction failure) is silently discarded.

🐛 Suggested fix: report both failures

- let (toolchain_res, framework_res) = tokio::join!(toolchain_fut, framework_fut);- toolchain_res?;- framework_res?;+ let (toolchain_res, framework_res) = tokio::join!(toolchain_fut, framework_fut);+ if let (Err(t_err), Err(f_err)) = (&toolchain_res, &framework_res) {+ tracing::error!("toolchain install also failed: {}", t_err);+ return Err(f_err_into_fbuild_error(f_err)); // or combine into a single error type+ }+ toolchain_res?;+ framework_res?;

🤖 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/src/esp32/orchestrator/packages.rs` around lines 110 -
112, The error handling after tokio::join! in the package orchestration flow
only propagates the toolchain result first, so a simultaneous framework/libs
failure can be lost. Update the logic around the join in packages.rs to preserve
and report the framework failure as well when both futures error, using the
existing toolchain_res and framework_res results from the join. Make sure the
implementation surfaces the more relevant error context instead of discarding
the second failure, especially for the framework/skeleton-libs extraction path.