From 5abd100b53cc84a9b16063e246ba58d2704391eb Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 4 Aug 2026 11:08:56 -0500 Subject: [PATCH] [core] Resolve platform CLI hooks from a registry instead of importing the platform package (#18044) --- esphome/__main__.py | 51 ++++---- esphome/platform_hooks.py | 113 +++++++++++++++++ tests/unit_tests/test_main.py | 86 ++++++++++--- tests/unit_tests/test_platform_hooks.py | 153 ++++++++++++++++++++++++ 4 files changed, 366 insertions(+), 37 deletions(-) create mode 100644 esphome/platform_hooks.py create mode 100644 tests/unit_tests/test_platform_hooks.py diff --git a/esphome/__main__.py b/esphome/__main__.py index 3893c678e1..fb51fdaf15 100644 --- a/esphome/__main__.py +++ b/esphome/__main__.py @@ -19,7 +19,7 @@ from typing import Protocol # Note: Do not import modules from esphome.components here, as this would # cause them to be loaded before external components are processed, resulting # in the built-in version being used instead of the external component one. -from esphome import const +from esphome import const, platform_hooks from esphome.const import ( ALLOWED_NAME_CHARS, ARGUMENT_HELP_DEVICE, @@ -630,16 +630,27 @@ def run_miniterm(config: ConfigType, port: str, args) -> int: return 1 _LOGGER.info("Starting log output from %s with baud rate %s", port, baud_rate) - process_stacktrace = None - + # Stacktrace analysis is optional; a broken platform import must not + # stop serial log streaming, but it is a real breakage and must not + # masquerade as an ordinary capability gap. try: - module = importlib.import_module("esphome.components." + CORE.target_platform) - process_stacktrace = module.process_stacktrace - except (AttributeError, ImportError): - _LOGGER.info( - 'Stacktrace analysis is unavailable: no compatible analyzer found for target platform "%s".', - CORE.target_platform, + process_stacktrace = platform_hooks.get_platform_hook( + CORE.target_platform, "process_stacktrace" ) + except ImportError as err: + _LOGGER.debug("Stacktrace analyzer import failed", exc_info=True) + _LOGGER.warning( + 'Stacktrace analysis is unavailable: analyzer for target platform "%s" failed to import: %s', + CORE.target_platform, + err, + ) + process_stacktrace = None + else: + if process_stacktrace is None: + _LOGGER.info( + 'Stacktrace analysis is unavailable: no compatible analyzer found for target platform "%s".', + CORE.target_platform, + ) backtrace_state = False ser = serial.Serial() @@ -1141,12 +1152,11 @@ def upload_program( config: ConfigType, args: ArgsProtocol, devices: list[str] ) -> tuple[int, str | None]: host = devices[0] - try: - module = importlib.import_module("esphome.components." + CORE.target_platform) - if module.upload_program(config, args, host): - return 0, host - except AttributeError: - pass + platform_upload = platform_hooks.get_platform_hook( + CORE.target_platform, "upload_program" + ) + if platform_upload is not None and platform_upload(config, args, host): + return 0, host port_type = get_port_type(host) @@ -1406,12 +1416,11 @@ def _should_subscribe_states(args: ArgsProtocol) -> bool: def show_logs(config: ConfigType, args: ArgsProtocol, devices: list[str]) -> int | None: - try: - module = importlib.import_module("esphome.components." + CORE.target_platform) - if module.show_logs(config, args, devices): - return 0 - except AttributeError: - pass + platform_show_logs = platform_hooks.get_platform_hook( + CORE.target_platform, "show_logs" + ) + if platform_show_logs is not None and platform_show_logs(config, args, devices): + return 0 if "logger" not in config: raise EsphomeError("Logger is not configured!") diff --git a/esphome/platform_hooks.py b/esphome/platform_hooks.py new file mode 100644 index 0000000000..2106f50319 --- /dev/null +++ b/esphome/platform_hooks.py @@ -0,0 +1,113 @@ +"""Registry of platform packages that provide optional CLI hooks. + +The logs/upload fast path must know whether a target platform overrides +``show_logs``/``upload_program`` or provides ``process_stacktrace`` +without importing the platform package to find out; importing one pulls +in the whole validation stack (config_validation, voluptuous, boards), +which costs seconds on slow hardware. Keep the mapping in sync with the +hook definitions in ``esphome/components/*/__init__.py``; a unit test +imports each platform package and fails when they drift. + +The compile-path ``run_compile`` hook is deliberately not registered: +compiling imports the platform package regardless, so its probe in +``__main__.py`` stays eager. The network log client's +``process_stacktrace`` probe in ``esphome/api_client.py`` still uses the +old importlib pattern; converting it is a separate change. +""" + +from __future__ import annotations + +from collections.abc import Callable +from importlib import import_module +import logging +from typing import Any, Final + +from esphome.const import ( + PLATFORM_ESP32, + PLATFORM_ESP8266, + PLATFORM_NRF52, + PLATFORM_RP2, + Platform, +) + +_LOGGER = logging.getLogger(__name__) + +# Hooks whose loss only degrades diagnostics; skipping one of these is +# logged at debug, while skipping a hook that changes what the CLI does +# (upload method, log transport) warns. A new hook is loud by default. +COSMETIC_HOOKS: Final = frozenset({"process_stacktrace"}) + +PLATFORM_HOOKS: Final[dict[str, frozenset[str]]] = { + "show_logs": frozenset({PLATFORM_NRF52}), + "upload_program": frozenset({PLATFORM_NRF52}), + "process_stacktrace": frozenset( + {PLATFORM_ESP32, PLATFORM_ESP8266, PLATFORM_NRF52, PLATFORM_RP2} + ), +} + + +# The registry only speaks for in-tree platforms; a target platform +# supplied via external_components is normally not in Platform and falls +# back to probing the imported package, as the CLI did before the +# registry. Deliberate trade: an external component that shadows an +# in-tree platform name (the meta finder allows it) is treated as the +# in-tree platform here, so its own hooks are not probed. +_IN_TREE_PLATFORMS: Final = frozenset(Platform) + + +def get_platform_hook(platform: str, hook: str) -> Callable[..., Any] | None: + """Return ``esphome.components..`` or None. + + In-tree platforms not registered for the hook return None without + being imported. A registered platform that no longer defines the + hook also returns None, so a stale registry degrades to the generic + path instead of raising. + """ + registered = platform in PLATFORM_HOOKS[hook] + if not registered and platform in _IN_TREE_PLATFORMS: + return None + # For external platforms this probes the imported package like the + # CLI used to; the package can be missing entirely on the warm-cache + # path, where the external_components meta finder never registered. + # Degrade to the generic path then, but let a failure deeper in the + # package (missing dependency) surface. + module_name = f"esphome.components.{platform}" + try: + module = import_module(module_name) + except ModuleNotFoundError as err: + if registered or err.name != module_name: + raise + if hook in COSMETIC_HOOKS: + _LOGGER.debug( + "External platform %s is not importable; using the generic %s path", + platform, + hook, + ) + else: + # Deliberately loud even though the warm-cache path makes + # this expected: the user's platform hooks are not in effect + # for this run, and a silently substituted upload method is + # worse than a routine warning. + _LOGGER.warning( + "External platform %s is not importable; using the generic %s path", + platform, + hook, + ) + return None + handler = getattr(module, hook, None) + if handler is None: + if registered: + _LOGGER.warning( + "%s is registered for %s but no longer exposes it; using the generic path", + platform, + hook, + ) + else: + # The common case for external platforms; debug so a typoed + # hook name is still diagnosable without being noisy. + _LOGGER.debug( + "External platform %s does not expose %s; using the generic path", + platform, + hook, + ) + return handler diff --git a/tests/unit_tests/test_main.py b/tests/unit_tests/test_main.py index 289497057f..2c9a88afb6 100644 --- a/tests/unit_tests/test_main.py +++ b/tests/unit_tests/test_main.py @@ -94,6 +94,7 @@ from esphome.const import ( PLATFORM_BK72XX, PLATFORM_ESP32, PLATFORM_ESP8266, + PLATFORM_NRF52, PLATFORM_RP2, Toolchain, ) @@ -2862,18 +2863,17 @@ def test_upload_program_ota_with_mqtt_empty_broker( assert "MQTT IP discovery failed" in caplog.text -@patch("esphome.__main__.importlib.import_module") +@patch("esphome.platform_hooks.get_platform_hook") def test_upload_program_platform_specific_handler( - mock_import: Mock, + mock_get_hook: Mock, mock_get_port_type: Mock, ) -> None: """Test upload_program with platform-specific upload handler.""" - setup_core(platform="custom_platform") + setup_core(platform=PLATFORM_NRF52) mock_get_port_type.return_value = "CUSTOM" - mock_module = MagicMock() - mock_module.upload_program.return_value = True - mock_import.return_value = mock_module + platform_upload = MagicMock(return_value=True) + mock_get_hook.return_value = platform_upload config = {} args = MockArgs() @@ -2883,8 +2883,8 @@ def test_upload_program_platform_specific_handler( assert exit_code == 0 assert host == "custom_device" - mock_import.assert_called_once_with("esphome.components.custom_platform") - mock_module.upload_program.assert_called_once_with(config, args, "custom_device") + mock_get_hook.assert_called_once_with(PLATFORM_NRF52, "upload_program") + platform_upload.assert_called_once_with(config, args, "custom_device") def test_show_logs_serial( @@ -3108,16 +3108,15 @@ def test_show_logs_no_method_configured() -> None: show_logs(CORE.config, args, devices) -@patch("esphome.__main__.importlib.import_module") +@patch("esphome.platform_hooks.get_platform_hook") def test_show_logs_platform_specific_handler( - mock_import: Mock, + mock_get_hook: Mock, ) -> None: """Test show_logs with platform-specific logs handler.""" - setup_core(platform="custom_platform", config={"logger": {}}) + setup_core(platform=PLATFORM_NRF52, config={"logger": {}}) - mock_module = MagicMock() - mock_module.show_logs.return_value = True - mock_import.return_value = mock_module + platform_show_logs = MagicMock(return_value=True) + mock_get_hook.return_value = platform_show_logs config = {"logger": {}} args = MockArgs() @@ -3126,8 +3125,8 @@ def test_show_logs_platform_specific_handler( result = show_logs(config, args, devices) assert result == 0 - mock_import.assert_called_once_with("esphome.components.custom_platform") - mock_module.show_logs.assert_called_once_with(config, args, devices) + mock_get_hook.assert_called_once_with(PLATFORM_NRF52, "show_logs") + platform_show_logs.assert_called_once_with(config, args, devices) def test_has_mqtt_logging_no_log_topic() -> None: @@ -5717,6 +5716,61 @@ def test_run_miniterm_batches_lines_with_same_timestamp( ) +def test_run_miniterm_analyzer_import_failure_keeps_streaming( + caplog: pytest.LogCaptureFixture, +) -> None: + """A broken platform import must not stop serial log streaming.""" + mock_serial = MockSerial([b"[I][app:100]: Line 1\r\n", MOCK_SERIAL_END]) + + CORE.data[KEY_CORE] = {KEY_TARGET_PLATFORM: PLATFORM_ESP32} + config = { + CONF_LOGGER: { + CONF_BAUD_RATE: 115200, + "deassert_rts_dtr": False, + } + } + args = MockArgs() + + with ( + caplog.at_level("INFO", logger="esphome.__main__"), + patch("serial.Serial", return_value=mock_serial), + patch( + "esphome.platform_hooks.get_platform_hook", + side_effect=ImportError("broken platform package"), + ), + ): + result = run_miniterm(config, "/dev/ttyUSB0", args) + + assert result == 0 + # A broken package is distinguishable from a plain capability gap. + assert "failed to import: broken platform package" in caplog.text + + +def test_run_miniterm_no_stacktrace_analyzer( + caplog: pytest.LogCaptureFixture, +) -> None: + """Platforms without a stacktrace analyzer log an info and stream anyway.""" + mock_serial = MockSerial([b"[I][app:100]: Line 1\r\n", MOCK_SERIAL_END]) + + CORE.data[KEY_CORE] = {KEY_TARGET_PLATFORM: PLATFORM_BK72XX} + config = { + CONF_LOGGER: { + CONF_BAUD_RATE: 115200, + "deassert_rts_dtr": False, + } + } + args = MockArgs() + + with ( + caplog.at_level("INFO", logger="esphome.__main__"), + patch("serial.Serial", return_value=mock_serial), + ): + result = run_miniterm(config, "/dev/ttyUSB0", args) + + assert result == 0 + assert "Stacktrace analysis is unavailable" in caplog.text + + def test_run_miniterm_different_chunks_different_timestamps( capfd: CaptureFixture[str], ) -> None: diff --git a/tests/unit_tests/test_platform_hooks.py b/tests/unit_tests/test_platform_hooks.py new file mode 100644 index 0000000000..27d37e1d76 --- /dev/null +++ b/tests/unit_tests/test_platform_hooks.py @@ -0,0 +1,153 @@ +"""Guard the platform CLI-hook registry in ``esphome.platform_hooks``. + +The registry lets the logs/upload fast path skip importing platform +packages that don't provide a hook; these tests fail when a platform +gains or loses a hook without the registry being updated, and pin down +that the fast path really avoids the import. +""" + +from __future__ import annotations + +import importlib +from unittest.mock import Mock + +import pytest + +from esphome import platform_hooks +from esphome.const import PLATFORM_ESP32, Platform + + +def test_no_unregistered_platform_exposes_a_hook() -> None: + """Every platform hook the packages expose must be registered. + + Behavioural on purpose: a hook added as a re-export, an assignment, + or an ``async def`` is invisible to source scanning but very visible + to ``hasattr``, and an unregistered hook is silently never called. + The registered direction is covered by + test_every_registered_pair_resolves below. + """ + for platform in frozenset(Platform): + module = importlib.import_module(f"esphome.components.{platform}") + for hook, registered in platform_hooks.PLATFORM_HOOKS.items(): + if hasattr(module, hook): + assert platform in registered, ( + f"{platform} exposes {hook} but is not registered for it. " + "Update esphome/platform_hooks.py." + ) + + +def test_registered_platform_resolves_hook() -> None: + hook = platform_hooks.get_platform_hook(PLATFORM_ESP32, "process_stacktrace") + from esphome.components import esp32 + + assert hook is esp32.process_stacktrace + + +def test_every_registered_pair_resolves() -> None: + """Each registered platform must actually expose the hook at runtime. + + Text scanning can miss re-exports or decorated definitions; this is + the behavioural check for the direction that matters when the CLI + runs. + """ + for hook, platforms in platform_hooks.PLATFORM_HOOKS.items(): + for platform in platforms: + assert callable(platform_hooks.get_platform_hook(platform, hook)), ( + f"{platform} is registered for {hook} but does not expose it" + ) + + +def test_external_platform_falls_back_to_probe( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Out-of-tree target platforms keep working via the dynamic probe.""" + module = type("FakePlatform", (), {"show_logs": staticmethod(lambda *a: True)}) + imported: list[str] = [] + + def fake_import(name: str): + imported.append(name) + return module + + monkeypatch.setattr(platform_hooks, "import_module", fake_import) + hook = platform_hooks.get_platform_hook("my_external_chip", "show_logs") + assert hook is module.show_logs + assert imported == ["esphome.components.my_external_chip"] + + +def test_external_platform_missing_module_degrades( + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, +) -> None: + """A warm-cache run may not have the external package importable. + + Skipping a behavior-changing hook is visible at warning; losing + stacktrace decoding is cosmetic and stays at debug. + """ + monkeypatch.setattr( + platform_hooks, + "import_module", + Mock( + side_effect=ModuleNotFoundError( + "not found", name="esphome.components.my_external_chip" + ) + ), + ) + assert platform_hooks.get_platform_hook("my_external_chip", "show_logs") is None + assert "not importable" in caplog.text + assert any(r.levelname == "WARNING" for r in caplog.records) + + caplog.clear() + assert ( + platform_hooks.get_platform_hook("my_external_chip", "process_stacktrace") + is None + ) + assert not any(r.levelname == "WARNING" for r in caplog.records) + + +def test_external_platform_without_hook_logs_debug( + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, +) -> None: + """The common no-hook case stays quiet but diagnosable.""" + caplog.set_level("DEBUG", logger="esphome.platform_hooks") + module = type("ExternalPlatform", (), {}) # imports fine, no hook + monkeypatch.setattr(platform_hooks, "import_module", Mock(return_value=module)) + assert platform_hooks.get_platform_hook("my_external_chip", "show_logs") is None + assert "does not expose" in caplog.text + assert not any(r.levelname == "WARNING" for r in caplog.records) + + +def test_stale_registry_entry_warns( + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, +) -> None: + """A vendored tree where a registered hook vanished must say so.""" + module = type("StalePlatform", (), {}) # registered but no hook + monkeypatch.setattr(platform_hooks, "import_module", Mock(return_value=module)) + assert platform_hooks.get_platform_hook("nrf52", "show_logs") is None + assert "no longer exposes it" in caplog.text + + +def test_external_platform_broken_dependency_raises( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A missing dependency inside the external package must surface.""" + monkeypatch.setattr( + platform_hooks, + "import_module", + Mock(side_effect=ModuleNotFoundError("not found", name="some_missing_dep")), + ) + with pytest.raises(ModuleNotFoundError, match="not found"): + platform_hooks.get_platform_hook("my_external_chip", "show_logs") + + +def test_lookup_miss_does_not_import_platform_package( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The whole point: probing a platform without hooks must not import it.""" + monkeypatch.setattr( + platform_hooks, + "import_module", + Mock(side_effect=AssertionError("platform package imported on registry miss")), + ) + assert platform_hooks.get_platform_hook(PLATFORM_ESP32, "show_logs") is None