[espidf] Install only the toolchains for the variants being built - #17688
Conversation
|
@esphbot review |
|
To use the changes in this PR: # Clone the repository:
git clone https://github.com/esphome/esphome
cd esphome
# Checkout the PR branch:
git fetch origin pull/17688/head:espidf-per-variant-targets
git checkout espidf-per-variant-targets
# Install the development version:
script/setup
# Activate the development version:
source venv/bin/activateNow you can run (Added by the PR bot) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #17688 +/- ##
==========================================
+ Coverage 86.49% 86.53% +0.04%
==========================================
Files 60 60
Lines 14137 14182 +45
Branches 2132 2141 +9
==========================================
+ Hits 12228 12273 +45
Misses 1596 1596
Partials 313 313
🚀 New features to boost your workflow:
|
Memory Impact AnalysisComponents:
This analysis runs automatically when components change. Memory usage is measured from a representative test configuration. |
|
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
|
@esphbot review |
|
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
|
@esphbot review |
|
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
aa165a0 to
33fb12f
Compare
3e8d1af to
74479b8
Compare
|
@esphbot review |
|
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
|
@esphbot review |
|
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
|
👋 Hi there! This PR modifies 1 file(s) with codeowners. @esphome/core - As codeowner(s) of the affected files, your review would be appreciated! 🙏 Note: Automatic review request may have failed, but you're still welcome to review. |
There was a problem hiding this comment.
Pull request overview
Optimizes ESP-IDF tool installation in ESPHome by restricting idf_tools.py install targets to the currently configured ESP32 variant (unless explicitly overridden), reducing unnecessary toolchain downloads and avoiding unnecessary Python venv rebuilds while keeping existing “all targets” installs valid.
Changes:
- Pass per-variant ESP-IDF targets (derived from
CORE.datavariant) intocheck_esp_idf_install, with an explicitESPHOME_IDF_DEFAULT_TARGETSoverride still forcingall. - Update framework install stamping logic to treat target supersets as valid, merge installed targets over time, and decouple Python venv rebuilds from toolchain installs.
- Add/extend unit tests covering configured targets, stamp coverage/merge behavior, and tools.json patching for demoting unused tools and pruning xtensa targets from
riscv32-esp-elf.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit_tests/test_espidf_toolchain.py | Adds unit tests for deriving IDF targets from the configured ESP32 variant and updated check_esp_idf_install call signature. |
| tests/unit_tests/test_espidf_framework.py | Adds test coverage for stamp superset semantics, target merge behavior, env override precedence, and tools.json patch behavior. |
| esphome/espidf/toolchain.py | Uses configured variant to compute targets passed to check_esp_idf_install, avoiding installing unused toolchains. |
| esphome/espidf/framework.py | Implements stamp reading and “covers” logic, merges installed targets, prevents venv rebuilds on tools-only installs, and refines target selection precedence. |
| esphome/espidf/component.py | Reuses shared variant_to_idf_target helper for extra script target mapping. |
| esphome/components/esp32/const.py | Adds shared variant_to_idf_target() mapping helper. |
Comments suppressed due to low confidence (1)
esphome/espidf/framework.py:257
_write_stamp()writes stamp dicts that include lists (e.g.targets,tools,features), sodata: dict[str, str]is no longer accurate. Widen the annotation to avoid incorrect typing and future confusion.
def _write_stamp(file: PathType, data: dict[str, str]):
|
Will cause issues with caching in CI. Right now the IDF cache is written from any clang tidy run. After this change it will only cache a single variant and won't warm all variants. This will result in only half the cache getting saved and then longer CI run times. |
…ll all targets in CI
|
@esphbot review |
Already handled. One gap worth closing: |
|
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
…ct comparison, log skipped riscv patch
|
@esphbot review |
|
@esphbot review |
|
@esphbot review |
|
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
|
@esphbot review |
PR Review — [espidf] Install only the toolchains for the variants being builtMerge-ready. Every finding from the previous round is fixed, and the one remaining note is a pre-existing consistency nit outside the diff. What I verified this round, specifically:
✅ Resolved since last review (1)Previously-flagged issues verified fixed
Checklist
ℹ️ Triage summary1 pre-existing finding(s) on unchanged code suppressed (freeze). Silent Failure Analysis
🟡 **MEDIUM** — unvalidated override silently discards caller input
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
|
Thanks |
What does this implement/fix?
Chained on #17687; #17686 below it has merged. ESPHome passes
--targets=alltoidf_tools.py install, so every user downloads both compiler families even though a config builds exactly one variant;riscv32-esp-elfis a 292 MB download and 2.0 GB on disk,xtensa-esp-elfis 177 MB and 1.1 GB. This passes the configured variant instead, so a user who only builds xtensa chips never downloads the riscv toolchain and vice versa, roughly halving the fresh install.Not intended for backport. Like #17687 this is purely an install size and speed optimization; #17686, already merged, is the one fixing a real reported issue.
How it works: the variant from
CORE.databecomes the--targets=argument (via a sharedvariant_to_idf_targethelper, also adopted by the extra script runner which had its own copy of the mapping).idf_tools.pyaccumulates targets in idf-env.json across runs, so building a second variant later installs just its toolchain incrementally while everything already present reports "already installed". The install stamp now records the union of installed targets and treats a superset as valid, so alternating builds of two variants never re-run the installer, and existing stamps holdingallstay valid forever, meaning current installs see zero churn on upgrade. The ESP-IDF python env rebuild is also decoupled from tool installs; it depends on the framework version and features, not on which toolchains are present, so adding a variant no longer wipes the venv. An explicitly setESPHOME_IDF_DEFAULT_TARGETSstill overrides everything, so a builder image can pre-warm all targets. CI keeps installing every target regardless of the variant a job builds, so the shared toolchain cache stays complete. As a separate saving, tools.json is patched to drop the xtensa chips fromriscv32-esp-elf's supported targets; that entry exists only for the S2/S3 ULP coprocessor, which ESPHome never builds for, so xtensa only installs skip the riscv compiler entirely.Verified end to end with the bundled idf_tools.py from IDF 5.5.5 in a fresh tools path:
install --targets=esp32 requireddownloads onlyxtensa-esp-elf, esp-rom-elfs, the tool path export succeeds against that install, and a follow upinstall --targets=esp32c3 requiredaccumulates both targets in idf-env.json, skips the already installed tools, and pulls only the riscv toolchain.Types of changes
Related issue or feature (if applicable):
Pull request in esphome.io with documentation (if applicable):
Test Environment
Example entry for
config.yaml:# Tooling change only, no configuration impactChecklist:
tests/folder).If user exposed functionality or configuration variables are added/changed: