From b893cee6cae70812ce4dbc451820fc0999164b4a Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 18:07:46 -0500 Subject: [PATCH] Apply simplify pass: shared idedata warning, platform-keyed backend table, one owner for the cache prefix and trigger files --- .github/actions/cache-arduino8266/action.yml | 21 +++---- .github/workflows/ci.yml | 5 -- esphome/__main__.py | 58 ++++++++----------- esphome/arduino8266/toolchain.py | 37 ++++-------- esphome/build_helpers/idedata.py | 20 +++++++ script/determine-jobs.py | 51 ++++++++-------- tests/script/test_determine_jobs.py | 47 ++++++++------- .../unit_tests/test_arduino8266_toolchain.py | 6 +- 8 files changed, 118 insertions(+), 127 deletions(-) diff --git a/.github/actions/cache-arduino8266/action.yml b/.github/actions/cache-arduino8266/action.yml index 6a33b0d1aa..b23db96769 100644 --- a/.github/actions/cache-arduino8266/action.yml +++ b/.github/actions/cache-arduino8266/action.yml @@ -2,10 +2,11 @@ name: Cache Arduino ESP8266 description: > Resolve the pinned Arduino core and xtensa toolchain versions and cache the native ESP8266 install (~110 MB framework + toolchain; no ccache store, the - seed job saves before any compile runs). Callers must set env - ESPHOME_ARDUINO8266_PREFIX: ~/.esphome-arduino8266 and have the Python venv - already restored. Mirrors cache-esp-idf: only dev-branch pushes write the - shared cache, everything else restores. + seed job saves before any compile runs). Exports + ESPHOME_ARDUINO8266_PREFIX to the job so every later step installs into + the cached path; the Python venv must already be restored. Mirrors + cache-esp-idf: only dev-branch pushes write the shared cache, everything + else restores. runs: using: composite steps: @@ -16,14 +17,10 @@ runs: id: version shell: bash run: | - # The caller's install prefix must match the cached path below, or - # the cache silently stores/restores an empty directory. Compare - # tilde-expanded: the job env carries a literal ~ that Python's - # expanduser and actions/cache both resolve. - [ "${ESPHOME_ARDUINO8266_PREFIX/#\~/$HOME}" = "$HOME/.esphome-arduino8266" ] || { - echo "ESPHOME_ARDUINO8266_PREFIX is '$ESPHOME_ARDUINO8266_PREFIX', expected '$HOME/.esphome-arduino8266'" >&2 - exit 1 - } + # One owner for the install prefix: exporting it here (instead of a + # per-job env stanza) makes it impossible for a caller to install + # into a path other than the one cached below. + echo "ESPHOME_ARDUINO8266_PREFIX=$HOME/.esphome-arduino8266" >> "$GITHUB_ENV" . venv/bin/activate key=$(python -c 'from esphome.components.esp8266 import RECOMMENDED_ARDUINO_FRAMEWORK_VERSION as f; from esphome.arduino8266.framework import TOOLCHAIN_VERSION as t; print(f"{f}-{t}")') [ -n "$key" ] || exit 1 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 488059ad42..4b90df54f9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -188,10 +188,6 @@ jobs: # toolchain and discard it. if: github.event_name == 'push' && github.ref == 'refs/heads/dev' timeout-minutes: 15 - env: - # The composite action and the install below agree on this path by - # construction, not by matching the Python-side default - ESPHOME_ARDUINO8266_PREFIX: ~/.esphome-arduino8266 steps: - name: Check out code from GitHub uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -1169,7 +1165,6 @@ jobs: env: # Computed by script/determine-jobs.py (ESP8266_NATIVE_TEST_COMPONENTS) TEST_COMPONENTS: ${{ needs.determine-jobs.outputs.esp8266-native-components }} - ESPHOME_ARDUINO8266_PREFIX: ~/.esphome-arduino8266 steps: - name: Check out code from GitHub uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 diff --git a/esphome/__main__.py b/esphome/__main__.py index 1875d23576..b74775524f 100644 --- a/esphome/__main__.py +++ b/esphome/__main__.py @@ -48,6 +48,7 @@ from esphome.const import ( ENV_NOGITIGNORE, KEY_ESP32, KEY_VARIANT, + NATIVE_TOOLCHAINS, SECRETS_FILES, Toolchain, ) @@ -815,11 +816,9 @@ def write_cpp_file() -> int: from esphome.build_gen import espidf espidf.write_project() - elif CORE.using_native_toolchain: - # Native builds generate their project at compile time; never write - # a platformio.ini - pass - else: + elif not CORE.using_native_toolchain: + # Other native builds generate their project at compile time; + # never write a platformio.ini for them from esphome.build_gen import platformio platformio.write_project() @@ -861,20 +860,9 @@ def compile_program(args: ArgsProtocol, config: ConfigType) -> int: toolchain.create_factory_bin() toolchain.create_ota_bin() toolchain.create_elf_copy() - from esphome.build_helpers.idedata import IDEDATA_BEST_EFFORT_ERRORS + from esphome.build_helpers.idedata import warn_if_idedata_missing - try: - if toolchain.get_idedata() is None: - _LOGGER.warning("No idedata was generated for this build") - except IDEDATA_BEST_EFFORT_ERRORS as err: - # The firmware already built; an idedata failure must not fail - # a successful build. - _LOGGER.warning( - "Could not generate idedata: %s (IDE, clang-tidy, and " - "memory-analysis data will be unavailable for this build)", - err, - ) - _LOGGER.debug("Idedata failure detail", exc_info=True) + warn_if_idedata_missing(toolchain.get_idedata) elif CORE.using_native_toolchain: raise EsphomeError( f"Toolchain '{CORE.toolchain.value}' resolved but no platform " @@ -1936,25 +1924,28 @@ def command_update_all(args: ArgsProtocol) -> int | None: return run_multiple_configs(files, build_command) -# Native build backend per toolchain; keep in sync with NATIVE_TOOLCHAINS -# in esphome.const. Keyed by toolchain rather than a platform hook so the -# serial upload/logs fast path never imports the platform component package -# (see the esp32 variant comment in upload_using_esptool). +# Native build backend per (target platform, toolchain). Keyed here rather +# than through a platform hook so the serial upload/logs fast path never +# imports the platform component package (see the esp32 variant comment in +# upload_using_esptool); the platform half comes from CORE.data the same way. _NATIVE_TOOLCHAIN_MODULES = { - Toolchain.ESP_IDF: "esphome.espidf.toolchain", - Toolchain.ARDUINO: "esphome.arduino8266.toolchain", + ("esp32", Toolchain.ESP_IDF): "esphome.espidf.toolchain", + ("esp8266", Toolchain.ARDUINO): "esphome.arduino8266.toolchain", } +# Structure over prose: a native toolchain the table does not serve is a bug +assert {tc for _, tc in _NATIVE_TOOLCHAIN_MODULES} == set(NATIVE_TOOLCHAINS) def _native_toolchain_module(): """The native build backend module for the resolved toolchain.""" if not CORE.using_native_toolchain: return None - if (module_path := _NATIVE_TOOLCHAIN_MODULES.get(CORE.toolchain)) is None: - # A native toolchain missing from the table is a bug; degrading to - # the PlatformIO path would build with the wrong backend + key = (CORE.target_platform, CORE.toolchain) + if (module_path := _NATIVE_TOOLCHAIN_MODULES.get(key)) is None: + # Degrading to the PlatformIO path would build with the wrong backend raise EsphomeError( - f"Toolchain '{CORE.toolchain.value}' has no native build backend module" + f"Toolchain '{CORE.toolchain.value}' has no native build backend " + f"module for platform {CORE.target_platform}" ) return importlib.import_module(module_path) @@ -2028,10 +2019,9 @@ def command_analyze_memory(args: ArgsProtocol, config: ConfigType) -> int: # Get idedata for analysis idedata = None if native_toolchain is not None: - for tool in ( - native_toolchain.get_objdump_path(), - native_toolchain.get_readelf_path(), - ): + objdump = native_toolchain.get_objdump_path() + readelf = native_toolchain.get_readelf_path() + for tool in (objdump, readelf): if not tool.is_file(): # The analyzer would silently fall back to host binutils, # which cannot read the target ELF. clean-all is heavy for @@ -2042,8 +2032,8 @@ def command_analyze_memory(args: ArgsProtocol, config: ConfigType) -> int: tool, ) return 1 - objdump_path = str(native_toolchain.get_objdump_path()) - readelf_path = str(native_toolchain.get_readelf_path()) + objdump_path = str(objdump) + readelf_path = str(readelf) firmware_elf = native_toolchain.get_elf_path() if not firmware_elf.is_file(): diff --git a/esphome/arduino8266/toolchain.py b/esphome/arduino8266/toolchain.py index 3e6fd2a183..ad7134cb8a 100644 --- a/esphome/arduino8266/toolchain.py +++ b/esphome/arduino8266/toolchain.py @@ -101,9 +101,7 @@ def run_compile(config: ConfigType, verbose: bool) -> int: if ( ninja_changed or not compdb.is_file() - or ( - ninja_file.is_file() and compdb.stat().st_mtime < ninja_file.stat().st_mtime - ) + or compdb.stat().st_mtime < ninja_file.stat().st_mtime ): _write_compile_commands(paths.ninja, build_dir, env) @@ -149,9 +147,9 @@ def run_compile(config: ConfigType, verbose: bool) -> int: # the factory/ota copies are what upload and OTA actually consume build_dir_artifacts = ( get_elf_path(), - get_build_dir() / "firmware.bin", + build_dir / "firmware.bin", get_factory_firmware_path(), - get_build_dir() / "firmware.ota.bin", + build_dir / "firmware.ota.bin", ) for artifact in build_dir_artifacts: if not artifact.is_file(): @@ -162,31 +160,16 @@ def run_compile(config: ConfigType, verbose: bool) -> int: # The cause was already warned; name the consequence so a build # contributing no RAM/Flash metric is visible to CI harnesses _LOGGER.warning("Firmware size summary unavailable for this build") - from esphome.build_helpers.idedata import IDEDATA_BEST_EFFORT_ERRORS + from esphome.build_helpers.idedata import warn_if_idedata_missing - try: - idedata = get_idedata(ccache) - except IDEDATA_BEST_EFFORT_ERRORS as err: - # Broad on purpose: idedata is a bonus artifact; nothing here may - # fail a successful build. - _LOGGER.warning( - "Could not generate idedata: %s (IDE, clang-tidy, and " - "memory-analysis data will be unavailable for this build)", - err, - ) - _LOGGER.debug("Idedata failure detail", exc_info=True) - else: - if idedata is None: - _LOGGER.warning( - "Could not generate idedata from %s", - build_dir / "compile_commands.json", - ) + warn_if_idedata_missing(lambda: get_idedata(ccache)) return 0 def _write_compile_commands( ninja_path: Path, build_dir: Path, env: dict[str, str] ) -> None: + compdb = build_dir / "compile_commands.json" result = subprocess.run( [str(ninja_path), "-C", str(build_dir), "-t", "compdb", "c", "cxx", "asm"], env=env, @@ -198,12 +181,12 @@ def _write_compile_commands( if result.returncode != 0: # Drop any stale database so consumers (IDE integration, clang-tidy, # the memory analyzer) can't silently read outdated data. - (build_dir / "compile_commands.json").unlink(missing_ok=True) + compdb.unlink(missing_ok=True) raise EsphomeError(f"Could not generate compile_commands.json: {result.stderr}") try: entries = json.loads(result.stdout) except ValueError as err: - (build_dir / "compile_commands.json").unlink(missing_ok=True) + compdb.unlink(missing_ok=True) raise EsphomeError( f"ninja produced an unparsable compile database: {err} " f"(output starts {result.stdout[:120]!r})" @@ -211,14 +194,14 @@ def _write_compile_commands( if not entries: # compdb exits 0 with [] for unknown rule names; a renamed compile # rule must fail the build, not silently strand every consumer - (build_dir / "compile_commands.json").unlink(missing_ok=True) + compdb.unlink(missing_ok=True) raise EsphomeError( "ninja produced an empty compile database; the generator's rule " "names no longer match" ) # write_file_if_changed keeps the mtime stable on no-op builds so the # idedata cache in get_idedata() stays valid. - write_file_if_changed(build_dir / "compile_commands.json", result.stdout) + write_file_if_changed(compdb, result.stdout) def _parse_app_size(build_dir: Path, paths: framework.InstalledPaths) -> int | None: diff --git a/esphome/build_helpers/idedata.py b/esphome/build_helpers/idedata.py index 038fe64970..7edd5a76ff 100644 --- a/esphome/build_helpers/idedata.py +++ b/esphome/build_helpers/idedata.py @@ -11,6 +11,7 @@ consumers (IDE integration, clang-tidy) expect: from __future__ import annotations +from collections.abc import Callable import json import logging import os @@ -31,6 +32,25 @@ IDEDATA_BEST_EFFORT_ERRORS = ( ValueError, ) + +def warn_if_idedata_missing(get_idedata: Callable[[], dict | None]) -> None: + """Run an idedata generator, downgrading any failure to a warning. + + Shared by the native backends: the firmware already built, so a missing + or broken idedata must not fail a successful build. + """ + try: + if get_idedata() is None: + _LOGGER.warning("No idedata was generated for this build") + except IDEDATA_BEST_EFFORT_ERRORS as err: + _LOGGER.warning( + "Could not generate idedata: %s (IDE, clang-tidy, and " + "memory-analysis data will be unavailable for this build)", + err, + ) + _LOGGER.debug("Idedata failure detail", exc_info=True) + + _LOGGER = logging.getLogger(__name__) # C++ translation-unit suffixes used to identify ESPHome source files. diff --git a/script/determine-jobs.py b/script/determine-jobs.py index d507fff1d7..0e4d796d1f 100755 --- a/script/determine-jobs.py +++ b/script/determine-jobs.py @@ -52,7 +52,6 @@ import argparse from collections import Counter from collections.abc import Callable from enum import StrEnum -import functools from functools import cache import json import os @@ -506,14 +505,19 @@ ESP32_PLATFORMIO_TRIGGER_PATH_PREFIXES = ("esphome/platformio/",) # - esphome/build_gen/platformio.py -- the PlatformIO build generator # - script/test_build_components.py -- the harness the job invokes # - .github/workflows/ci.yml -- the job's own definition -ESP32_PLATFORMIO_TRIGGER_FILES = frozenset( +# Shared by every toolchain smoke-test job: the harness it invokes and the +# workflow that defines it +_SMOKE_HARNESS_TRIGGER_FILES = frozenset( { - "esphome/build_gen/platformio.py", "script/test_build_components.py", ".github/workflows/ci.yml", } ) +ESP32_PLATFORMIO_TRIGGER_FILES = _SMOKE_HARNESS_TRIGGER_FILES | { + "esphome/build_gen/platformio.py", +} + def _path_or_file_trigger( files: list[str], @@ -526,17 +530,14 @@ def _path_or_file_trigger( ) -@functools.lru_cache +@cache def _cached_components_closure(files: tuple[str, ...]) -> frozenset[str]: - """The dependency closure walk is expensive; every toolchain smoke-test - job asks for the same file list, so compute it once per run.""" - return frozenset(_changed_components_closure(list(files))) + """Dependency closure of the changed components, from the changed files. - -def _changed_components_closure(files: list[str]) -> set[str]: - """Dependency closure of the changed components, from the changed files.""" + The walk is expensive and every toolchain smoke-test job asks for the + same file list, so compute it once per run.""" component_files = [f for f in files if filter_component_and_test_files(f)] - return set(get_components_with_dependencies(component_files, True)) + return frozenset(get_components_with_dependencies(component_files, True)) def _esp32_platformio_path_or_file_trigger(files: list[str]) -> bool: @@ -617,7 +618,7 @@ def esp32_platformio_components_to_test(branch: str | None = None) -> list[str]: def _toolchain_components_to_test( branch: str | None, - test_set: frozenset[str] | set[str], + test_set: frozenset[str], infra_trigger: Callable[[list[str]], bool], ) -> list[str]: """The shared narrowing rule for the per-toolchain smoke-test jobs.""" @@ -671,18 +672,20 @@ ESP8266_NATIVE_TRIGGER_PATH_PREFIXES = ( "esphome/arduino/", "esphome/build_helpers/", ) -ESP8266_NATIVE_TRIGGER_FILES = _NATIVE_SHARED_TRIGGER_FILES | { - "esphome/build_gen/arduino8266.py", - "esphome/build_gen/build_tool.py", - "esphome/components/esp8266/build_surgery.py", - "esphome/components/esp8266/boards.py", - "esphome/platformio/registry.py", - # esp8266/__init__.py imports copy_ccache_script from it - "esphome/platformio/toolchain.py", - "script/test_build_components.py", - ".github/workflows/ci.yml", - ".github/actions/cache-arduino8266/action.yml", -} +ESP8266_NATIVE_TRIGGER_FILES = ( + _NATIVE_SHARED_TRIGGER_FILES + | _SMOKE_HARNESS_TRIGGER_FILES + | { + "esphome/build_gen/arduino8266.py", + "esphome/build_gen/build_tool.py", + "esphome/components/esp8266/build_surgery.py", + "esphome/components/esp8266/boards.py", + "esphome/platformio/registry.py", + # esp8266/__init__.py imports copy_ccache_script from it + "esphome/platformio/toolchain.py", + ".github/actions/cache-arduino8266/action.yml", + } +) def _esp8266_native_path_or_file_trigger(files: list[str]) -> bool: diff --git a/tests/script/test_determine_jobs.py b/tests/script/test_determine_jobs.py index edd12d8d14..861081dcaf 100644 --- a/tests/script/test_determine_jobs.py +++ b/tests/script/test_determine_jobs.py @@ -3111,33 +3111,36 @@ def test_memory_impact_elf_layouts_are_found(tmp_path: Path) -> None: assert find_elf_path(build_path) == elf, f"{platform} ELF not found" -def test_esp8266_native_components_full_list_on_infra_change() -> None: - """Native-ESP8266 infrastructure changes run the full test list.""" - for changed in ( - ["esphome/arduino8266/framework.py"], - ["esphome/build_gen/arduino8266.py"], - ["esphome/components/esp8266/build_surgery.py"], +@pytest.mark.parametrize( + "changed", + [ + "esphome/arduino8266/framework.py", + "esphome/build_gen/arduino8266.py", + "esphome/components/esp8266/build_surgery.py", # Shared modules the native build depends on - ["esphome/build_helpers/idedata.py"], - ["esphome/platformio/library.py"], + "esphome/build_helpers/idedata.py", + "esphome/platformio/library.py", # Top-level esphome/*.py modules the backend imports directly - ["esphome/framework_helpers.py"], - ["esphome/writer.py"], + "esphome/framework_helpers.py", + "esphome/writer.py", # esp8266/__init__.py imports copy_ccache_script from it - ["esphome/platformio/toolchain.py"], + "esphome/platformio/toolchain.py", # The composite cache action must not ship unexercised - [".github/actions/cache-arduino8266/action.yml"], + ".github/actions/cache-arduino8266/action.yml", + ], +) +def test_esp8266_native_components_full_list_on_infra_change(changed: str) -> None: + """Native-ESP8266 infrastructure changes run the full test list.""" + with ( + patch.object(determine_jobs, "changed_files", return_value=[changed]), + patch.object( + determine_jobs, + "get_components_with_dependencies", + return_value=["wifi"], + ), ): - with ( - patch.object(determine_jobs, "changed_files", return_value=changed), - patch.object( - determine_jobs, - "get_components_with_dependencies", - return_value=["wifi"], - ), - ): - result = determine_jobs.esp8266_native_components_to_test() - assert result == sorted(determine_jobs.ESP8266_NATIVE_TEST_COMPONENTS) + result = determine_jobs.esp8266_native_components_to_test() + assert result == sorted(determine_jobs.ESP8266_NATIVE_TEST_COMPONENTS) @pytest.mark.parametrize( diff --git a/tests/unit_tests/test_arduino8266_toolchain.py b/tests/unit_tests/test_arduino8266_toolchain.py index dbeb650214..82f950e7fa 100644 --- a/tests/unit_tests/test_arduino8266_toolchain.py +++ b/tests/unit_tests/test_arduino8266_toolchain.py @@ -147,8 +147,6 @@ def test_run_compile_noop_skips_the_build_spawn(tmp_path: Path) -> None: def test_run_compile_regenerates_stale_compdb(tmp_path: Path) -> None: """An interrupted run can leave build.ninja newer than the compile DB; mere existence must not skip regeneration.""" - import os - build_dir = toolchain.get_build_dir() build_dir.mkdir(parents=True, exist_ok=True) (build_dir / "build.ninja").write_text("") @@ -246,7 +244,7 @@ def test_run_compile_warns_when_idedata_fails( patch.object(toolchain, "get_idedata", return_value=None), ): assert toolchain.run_compile({CONF_ESPHOME: {}}, verbose=False) == 0 - assert "Could not generate idedata" in caplog.text + assert "No idedata was generated for this build" in caplog.text def test_write_compile_commands(tmp_path: Path) -> None: @@ -422,6 +420,8 @@ def test_run_compile_skips_compdb_when_ninja_unchanged(tmp_path: Path) -> None: """An unchanged build.ninja means the compile DB is already current.""" build_dir = toolchain.get_build_dir() build_dir.mkdir(parents=True, exist_ok=True) + # write_project (stubbed below) always leaves a build.ninja behind + (build_dir / "build.ninja").write_text("# manifest") def run(regenerate_expected: bool) -> None: with (