From 65609bb94b7a3b0b3acd2c804156fe175ecddf31 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 12:46:16 -0500 Subject: [PATCH] Move the backend-agnostic library hardening down from the arduino backend PR The typed IncompatiblePlatform exception, dependency_is_usable, warn_properties_depends, the lex_build_flags helper (espidf switches to it), the normalize_dependencies validation, the manifest shape check, the component-level drop-warning split, and the SCons case-sensitive .C/.C++ suffixes all harden the shared converter independently of the arduino backend, so they belong in this PR; the provides hook and the version-less reconciliation stay with the backend that needs them. --- esphome/espidf/component.py | 21 +-- esphome/platformio/library.py | 150 ++++++++++++++++++-- tests/unit_tests/test_platformio_library.py | 122 ++++++++++++++++ 3 files changed, 262 insertions(+), 31 deletions(-) diff --git a/esphome/espidf/component.py b/esphome/espidf/component.py index 567cde65e2..4eeaa30e7f 100644 --- a/esphome/espidf/component.py +++ b/esphome/espidf/component.py @@ -27,8 +27,7 @@ from esphome.platformio.library import ( collect_filtered_files, convert_libraries, ensure_list, - join_flag_args, - split_flag_entry, + lex_build_flags, split_list_by_condition, ) @@ -89,22 +88,12 @@ def generate_cmakelists_txt(component: IDFComponent) -> str: build_src_filter = ensure_list( component.data.get("build", {}).get("srcFilter", DEFAULT_BUILD_SRC_FILTER) ) - build_flags = ensure_list( - component.data.get("build", {}).get("flags", DEFAULT_BUILD_FLAGS) - ) # PlatformIO shell-lexes each build.flags entry; bare -I/-L/-l/-D tokens # re-glue to their argument so the prefix classifiers below route them. - # Joined per entry, as SCons's ParseFlags lexes each string - # independently: a dangling -I ending one entry must warn, not absorb - # the next entry's first token. - build_flags = [ - token - for entry in build_flags - for token in join_flag_args( - split_flag_entry(entry, f"library {component.name}"), - f"library {component.name}", - ) - ] + build_flags = lex_build_flags( + component.data.get("build", {}).get("flags", DEFAULT_BUILD_FLAGS), + f"library {component.name}", + ) # List all sources files build_src_files = collect_filtered_files( diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index 7ce45a7fca..fb12f6f92b 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -55,6 +55,8 @@ SOURCE_KIND_FOR_SUFFIX: dict[str, str] = { ".cc": "cxx", ".cxx": "cxx", ".c++": "cxx", + ".C": "cxx", + ".C++": "cxx", ".S": "asm", ".spp": "asm", ".SPP": "asm", @@ -206,6 +208,14 @@ class InvalidLibrary(Exception): pass +class IncompatiblePlatform(InvalidLibrary): + """The manifest's platform filter rejected the target platform. + + A distinct type so callers can treat the routine cross-platform skip + differently from other manifest problems without matching message text. + """ + + class ConvertedLibrary: """A resolved PlatformIO library plus its parsed manifest and on-disk path. @@ -435,7 +445,7 @@ def check_library_data(data: dict, platform: str | None, framework: str): valid_platforms = platform is None or "*" in platforms or platform in platforms if not valid_platforms: - raise InvalidLibrary(f"Unsupported library platforms: {platforms}") + raise IncompatiblePlatform(f"Unsupported library platforms: {platforms}") frameworks = data.get("frameworks", "*") if isinstance(frameworks, str): @@ -571,6 +581,24 @@ def split_flag_entry(entry: Any, owner: str) -> list[str]: raise EsphomeError(f"Malformed build flag {entry!r} in {owner}: {err}") from err +def lex_build_flags(entries: str | list[str], owner: str) -> list[str]: + """Shell-lex a manifest ``build.flags`` list into joined tokens. + + Each entry is lexed the way PlatformIO's ParseFlags does, and bare + ``-I``/``-L``/``-l``/``-D`` tokens re-glue to their argument across the + whole stream. Used by the espidf and arduino backends; zephyr still + classifies raw entries. + """ + # Join per entry, as SCons's ParseFlags lexes each string independently: + # a dangling -I ending one entry must warn, not absorb the next entry's + # first token. + return [ + token + for entry in ensure_list(entries) + for token in join_flag_args(split_flag_entry(entry, owner), owner) + ] + + # Flags whose argument may follow as a separate token; ParseFlags glues them BARE_ARG_FLAGS = frozenset({"-I", "-L", "-l", "-D"}) @@ -591,19 +619,60 @@ def join_flag_args(tokens: Iterable[str], owner: str) -> list[str]: return out -def normalize_dependencies(dependencies: Any) -> list[dict]: +def warn_properties_depends(name: str, data: object) -> None: + """Warn when a manifest declares dependencies only as ``depends=``. + + The dependency walk reads the JSON ``dependencies`` key; the raw + ``library.properties`` spelling would otherwise drop silently. + """ + if isinstance(data, dict) and not data.get("dependencies") and data.get("depends"): + _LOGGER.warning( + "Library %s declares dependencies via library.properties " + "depends=, which are not resolved automatically; add them with " + "add_library() if needed", + name, + ) + + +def dependency_is_usable( + dep: dict, platform: str | None, framework: str, requester: str +) -> bool: + """Compatibility filter for a manifest dependency: platform mismatches + skip at debug, any other ``InvalidLibrary`` warns naming the requester.""" + try: + check_library_data(dep, platform, framework) + except IncompatiblePlatform as e: + _LOGGER.debug("Skip dependency %s of %s: %s", dep.get("name"), requester, e) + return False + except InvalidLibrary as e: + _LOGGER.warning( + "Skipping dependency %s of %s: %s", dep.get("name"), requester, e + ) + return False + return True + + +def normalize_dependencies( + dependencies: Any, manifest_name: str = "manifest" +) -> list[dict]: """Normalize a library manifest's ``dependencies`` to a list of dicts. - PIO's library.json accepts both the list-of-dicts form and the shorthand - dict form (``{"owner/Name": "version_spec"}``); normalize the latter so - callers see a uniform list. + PIO's library.json accepts the list-of-dicts form, the shorthand dict + form (``{"owner/Name": "version_spec"}``), bare name strings inside the + list, and a plain (possibly comma-separated) string; normalize them all + so callers see a uniform list. ``manifest_name`` names the manifest in the + warning for entries that cannot be normalized. """ if not dependencies: return [] + if isinstance(dependencies, str): + # A plain string is one or more comma-separated names; iterating it + # as a list would shred it into one-character "libraries" + return [{"name": n.strip()} for n in dependencies.split(",") if n.strip()] if isinstance(dependencies, dict): normalized = [] for raw_name, spec in dependencies.items(): - if "/" in raw_name: + if isinstance(raw_name, str) and "/" in raw_name: owner, pkgname = raw_name.split("/", 1) else: owner, pkgname = None, raw_name @@ -612,9 +681,46 @@ def normalize_dependencies(dependencies: Any) -> list[dict]: entry.update(spec) else: entry["version"] = spec + if not isinstance(name := entry.get("name"), str) or not name: + _LOGGER.warning( + "Ignoring unrecognized dependency entry %r of %s", + entry, + manifest_name, + ) + continue normalized.append(entry) return normalized - return [d for d in dependencies if isinstance(d, dict)] + if not isinstance(dependencies, (list, tuple)): + _LOGGER.warning( + "Ignoring unrecognized dependencies %r of %s", + dependencies, + manifest_name, + ) + return [] + normalized = [] + for entry in dependencies: + if isinstance(entry, dict): + name = entry.get("name") + if not isinstance(name, str) or not name: + # A dependency name must be a non-empty string; every + # consumer indexes or joins it + _LOGGER.warning( + "Ignoring unrecognized dependency entry %r of %s", + entry, + manifest_name, + ) + continue + normalized.append(entry) + elif isinstance(entry, str) and entry: + # PIO also accepts a bare list of names ("dependencies": ["Wire"]) + normalized.append({"name": entry}) + else: + _LOGGER.warning( + "Ignoring unrecognized dependency entry %r of %s", + entry, + manifest_name, + ) + return normalized @dataclass @@ -900,30 +1006,44 @@ def convert_libraries( f"library.properties in {source_dir}" ) + if not isinstance(component.data, dict) or not isinstance( + component.data.get("build", {}), dict + ): + # A bare json.load imposes no shape; every backend dereferences + # data/build, so validate once here and name the library + raise EsphomeError(f"Library {key} has a malformed manifest") + warn_properties_depends(component.name, component.data) + try: check_library_data(component.data, backend.platform, backend.framework) except InvalidLibrary as e: - # Skip an incompatible transitive dependency, but fail fast if a - # top-level library the build explicitly requested is incompatible. + # Fail fast if a top-level library the build explicitly requested + # is incompatible; the routine cross-platform skip stays at + # debug, any other cause warns (a silent drop resurfaces as + # undefined symbols at link) if key in top_level_keys: raise RuntimeError( f"Requested library {key} is not compatible with " f"{backend.framework}: {e}" ) from e - _LOGGER.debug("Skip incompatible dependency %s: %s", key, str(e)) + if isinstance(e, IncompatiblePlatform): + _LOGGER.debug("Skip incompatible dependency %s: %s", key, str(e)) + else: + _LOGGER.warning("Skipping dependency %s: %s", key, str(e)) continue components[key] = component # Requirements changed (we got past the short-circuit above), so # (re)walk this component's dependencies. node.edges = set() - for dependency in normalize_dependencies(component.data.get("dependencies")): + for dependency in normalize_dependencies( + component.data.get("dependencies"), component.name + ): if "name" not in dependency or "version" not in dependency: continue - try: - check_library_data(dependency, backend.platform, backend.framework) - except InvalidLibrary as e: - _LOGGER.debug("Skip dependency %s: %s", dependency.get("name"), str(e)) + if not dependency_is_usable( + dependency, backend.platform, backend.framework, component.name + ): continue dep_name = _owner_pkgname_to_name( dependency.get("owner"), dependency.get("name") diff --git a/tests/unit_tests/test_platformio_library.py b/tests/unit_tests/test_platformio_library.py index d050acad9e..60520f3df1 100644 --- a/tests/unit_tests/test_platformio_library.py +++ b/tests/unit_tests/test_platformio_library.py @@ -558,6 +558,128 @@ def test_join_flag_args_trailing_bare_flag_warns( assert "Ignoring trailing '-l'" in caplog.text +def test_lex_build_flags_dangling_flag_does_not_cross_entries( + caplog: pytest.LogCaptureFixture, +) -> None: + """Each entry is lexed independently, as ParseFlags does: a dangling -I + ending one entry warns instead of absorbing the next entry's first token.""" + from esphome.platformio.library import lex_build_flags + + assert lex_build_flags(["-Wall -I", "-DFOO=1"], "lib x") == ["-Wall", "-DFOO=1"] + assert "Ignoring trailing '-I'" in caplog.text + + +def test_normalize_dependencies_forms(caplog) -> None: + """Every PIO-legal spelling normalizes; unrecognizable entries warn.""" + from esphome.platformio.library import normalize_dependencies + + assert normalize_dependencies( + ["Wire", {"name": "SPI"}, 5, "", {"version": "1.0"}], "libx" + ) == [ + {"name": "Wire"}, + {"name": "SPI"}, + ] + # The int, the empty string, and the nameless dict all warn + assert caplog.text.count("unrecognized dependency entry") == 3 + # A plain string is names, never iterated into characters + assert normalize_dependencies("Wire, SPI") == [ + {"name": "Wire"}, + {"name": "SPI"}, + ] + assert normalize_dependencies("Wire") == [{"name": "Wire"}] + # A non-iterable value fails by manifest name, never a bare TypeError + assert normalize_dependencies(5, "libx") == [] + assert "Ignoring unrecognized dependencies 5 of libx" in caplog.text + # The dict-shorthand form validates names like the list form: an empty + # key and a spec overriding name with a non-string both warn and drop + assert normalize_dependencies( + {"": "1.0", "Wire": {"name": 123, "version": "1.0"}, "SPI": "*"}, "libx" + ) == [{"name": "SPI", "owner": None, "version": "*"}] + assert caplog.text.count("unrecognized dependency entry") == 5 + + +@pytest.mark.parametrize( + "manifest", [["not", "a", "manifest"], {"name": "A", "build": "src"}] +) +def test_convert_libraries_malformed_manifest_raises( + tmp_path, monkeypatch, manifest +) -> None: + """A manifest without the expected dict shape fails by library name + before any backend dereferences data/build.""" + _patch_download_with_manifests(monkeypatch, tmp_path, {"esphome/A": manifest}) + with pytest.raises(EsphomeError, match="has a malformed manifest"): + convert_libraries([Library("esphome/A", None, None)], _backend()) + + +def test_walk_warns_for_properties_only_depends( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """A manifest declaring dependencies only as library.properties depends= + warns in the shared walk, so every backend reports the drop.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + {"esphome/A": "name=A\nversion=1.0\ndepends=Wire, SPI\n"}, + properties=("esphome/A",), + ) + convert_libraries([Library("esphome/A", "1.0.0", None)], _backend()) + assert "declares dependencies via library.properties" in caplog.text + + +def test_walk_warns_for_nonplatform_invalid_library( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """A dependency dropped for any cause other than the routine platform + filter is visible in every backend.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + {"esphome/A": {"name": "A", "dependencies": [{"name": "B", "version": "1.0"}]}}, + ) + calls = {"n": 0} + real = lib.check_library_data + + def flaky(data, platform, framework): + calls["n"] += 1 + if calls["n"] > 1: + raise InvalidLibrary("manifest is corrupt") + return real(data, platform, framework) + + monkeypatch.setattr(lib, "check_library_data", flaky) + convert_libraries([Library("esphome/A", None, None)], _backend()) + assert "Skipping dependency B of esphome/A: manifest is corrupt" in caplog.text + + +def test_convert_libraries_warns_for_nonplatform_invalid_dependency_component( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """A dependency component dropped for any cause other than the platform + filter warns; only the routine cross-platform skip stays at debug.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + { + "esphome/A": { + "name": "A", + "dependencies": [{"name": "C", "owner": "esphome", "version": "1.0"}], + }, + "esphome/C": {"name": "C"}, + }, + ) + real = lib.check_library_data + + def flaky(data, platform, framework): + # Fail only on C's resolved manifest, not on A's dependency entry + if data.get("name") == "C" and "version" not in data: + raise InvalidLibrary("manifest is corrupt") + return real(data, platform, framework) + + monkeypatch.setattr(lib, "check_library_data", flaky) + convert_libraries([Library("esphome/A", "1.0.0", None)], _backend()) + assert "manifest is corrupt" in caplog.text + assert "Skipping dependency" in caplog.text + + def test_split_flag_entry_non_string_is_clean() -> None: """A dict or number from a third-party manifest fails naming the entry, not with an opaque shlex traceback."""