Merge branch 'esp8266-native-shared-helpers' into esp8266-native-build-surgery

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(

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")

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."""