mirror of
https://github.com/esphome/esphome.git
synced 2026-08-22 22:26:21 +00:00
[core] Resolve platform CLI hooks from a registry instead of importing the platform package (#18044)
This commit is contained in:
+30
-21
@@ -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!")
|
||||
|
||||
@@ -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.<platform>.<hook>`` 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
|
||||
@@ -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:
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user