diff --git a/esphome/bundle.py b/esphome/bundle.py index 45d9a74b49..f499e0de1b 100644 --- a/esphome/bundle.py +++ b/esphome/bundle.py @@ -418,6 +418,18 @@ class ConfigBundleCreator: must ship every candidate so the remote build can pick any one. """ discovered = yaml_util.discover_user_yaml_files(self._config_path) + if discovered.load_errors: + _LOGGER.warning( + "Bundle may be incomplete; could not load all configuration files: %s", + "; ".join(discovered.load_errors), + ) + if discovered.unresolved: + _LOGGER.warning( + "Bundle may be incomplete; %d !include path(s) use " + "substitutions and cannot be captured: %s", + len(discovered.unresolved), + ", ".join(discovered.unresolved), + ) self._secrets_paths.update(discovered.secrets) # A !secret inside a file this re-parse does not reach (for example # a git-fetched package the builder re-fetches) still resolves diff --git a/esphome/components/packages/__init__.py b/esphome/components/packages/__init__.py index 9498d4b4da..577385b217 100644 --- a/esphome/components/packages/__init__.py +++ b/esphome/components/packages/__init__.py @@ -1,5 +1,6 @@ from collections import UserDict from collections.abc import Callable +from dataclasses import dataclass, field from functools import reduce import logging from pathlib import Path @@ -35,11 +36,45 @@ from esphome.const import ( CONF_VARS, __version__ as ESPHOME_VERSION, ) -from esphome.core import EsphomeError +from esphome.core import CORE, EsphomeError _LOGGER = logging.getLogger(__name__) DOMAIN = "packages" + + +@dataclass(frozen=True) +class RemotePackageSource: + """A remote source a package was fetched from while processing the config.""" + + url: str + ref: str | None + + +@dataclass +class PackagesData: + """Per-run package state, keyed under DOMAIN in CORE.data.""" + + # Insertion-ordered and de-duplicated + remote_sources: dict[RemotePackageSource, None] = field(default_factory=dict) + + +def _get_data() -> PackagesData: + if DOMAIN not in CORE.data: + CORE.data[DOMAIN] = PackagesData() + return CORE.data[DOMAIN] + + +def get_remote_package_sources() -> tuple[RemotePackageSource, ...]: + """Remote sources fetched for this config, in fetch order. + + store_yaml uses them to tell remote parts of the config from local files. + """ + if (data := CORE.data.get(DOMAIN)) is None: + return () + return tuple(data.remote_sources) + + # Guard against infinite include chains (e.g. A includes B includes A). MAX_INCLUDE_DEPTH = 20 @@ -193,6 +228,9 @@ def _process_remote_package(config: dict[str, Any]) -> dict[str, Any]: username=config.get(CONF_USERNAME), password=config.get(CONF_PASSWORD), ) + _get_data().remote_sources[ + RemotePackageSource(config[CONF_URL], config.get(CONF_REF)) + ] = None files: list[dict[str, Any]] = [] # ``repo_root`` is the directory containing ``.git`` and must be passed diff --git a/esphome/yaml_util.py b/esphome/yaml_util.py index 5882c133f6..f02819a3ed 100644 --- a/esphome/yaml_util.py +++ b/esphome/yaml_util.py @@ -12,7 +12,7 @@ import math import os from pathlib import Path import re -from typing import Any +from typing import Any, NamedTuple import uuid import yaml @@ -410,9 +410,11 @@ def _load_include_candidates( include: IncludeFile, *, warn_on_unresolved: bool, + warn_on_load_error: bool, seen: set[int], expanded_paths: set[Path], keepalive: list[Any], + result: ForceLoadResult, ) -> None: """Load every filesystem candidate for an unresolved ``IncludeFile``.""" from voluptuous import Invalid @@ -425,6 +427,7 @@ def _load_include_candidates( include.file, include.parent_file, ) + result.unresolved.append(str(include.file)) return _LOGGER.debug( "Expanding !include %s (referenced from %s) to %d candidate file(s)", @@ -439,27 +442,27 @@ def _load_include_candidates( try: loaded = include.with_file(candidate.as_posix()).load() except (EsphomeError, Invalid) as err: - # Unlike an unresolved pattern (expected during the discovery - # re-parse), a matched on-disk candidate that fails to load is a - # genuine user error; warn in every mode. The file itself is - # still tracked (the load listener fires before parsing), only - # its nested includes go undiscovered. - _LOGGER.warning( + # A real user error. The file stays tracked (the listener fires + # before parsing); only its nested includes are missed. + (_LOGGER.warning if warn_on_load_error else _LOGGER.debug)( "Failed to load candidate %s for !include %s: %s", candidate, include.file, err, ) + result.errors.append(f"{candidate}: {err}") continue # The throwaway IncludeFile is this tree's only owner; keep the tree # alive so ids recorded in ``seen`` stay unique for the traversal. keepalive.append(loaded) - force_load_include_files( + _force_load_include_files( loaded, warn_on_unresolved=warn_on_unresolved, + warn_on_load_error=warn_on_load_error, _seen=seen, _expanded_paths=expanded_paths, _keepalive=keepalive, + _result=result, ) @@ -473,14 +476,19 @@ def find_secret_references(text: str) -> set[str]: return {match.group(1) for match in _SECRET_REFERENCE_RE.finditer(text)} +class ForceLoadResult(NamedTuple): + """Templated includes that matched no file, and includes that failed to load. + + Either being non-empty means the walk was incomplete. + """ + + unresolved: list[str] + errors: list[str] + + def force_load_include_files( - obj: Any, - *, - warn_on_unresolved: bool = True, - _seen: set[int] | None = None, - _expanded_paths: set[Path] | None = None, - _keepalive: list[Any] | None = None, -) -> None: + obj: Any, *, warn_on_unresolved: bool = True, warn_on_load_error: bool = True +) -> ForceLoadResult: """Recursively resolve any deferred ``IncludeFile`` instances in a YAML tree. Nested ``!include`` returns a deferred ``IncludeFile`` that is only resolved @@ -495,8 +503,30 @@ def force_load_include_files( expression could select. By default a warning is logged when no candidate exists; pass ``warn_on_unresolved=False`` (used by discovery paths that run on a fresh re-parse where substitutions haven't been applied yet) to - demote it to a debug log. + demote it to a debug log. Callers that report ``errors`` themselves pass + ``warn_on_load_error=False`` so load failures are not logged twice. """ + result = ForceLoadResult([], []) + _force_load_include_files( + obj, + warn_on_unresolved=warn_on_unresolved, + warn_on_load_error=warn_on_load_error, + _result=result, + ) + return result + + +def _force_load_include_files( + obj: Any, + *, + warn_on_unresolved: bool = True, + warn_on_load_error: bool = True, + _seen: set[int] | None = None, + _expanded_paths: set[Path] | None = None, + _keepalive: list[Any] | None = None, + _result: ForceLoadResult, +) -> None: + """Walk for :func:`force_load_include_files`, adding to ``result``.""" from voluptuous import Invalid if _seen is None: @@ -519,51 +549,60 @@ def force_load_include_files( _load_include_candidates( obj, warn_on_unresolved=warn_on_unresolved, + warn_on_load_error=warn_on_load_error, seen=_seen, expanded_paths=_expanded_paths, keepalive=_keepalive, + result=_result, ) return try: loaded = obj.load() except (EsphomeError, Invalid) as err: - _LOGGER.warning( + (_LOGGER.warning if warn_on_load_error else _LOGGER.debug)( "Failed to load !include %s (referenced from %s): %s", obj.file, obj.parent_file, err, ) + _result.errors.append(f"{obj.file}: {err}") return - force_load_include_files( + _force_load_include_files( loaded, warn_on_unresolved=warn_on_unresolved, + warn_on_load_error=warn_on_load_error, _seen=_seen, _expanded_paths=_expanded_paths, _keepalive=_keepalive, + _result=_result, ) elif isinstance(obj, dict): if id(obj) in _seen: return _seen.add(id(obj)) for value in obj.values(): - force_load_include_files( + _force_load_include_files( value, warn_on_unresolved=warn_on_unresolved, + warn_on_load_error=warn_on_load_error, _seen=_seen, _expanded_paths=_expanded_paths, _keepalive=_keepalive, + _result=_result, ) elif isinstance(obj, (list, tuple)): if id(obj) in _seen: return _seen.add(id(obj)) for item in obj: - force_load_include_files( + _force_load_include_files( item, warn_on_unresolved=warn_on_unresolved, + warn_on_load_error=warn_on_load_error, _seen=_seen, _expanded_paths=_expanded_paths, _keepalive=_keepalive, + _result=_result, ) @@ -575,11 +614,14 @@ class DiscoveredYamlFiles: were re-parsing the user's config; ``secrets`` is the subset whose *un-resolved* filename matched :data:`esphome.const.SECRETS_FILES` (so a ``secrets.yaml`` symlinked to a differently-named target is still - flagged as secrets). + flagged as secrets). ``files`` is incomplete when ``unresolved`` (templated + include paths) or ``load_errors`` is non-empty. """ files: list[Path] = field(default_factory=list) secrets: set[Path] = field(default_factory=set) + unresolved: list[str] = field(default_factory=list) + load_errors: list[str] = field(default_factory=list) def discover_user_yaml_files(config_path: Path) -> DiscoveredYamlFiles: @@ -609,9 +651,13 @@ def discover_user_yaml_files(config_path: Path) -> DiscoveredYamlFiles: try: try: data = load_yaml(config_path) - except EsphomeError: - return DiscoveredYamlFiles(list(loaded), secrets) - force_load_include_files(data, warn_on_unresolved=False) + except EsphomeError as err: + return DiscoveredYamlFiles( + list(loaded), secrets, load_errors=[f"{config_path}: {err}"] + ) + unresolved, load_errors = force_load_include_files( + data, warn_on_unresolved=False, warn_on_load_error=False + ) finally: _load_listeners.remove(_capture_secret) @@ -622,7 +668,7 @@ def discover_user_yaml_files(config_path: Path) -> DiscoveredYamlFiles: if path not in seen: seen.add(path) unique.append(path) - return DiscoveredYamlFiles(unique, secrets) + return DiscoveredYamlFiles(unique, secrets, unresolved, load_errors) def _add_data_ref(fn): diff --git a/tests/component_tests/packages/test_packages.py b/tests/component_tests/packages/test_packages.py index 418cab5ea1..fd7e9f1278 100644 --- a/tests/component_tests/packages/test_packages.py +++ b/tests/component_tests/packages/test_packages.py @@ -10,9 +10,11 @@ import pytest from esphome import bundle from esphome.components.packages import ( CONFIG_SCHEMA, + RemotePackageSource, _substitute_package_definition, _walk_packages, do_packages_pass, + get_remote_package_sources, merge_packages, resolve_packages, ) @@ -614,6 +616,49 @@ def test_package_remove_by_missing_id() -> None: assert actual == expected +def test_get_remote_package_sources_empty_without_remote_packages() -> None: + """A config without remote packages records no sources.""" + assert get_remote_package_sources() == () + + +@patch("esphome.yaml_util.load_yaml") +@patch("pathlib.Path.is_file") +@patch("esphome.git.clone_or_update") +def test_get_remote_package_sources_records_each_fetch_once( + mock_clone_or_update, mock_is_file, mock_load_yaml +) -> None: + """Every fetched remote source is recorded exactly once.""" + mock_clone_or_update.return_value = (Path("/tmp/noexists"), MagicMock()) + mock_is_file.return_value = True + mock_load_yaml.side_effect = lambda *args, **kwargs: OrderedDict() + + config = { + CONF_PACKAGES: { + "first": { + CONF_URL: "https://github.com/esphome/repo-a", + CONF_REF: "main", + CONF_FILES: [TEST_YAML_FILENAME], + }, + "again": { + CONF_URL: "https://github.com/esphome/repo-a", + CONF_REF: "main", + CONF_FILES: ["other.yaml"], + }, + "second": { + CONF_URL: "https://github.com/esphome/repo-b", + CONF_FILES: [TEST_YAML_FILENAME], + }, + } + } + packages_pass(config) + + # packages are processed last to first, so repo-b is fetched before repo-a + assert get_remote_package_sources() == ( + RemotePackageSource("https://github.com/esphome/repo-b", None), + RemotePackageSource("https://github.com/esphome/repo-a", "main"), + ) + + @patch("esphome.yaml_util.load_yaml") @patch("pathlib.Path.is_file") @patch("esphome.git.clone_or_update") diff --git a/tests/unit_tests/test_bundle.py b/tests/unit_tests/test_bundle.py index 6a08676784..fe4bf19715 100644 --- a/tests/unit_tests/test_bundle.py +++ b/tests/unit_tests/test_bundle.py @@ -1363,7 +1363,8 @@ def test_discover_files_candidate_outside_config_dir_skipped( def test_discover_files_nested_include_load_failure( tmp_path: Path, caplog: pytest.LogCaptureFixture ) -> None: - """A nested !include pointing at a missing file is logged and skipped.""" + """A nested !include pointing at a missing file is reported in the bundle + summary, not a second time by yaml_util, and skipped.""" config_dir = _setup_config_dir(tmp_path) (config_dir / "test.yaml").write_text( "esphome:\n name: test\nwifi: !include missing.yaml\n" @@ -1374,10 +1375,12 @@ def test_discover_files_nested_include_load_failure( paths = [f.path for f in files] assert "test.yaml" in paths + warnings = [r.message for r in caplog.records if r.levelname == "WARNING"] assert any( - "failed to load !include" in r.message.lower() and "missing.yaml" in r.message - for r in caplog.records + m.startswith("Bundle may be incomplete") and "missing.yaml" in m + for m in warnings ) + assert not any("failed to load !include" in m.lower() for m in warnings) def test_force_load_skips_duplicate_include_file() -> None: diff --git a/tests/unit_tests/test_yaml_util.py b/tests/unit_tests/test_yaml_util.py index 24a431b0c0..c30b03467f 100644 --- a/tests/unit_tests/test_yaml_util.py +++ b/tests/unit_tests/test_yaml_util.py @@ -1280,22 +1280,52 @@ def test_force_load_include_files_unresolved_log_level( assert matching == [expect_level] +def test_force_load_include_files_returns_unresolved_paths( + patch_include_file: None, +) -> None: + """Includes with substitution-templated paths are reported back to the + caller; resolvable ones are not.""" + templated = _StubInclude("${var}.yaml", unresolved=True) + plain = _StubInclude("ok.yaml") + result = force_load_include_files({"a": templated, "b": plain}) + assert result.unresolved == [str(templated.file)] + assert result.errors == [] + assert plain.load_calls == 1 + + def test_force_load_include_files_warns_on_load_failure( patch_include_file: None, caplog: pytest.LogCaptureFixture, ) -> None: - """An `EsphomeError` raised by `load()` is caught and logged, not propagated.""" + """An `EsphomeError` raised by `load()` is caught, logged, and reported to + the caller — not propagated.""" stub = _StubInclude("missing.yaml", raise_on_load=EsphomeError("boom")) with caplog.at_level("WARNING", logger="esphome.yaml_util"): - force_load_include_files({"k": stub}) + result = force_load_include_files({"k": stub}) assert any( "Failed to load !include" in r.message and "missing.yaml" in r.message for r in caplog.records ) + assert result.errors == [f"{stub.file}: boom"] + assert result.unresolved == [] + + +def test_force_load_include_files_load_failure_quiet_when_caller_reports( + patch_include_file: None, + caplog: pytest.LogCaptureFixture, +) -> None: + """With warn_on_load_error=False the failure is still returned but only + logged at debug, so a caller that reports it does not duplicate it.""" + stub = _StubInclude("missing.yaml", raise_on_load=EsphomeError("boom")) + with caplog.at_level("DEBUG", logger="esphome.yaml_util"): + result = force_load_include_files({"k": stub}, warn_on_load_error=False) + assert result.errors == [f"{stub.file}: boom"] + failures = [r for r in caplog.records if "Failed to load !include" in r.message] + assert [r.levelname for r in failures] == ["DEBUG"] def test_discovered_yaml_files_holds_files_and_secrets() -> None: - """`DiscoveredYamlFiles` is a small data carrier; both fields are mandatory.""" + """`DiscoveredYamlFiles` is a small data carrier.""" files = [Path("/tmp/a.yaml")] secrets = {Path("/tmp/a.yaml")} discovered = DiscoveredYamlFiles(files, secrets) @@ -1358,11 +1388,24 @@ def test_discover_user_yaml_files_flags_secrets_symlink(tmp_path: Path) -> None: assert target.resolve() in discovered.secrets -def test_discover_user_yaml_files_swallows_parse_errors(tmp_path: Path) -> None: - """A YAML parse failure returns whatever was tracked so far without raising.""" +def test_discover_user_yaml_files_reports_parse_errors(tmp_path: Path) -> None: + """A YAML parse failure is surfaced in `.load_errors` (not raised), so + consumers can tell the file set is incomplete.""" entry = _write(tmp_path, "entry.yaml", "esphome: [unterminated\n") discovered = discover_user_yaml_files(entry) assert isinstance(discovered, DiscoveredYamlFiles) + assert len(discovered.load_errors) == 1 + assert "entry.yaml" in discovered.load_errors[0] + + +def test_discover_user_yaml_files_reports_unresolved_includes( + tmp_path: Path, +) -> None: + """A substitution-templated `!include` path is surfaced in `.unresolved`.""" + entry = _write_entry_including(tmp_path, "${board}.yaml") + discovered = discover_user_yaml_files(entry) + assert len(discovered.unresolved) == 1 + assert "${board}.yaml" in discovered.unresolved[0] def test_discover_user_yaml_files_deduplicates(tmp_path: Path) -> None: @@ -1589,9 +1632,9 @@ def test_discover_user_yaml_files_many_candidates_keep_nested_includes( def test_discover_user_yaml_files_bad_candidate_still_tracked( tmp_path: Path, caplog: pytest.LogCaptureFixture ) -> None: - """A matched candidate that fails to parse warns even during discovery, - stays tracked (the load listener fires before parsing), and doesn't block - other candidates.""" + """A matched candidate that fails to parse is reported in ``load_errors`` + (logged only at debug, the caller reports it), stays tracked (the load + listener fires before parsing), and doesn't block other candidates.""" _write(tmp_path, "keys/good.yaml", "api:\n") _write(tmp_path, "keys/bad.yaml", "esphome: [unterminated\n") with caplog.at_level("DEBUG", logger="esphome.yaml_util"): @@ -1604,7 +1647,8 @@ def test_discover_user_yaml_files_bad_candidate_still_tracked( matching = [ r.levelname for r in caplog.records if "Failed to load candidate" in r.message ] - assert matching == ["WARNING"] + assert matching == ["DEBUG"] + assert any("bad.yaml" in e for e in discovered.load_errors) def test_discover_user_yaml_files_tolerates_templated_top_level_include(