From d344fd74d1c8ce7c3242d93e483ec030b084548a Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 4 Aug 2026 14:47:07 -0500 Subject: [PATCH] [core] Read the esp32 variant for esptool without importing the esp32 package (#18067) --- esphome/__main__.py | 9 ++-- esphome/storage_json.py | 6 +-- .../fixtures/lazy_imports/_leak_report.py | 19 ++++++++ .../lazy_imports/esptool_upload_fast_path.py | 45 +++++++++++++++++++ .../lazy_imports/storage_json_fast_path.py | 12 ++--- tests/unit_tests/test_lazy_imports.py | 42 ++++++++++++----- tests/unit_tests/test_main.py | 13 +++++- 7 files changed, 120 insertions(+), 26 deletions(-) create mode 100644 tests/unit_tests/fixtures/lazy_imports/_leak_report.py create mode 100644 tests/unit_tests/fixtures/lazy_imports/esptool_upload_fast_path.py diff --git a/esphome/__main__.py b/esphome/__main__.py index 2b7e1ac85c..d31d3e9399 100644 --- a/esphome/__main__.py +++ b/esphome/__main__.py @@ -48,6 +48,8 @@ from esphome.const import ( CONF_WEB_SERVER, CONF_WIFI, ENV_NOGITIGNORE, + KEY_ESP32, + KEY_VARIANT, SECRETS_FILES, Toolchain, ) @@ -923,9 +925,10 @@ def upload_using_esptool( mcu = "esp8266" if CORE.is_esp32: - from esphome.components.esp32 import get_esp32_variant - - mcu = get_esp32_variant().lower() + # Same lookup as esp32.get_esp32_variant(), read directly so the + # serial upload path does not import the esp32 package; both the + # validator and the warm-cache apply_to_core populate this key. + mcu = CORE.data[KEY_ESP32][KEY_VARIANT].lower() line_callbacks: list[Callable[[str], str | None]] = [] if ( diff --git a/esphome/storage_json.py b/esphome/storage_json.py index 2aa76aabaa..2ba26ec711 100644 --- a/esphome/storage_json.py +++ b/esphome/storage_json.py @@ -329,9 +329,9 @@ class StorageJSON: } # The compile pipeline populates CORE.data[KEY_ESP32] when esp32's # validator runs; on the cache fast path that validator is skipped, - # so populate the variant upload_using_esptool reads via - # esp32.get_esp32_variant(). target_platform on disk is the variant - # (e.g. "ESP32S3"); core_platform is the family (e.g. "esp32"). + # so populate the variant upload_using_esptool reads from + # CORE.data[KEY_ESP32][KEY_VARIANT]. target_platform on disk is the + # variant (e.g. "ESP32S3"); core_platform is the family (e.g. "esp32"). if target_platform == const.PLATFORM_ESP32: esp32_data = {KEY_VARIANT: self.target_platform} if self.framework_version: diff --git a/tests/unit_tests/fixtures/lazy_imports/_leak_report.py b/tests/unit_tests/fixtures/lazy_imports/_leak_report.py new file mode 100644 index 0000000000..00d387cd04 --- /dev/null +++ b/tests/unit_tests/fixtures/lazy_imports/_leak_report.py @@ -0,0 +1,19 @@ +"""Shared tail for the lazy-import fixture scripts.""" + +import sys + + +def print_leaked_modules() -> None: + """Report argv-listed heavy modules (plus any component package) loaded. + + Any component package counts as a leak, not just the ones on the + watch list: executing one drags in codegen/validation machinery by + design. + """ + leaked = [module for module in sys.argv[1:] if module in sys.modules] + leaked += [ + module + for module in sys.modules + if module.startswith("esphome.components.") and module not in leaked + ] + print(",".join(leaked)) diff --git a/tests/unit_tests/fixtures/lazy_imports/esptool_upload_fast_path.py b/tests/unit_tests/fixtures/lazy_imports/esptool_upload_fast_path.py new file mode 100644 index 0000000000..e622948aed --- /dev/null +++ b/tests/unit_tests/fixtures/lazy_imports/esptool_upload_fast_path.py @@ -0,0 +1,45 @@ +"""Run the esptool serial-upload path and report which heavy modules loaded. + +Executed as a subprocess by test_lazy_imports.py: heavy module names come +in on argv, the ones found in sys.modules afterwards go out on stdout. +The variant reaches the esptool command line from CORE.data directly; if +someone re-adds the esp32 package import for it, this reports the leak. +""" + +import os +import sys +from unittest.mock import patch + +from _leak_report import print_leaked_modules + +from esphome.__main__ import upload_using_esptool +from esphome.const import ( + CONF_ESPHOME, + KEY_CORE, + KEY_ESP32, + KEY_TARGET_PLATFORM, + KEY_VARIANT, +) +from esphome.core import CORE + +# An ambient ESPHOME_USE_SUBPROCESS would route past the patched +# run_external_command into run_external_process and confuse the checks. +os.environ.pop("ESPHOME_USE_SUBPROCESS", None) + +CORE.data[KEY_CORE] = {KEY_TARGET_PLATFORM: "esp32"} +CORE.data[KEY_ESP32] = {KEY_VARIANT: "ESP32S3"} + +with patch("esphome.__main__.run_external_command", return_value=0) as mock_run: + rc = upload_using_esptool( + {CONF_ESPHOME: {"platformio_options": {}}}, "/dev/ttyUSB0", "firmware.bin", None + ) + +# Fail loudly if the upload path stopped doing its work; otherwise an +# empty leak list could just mean nothing ran. +if rc != 0: + sys.exit(f"upload_using_esptool returned {rc}") +cmd = list(mock_run.call_args[0][1:]) +if cmd[cmd.index("--chip") + 1] != "esp32s3": + sys.exit(f"variant did not reach the esptool command line: {cmd}") + +print_leaked_modules() diff --git a/tests/unit_tests/fixtures/lazy_imports/storage_json_fast_path.py b/tests/unit_tests/fixtures/lazy_imports/storage_json_fast_path.py index 01b23b8f04..f83a398cda 100644 --- a/tests/unit_tests/fixtures/lazy_imports/storage_json_fast_path.py +++ b/tests/unit_tests/fixtures/lazy_imports/storage_json_fast_path.py @@ -6,6 +6,8 @@ in on argv, the ones found in sys.modules afterwards go out on stdout. import sys +from _leak_report import print_leaked_modules + from esphome.const import KEY_ESP32, KEY_IDF_VERSION, KEY_VARIANT from esphome.core import CORE, Version from esphome.storage_json import StorageJSON @@ -41,12 +43,4 @@ if esp32_data.get(KEY_VARIANT) != "ESP32S3": if esp32_data.get(KEY_IDF_VERSION) != Version(5, 3, 1): sys.exit(f"apply_to_core did not parse the framework version: {esp32_data!r}") -# Any component package counts as a leak, not just the ones on the watch -# list: executing one drags in codegen/validation machinery by design. -leaked = [module for module in sys.argv[1:] if module in sys.modules] -leaked += [ - module - for module in sys.modules - if module.startswith("esphome.components.") and module not in leaked -] -print(",".join(leaked)) +print_leaked_modules() diff --git a/tests/unit_tests/test_lazy_imports.py b/tests/unit_tests/test_lazy_imports.py index e9015e129f..27e102a1f9 100644 --- a/tests/unit_tests/test_lazy_imports.py +++ b/tests/unit_tests/test_lazy_imports.py @@ -78,16 +78,13 @@ def test_watched_heavy_modules_exist() -> None: ) -def test_storage_json_fast_path_does_not_import_heavy_modules( - fixture_path: Path, -) -> None: - """``apply_to_core`` runs on the upload/logs fast path for every - platform; parsing the stored framework version must not drag in the - validation stack or the esp32 component package. +def _leaked_from_fixture(fixture_path: Path, script_name: str) -> str: + """Run a fixture script with the watched modules on argv. + + Running a script file drops the cwd from sys.path, so prepend the + repo root for the child; a non-zero exit surfaces the child's stderr. """ - script = fixture_path / "lazy_imports" / "storage_json_fast_path.py" - # Running a script file drops the cwd from sys.path, so prepend the - # repo root for the child; check=False keeps its stderr visible. + script = fixture_path / "lazy_imports" / script_name python_path = str(Path(__file__).parents[2]) if ambient := os.environ.get("PYTHONPATH"): python_path = os.pathsep.join((python_path, ambient)) @@ -100,7 +97,17 @@ def test_storage_json_fast_path_does_not_import_heavy_modules( check=False, ) assert result.returncode == 0, result.stderr - leaked = result.stdout.strip() + return result.stdout.strip() + + +def test_storage_json_fast_path_does_not_import_heavy_modules( + fixture_path: Path, +) -> None: + """``apply_to_core`` runs on the upload/logs fast path for every + platform; parsing the stored framework version must not drag in the + validation stack or the esp32 component package. + """ + leaked = _leaked_from_fixture(fixture_path, "storage_json_fast_path.py") assert not leaked, ( f"storage_json.apply_to_core pulls in heavy modules: {leaked}. " "The upload/logs fast path skips validation; importing the " @@ -108,6 +115,21 @@ def test_storage_json_fast_path_does_not_import_heavy_modules( ) +def test_esptool_upload_fast_path_does_not_import_heavy_modules( + fixture_path: Path, +) -> None: + """The esptool serial upload reads the esp32 variant from CORE.data; + resolving it must not drag in the esp32 component package or the + validation stack. + """ + leaked = _leaked_from_fixture(fixture_path, "esptool_upload_fast_path.py") + assert not leaked, ( + f"upload_using_esptool pulls in heavy modules: {leaked}. " + "The upload fast path skips validation; importing the validation " + "stack anyway defeats the validated-config cache." + ) + + def test_api_client_does_not_import_heavy_modules() -> None: """``esphome.api_client`` is on the logs fast path and must stay light. diff --git a/tests/unit_tests/test_main.py b/tests/unit_tests/test_main.py index 1ca78e1924..9bd09eed32 100644 --- a/tests/unit_tests/test_main.py +++ b/tests/unit_tests/test_main.py @@ -64,7 +64,12 @@ from esphome.__main__ import ( from esphome.address_cache import AddressCache from esphome.bundle import BUNDLE_EXTENSION, BundleFile, BundleResult from esphome.components import esp32 -from esphome.components.esp32 import KEY_ESP32, KEY_VARIANT, VARIANT_ESP32 +from esphome.components.esp32 import ( + KEY_ESP32, + KEY_VARIANT, + VARIANT_ESP32, + get_esp32_variant, +) from esphome.const import ( CONF_API, CONF_AUTH, @@ -1622,6 +1627,12 @@ def test_upload_using_esptool_path_conversion( assert isinstance(partitions_path, str) assert partitions_path.endswith("partitions.bin") + # The chip argument must track get_esp32_variant: upload_using_esptool + # reads CORE.data directly to avoid the esp32 package import, and the + # two resolutions must not drift. + chip = cmd_list[cmd_list.index("--chip") + 1] + assert chip == get_esp32_variant().lower() + def test_upload_using_esptool_skips_missing_extra_flash_images( tmp_path: Path,