From 524b278811c19bdb2223a5a9e68871c6b0c09f4c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 5 Aug 2026 19:09:00 -0500 Subject: [PATCH] [core] Defer stdlib imports out of the upload and logs fast path (#18105) --- esphome/__main__.py | 12 ++++-- esphome/api_client.py | 3 +- esphome/helpers.py | 15 +++++-- esphome/storage_json.py | 9 +++- esphome/util.py | 6 ++- .../lazy_imports/upload_command_fast_path.py | 18 +++++++- tests/unit_tests/test_lazy_imports.py | 42 ++++++++++++++++--- tests/unit_tests/test_main.py | 25 +++++++++++ tests/unit_tests/test_util.py | 16 +++---- 9 files changed, 121 insertions(+), 25 deletions(-) diff --git a/esphome/__main__.py b/esphome/__main__.py index 30aa48ddbe..f435b18bb3 100644 --- a/esphome/__main__.py +++ b/esphome/__main__.py @@ -2,16 +2,12 @@ import argparse from collections.abc import Callable from contextlib import suppress -from datetime import datetime import functools -import getpass import importlib import logging import os from pathlib import Path import re -import shutil -import subprocess import sys import time from typing import Protocol @@ -621,6 +617,8 @@ def _resolve_network_devices( def run_miniterm(config: ConfigType, port: str, args) -> int: + from datetime import datetime + from aioesphomeapi import LogParser import serial @@ -977,6 +975,8 @@ def upload_using_esptool( def upload_using_platformio(config: ConfigType, port: str) -> int: + import shutil + from esphome.platformio import toolchain # RP2040 platform-raspberrypi build recipe expects firmware.bin.signed for @@ -1014,6 +1014,8 @@ def upload_using_picotool(config: ConfigType) -> int: the mass storage copy approach that causes "disk not ejected properly" warnings on macOS. """ + import subprocess + from esphome.platformio import toolchain idedata = toolchain.get_idedata(config) @@ -1120,6 +1122,8 @@ def check_permissions(port: str): "the USB cable can be used for data and is not a power-only cable." ) if not (os.access(port, os.R_OK | os.W_OK)): + import getpass + raise EsphomeError( "You do not have read or write permission on the selected serial port. " "To resolve this issue, you can add your user to the dialout group " diff --git a/esphome/api_client.py b/esphome/api_client.py index b9a71a3ff7..a75f219b17 100644 --- a/esphome/api_client.py +++ b/esphome/api_client.py @@ -2,7 +2,6 @@ from __future__ import annotations import asyncio from contextlib import suppress -from datetime import datetime import logging from typing import TYPE_CHECKING, Any import warnings @@ -35,6 +34,8 @@ async def async_run_logs( subscribe_states: bool = True, ) -> None: """Run the logs command in the event loop.""" + from datetime import datetime + conf = config["api"] name = config["esphome"]["name"] port: int = int(conf[CONF_PORT]) diff --git a/esphome/helpers.py b/esphome/helpers.py index 5458f5edfc..15d9797ce1 100644 --- a/esphome/helpers.py +++ b/esphome/helpers.py @@ -8,12 +8,9 @@ import os from pathlib import Path import platform import re -import shutil import stat import sys -import tempfile from typing import TYPE_CHECKING, TextIO -from urllib.parse import urlparse from esphome.const import __version__ as ESPHOME_VERSION @@ -281,6 +278,9 @@ def resolve_ip_address( hosts = host else: if not is_ip_address(host): + # Deferred: upload/logs with an IP target never parse a URL. + from urllib.parse import urlparse + url = urlparse(host) if url.scheme != "": host = url.hostname @@ -432,6 +432,8 @@ def rmtree(path: Path | str) -> None: read-only flag and retrying. """ + import shutil + def _onexc(func, path, exc): if os.access(path, os.W_OK): raise exc @@ -469,6 +471,11 @@ def _write_file( Automatically creates all parent directories. """ + # Deferred: a cache-hit upload/logs run never writes a file; keep the + # tempfile/shutil chain (bz2, lzma, random) off that path. + import shutil + import tempfile + data = text if isinstance(text, str): data = text.encode() @@ -544,6 +551,8 @@ def copy_file_if_changed(src: Path, dst: Path) -> bool: Returns True if file was copied, False if files already matched. """ + import shutil + if file_compare(src, dst): return False dst.parent.mkdir(parents=True, exist_ok=True) diff --git a/esphome/storage_json.py b/esphome/storage_json.py index 2ba26ec711..a90a36b848 100644 --- a/esphome/storage_json.py +++ b/esphome/storage_json.py @@ -1,11 +1,11 @@ from __future__ import annotations import binascii -from datetime import datetime import json import logging import os from pathlib import Path +from typing import TYPE_CHECKING from esphome import const from esphome.const import ( @@ -24,6 +24,9 @@ from esphome.core import CORE, EsphomeError, Version from esphome.helpers import write_file_if_changed from esphome.types import CoreType +if TYPE_CHECKING: + from datetime import datetime + _LOGGER = logging.getLogger(__name__) @@ -372,6 +375,10 @@ class EsphomeStorageJSON: @property def last_update_check(self) -> datetime | None: + # Deferred: this module is on the upload/logs fast path; only the + # dashboard's update check touches these accessors. + from datetime import datetime + try: # Stored format is naive ISO without %z; preserved for backward compat. return datetime.strptime( # noqa: DTZ007 diff --git a/esphome/util.py b/esphome/util.py index 4a5986a90d..136d6362f2 100644 --- a/esphome/util.py +++ b/esphome/util.py @@ -5,7 +5,6 @@ import io import logging from pathlib import Path import re -import subprocess import sys from typing import TYPE_CHECKING, Any @@ -289,6 +288,9 @@ def run_external_command( def run_external_process(*cmd: str, **kwargs: Any) -> int | str: + # Deferred: an OTA upload/logs run never spawns an external process. + import subprocess + full_cmd = " ".join(shlex_quote(x) for x in cmd) _LOGGER.debug("Running: %s", full_cmd) filter_lines = kwargs.get("filter_lines") @@ -443,6 +445,8 @@ def detect_rp2040_bootsel(picotool_path: str | Path) -> BootselResult: Returns a BootselResult with the number of devices found (by counting 'type:' lines in output), and whether a permission error was detected. """ + import subprocess + try: result = subprocess.run( [str(picotool_path), "info", "-d"], diff --git a/tests/unit_tests/fixtures/lazy_imports/upload_command_fast_path.py b/tests/unit_tests/fixtures/lazy_imports/upload_command_fast_path.py index c03b89c33f..f0df08aa4e 100644 --- a/tests/unit_tests/fixtures/lazy_imports/upload_command_fast_path.py +++ b/tests/unit_tests/fixtures/lazy_imports/upload_command_fast_path.py @@ -18,7 +18,12 @@ from _leak_report import print_leaked_modules from _storage import make_storage import yaml -from esphome import __main__ as main_mod +# Everything imported past this point is the code under test; the pop +# below must only drop what the setup itself preloaded, or it would +# hide modules the dispatch chain pulls in (tarfile has no other guard). +_FIXTURE_PRELOADED = frozenset(sys.modules) + +from esphome import __main__ as main_mod # noqa: E402 CONFIG_TEXT = "esphome:\n name: t\n" @@ -50,6 +55,17 @@ with tempfile.TemporaryDirectory() as _td: dispatched["config"] = config return 0 + # This setup pre-imports some watched stdlib modules (tempfile above, + # write_file inside make_storage().save(), unittest.mock -> asyncio -> + # subprocess). Drop exactly those so only a genuine dispatch-time + # re-import is reported; live objects keep their references, so + # cleanup still works. Module-level re-imports are out of reach here + # (esphome.__main__ is already loaded) — the bare-import check in + # test_lazy_imports owns that contract. + for module in sys.argv[1:]: + if module in _FIXTURE_PRELOADED: + sys.modules.pop(module, None) + with patch.dict(main_mod.POST_CONFIG_ACTIONS, {"upload": fake_upload}): exit_code = main_mod.run_esphome( ["esphome", "upload", str(conf_path), "--device", "192.0.2.1"] diff --git a/tests/unit_tests/test_lazy_imports.py b/tests/unit_tests/test_lazy_imports.py index 764f2862da..8358f4b781 100644 --- a/tests/unit_tests/test_lazy_imports.py +++ b/tests/unit_tests/test_lazy_imports.py @@ -46,6 +46,22 @@ API_HEAVY_MODULES = ("aioesphomeapi",) # never pays for the bundle machinery and its tarfile chain. BUNDLE_HEAVY_MODULES = ("esphome.bundle", "tarfile") +# Stdlib modules deferred out of the dispatch fast path: a cache-hit +# upload/logs run never writes a file (tempfile), spawns a process +# (subprocess), parses a URL (urllib.parse), or prints a serial +# permission hint (getpass). shutil is deferred too but unwatchable: +# argparse imports it from every add_argument on py3.14. urllib.parse +# is only watchable on 3.13+ where pathlib stopped importing it. +STDLIB_FAST_PATH_MODULES = ( + "tempfile", + "subprocess", + "getpass", + # Pins the module-level contract only: PyYAML's constructor loads + # datetime during the cache parse until the JSON cache lands. + "datetime", + *(("urllib.parse",) if sys.version_info >= (3, 13) else ()), +) + def _leaked_heavy_modules(module: str, extra: tuple[str, ...] = ()) -> str: """Import ``module`` in a subprocess and report the heavy modules it pulled. @@ -70,8 +86,14 @@ def _leaked_heavy_modules(module: str, extra: tuple[str, ...] = ()) -> str: def test_main_module_does_not_import_heavy_modules() -> None: - """A bare ``import esphome.__main__`` must not drag in validation/codegen.""" - leaked = _leaked_heavy_modules("esphome.__main__") + """A bare ``import esphome.__main__`` must not drag in validation/codegen. + + The stdlib watch list rides along here because this check runs in a + clean subprocess: a module-level re-import anywhere on the chain is + caught, which the dispatch fixture (whose setup pre-imports them and + pops before dispatch) structurally cannot do. + """ + leaked = _leaked_heavy_modules("esphome.__main__", extra=STDLIB_FAST_PATH_MODULES) assert not leaked, ( f"esphome.__main__ imports heavy modules at top level: {leaked}. " "Import them lazily inside the command that needs them instead; " @@ -82,7 +104,12 @@ def test_main_module_does_not_import_heavy_modules() -> None: def test_watched_heavy_modules_exist() -> None: """A renamed heavy module would silently disable the leak checks.""" - for module in FAST_PATH_HEAVY_MODULES + API_HEAVY_MODULES + BUNDLE_HEAVY_MODULES: + for module in ( + FAST_PATH_HEAVY_MODULES + + API_HEAVY_MODULES + + BUNDLE_HEAVY_MODULES + + STDLIB_FAST_PATH_MODULES + ): assert importlib.util.find_spec(module) is not None, ( f"{module} no longer resolves; update the heavy-module lists" ) @@ -241,12 +268,15 @@ def test_upload_command_path_does_not_import_heavy_modules( and its tarfile chain. """ leaked = _leaked_from_fixture( - fixture_path, "upload_command_fast_path.py", extra=BUNDLE_HEAVY_MODULES + fixture_path, + "upload_command_fast_path.py", + extra=BUNDLE_HEAVY_MODULES + STDLIB_FAST_PATH_MODULES, ) assert not leaked, ( f"the upload dispatch path pulls in heavy modules: {leaked}. " "An ordinary run only needs the bundle suffix constant, and the " "cache parse must not resolve voluptuous; keep the esphome.bundle " - "import inside the branch that extracts one and the Invalid import " - "inside the branch that raises it." + "import inside the branch that extracts one, the Invalid import " + "inside the branch that raises it, and the deferred stdlib " + "imports inside the write/spawn/serial helpers that use them." ) diff --git a/tests/unit_tests/test_main.py b/tests/unit_tests/test_main.py index 9badf856bf..556bac9ee5 100644 --- a/tests/unit_tests/test_main.py +++ b/tests/unit_tests/test_main.py @@ -27,6 +27,7 @@ from esphome.__main__ import ( _unresolved_default_error, _validate_bootloader_binary, _validate_partition_table_binary, + check_permissions, choose_upload_log_host, command_analyze_memory, command_bundle, @@ -6715,3 +6716,27 @@ def test_command_idedata_esp_idf_no_build_errors() -> None: result = command_idedata(MagicMock(), CORE.config) assert result == 1 + + +@pytest.mark.skipif( + os.name != "posix", reason="serial permission checks are posix-only" +) +def test_check_permissions_missing_port() -> None: + """A nonexistent serial port raises the does-not-exist guidance.""" + with ( + patch("os.access", return_value=False), + pytest.raises(EsphomeError, match="serial port does not exist"), + ): + check_permissions("/dev/ttyUSB99") + + +@pytest.mark.skipif( + os.name != "posix", reason="serial permission checks are posix-only" +) +def test_check_permissions_unreadable_port() -> None: + """An existing but unreadable serial port raises the dialout guidance.""" + with ( + patch("os.access", side_effect=lambda _path, mode: mode == os.F_OK), + pytest.raises(EsphomeError, match="read or write permission"), + ): + check_permissions("/dev/ttyUSB99") diff --git a/tests/unit_tests/test_util.py b/tests/unit_tests/test_util.py index 581b1aca99..02309fbff8 100644 --- a/tests/unit_tests/test_util.py +++ b/tests/unit_tests/test_util.py @@ -561,7 +561,7 @@ def test_run_external_process_line_callbacks() -> None: return "PROCESS CALLBACK\n" return None - with patch("esphome.util.subprocess.run") as mock_run: + with patch("subprocess.run") as mock_run: def run_side_effect(*args: Any, **kwargs: Any) -> MagicMock: # Simulate subprocess writing to the stdout RedirectText @@ -635,7 +635,7 @@ def test_detect_rp2040_bootsel_found() -> None: """Test BOOTSEL device detection when device is present.""" mock_result = MagicMock() mock_result.stdout = b"Device Information\n type: RP2040\n" - with patch("esphome.util.subprocess.run", return_value=mock_result): + with patch("subprocess.run", return_value=mock_result): result = util.detect_rp2040_bootsel("/usr/bin/picotool") assert result.device_count == 1 assert result.permission_error is False @@ -645,7 +645,7 @@ def test_detect_rp2040_bootsel_multiple() -> None: """Test BOOTSEL detection with multiple devices.""" mock_result = MagicMock() mock_result.stdout = b"type: RP2040\ntype: RP2350\n" - with patch("esphome.util.subprocess.run", return_value=mock_result): + with patch("subprocess.run", return_value=mock_result): result = util.detect_rp2040_bootsel("/usr/bin/picotool") assert result.device_count == 2 assert result.permission_error is False @@ -658,7 +658,7 @@ def test_detect_rp2040_bootsel_none() -> None: b"No accessible RP2040/RP2350 devices in BOOTSEL mode were found.\n" ) mock_result.stderr = b"" - with patch("esphome.util.subprocess.run", return_value=mock_result): + with patch("subprocess.run", return_value=mock_result): result = util.detect_rp2040_bootsel("/usr/bin/picotool") assert result.device_count == 0 assert result.permission_error is False @@ -675,7 +675,7 @@ def test_detect_rp2040_bootsel_permission_error() -> None: b"but picotool was unable to connect. " b"Maybe try 'sudo' or check your permissions.\n" ) - with patch("esphome.util.subprocess.run", return_value=mock_result): + with patch("subprocess.run", return_value=mock_result): result = util.detect_rp2040_bootsel("/usr/bin/picotool") assert result.device_count == 0 assert result.permission_error is True @@ -686,7 +686,7 @@ def test_detect_rp2040_bootsel_libusb_access_error() -> None: mock_result = MagicMock() mock_result.stdout = b"" mock_result.stderr = b"LIBUSB_ERROR_ACCESS\n" - with patch("esphome.util.subprocess.run", return_value=mock_result): + with patch("subprocess.run", return_value=mock_result): result = util.detect_rp2040_bootsel("/usr/bin/picotool") assert result.device_count == 0 assert result.permission_error is True @@ -694,7 +694,7 @@ def test_detect_rp2040_bootsel_libusb_access_error() -> None: def test_detect_rp2040_bootsel_oserror() -> None: """Test BOOTSEL detection handles OSError.""" - with patch("esphome.util.subprocess.run", side_effect=OSError("not found")): + with patch("subprocess.run", side_effect=OSError("not found")): result = util.detect_rp2040_bootsel("/usr/bin/picotool") assert result.device_count == 0 assert result.permission_error is False @@ -703,7 +703,7 @@ def test_detect_rp2040_bootsel_oserror() -> None: def test_detect_rp2040_bootsel_timeout() -> None: """Test BOOTSEL detection handles timeout.""" with patch( - "esphome.util.subprocess.run", + "subprocess.run", side_effect=subprocess.TimeoutExpired("picotool", 10), ): result = util.detect_rp2040_bootsel("/usr/bin/picotool")