From eb958bd0a0cfd1e8dab16635900de120914f1480 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 13:34:19 -0500 Subject: [PATCH] Quote CPPDEFINES, warn on env access, degrade unmodelled subscripts, realify InvalidLibrary CPPDEFINES joins the quoted buckets (a spaced define no longer splits across tokens) and its tuple branch validates the pair elements. The fake env warns on attribute access rather than call, so hasattr/ truthiness branches are diagnosable, with dunder probes excluded; an unmodelled subscript warns and returns '' instead of a KeyError discarding the whole capture. Malformed platforms/frameworks values raise plain InvalidLibrary, giving the non-platform warning branches a real producer, and their tests use real manifests instead of monkeypatched raisers. The version-less dependency drop moves to debug: bundled names (Wire, SPI) made it per-build noise nobody can act on. --- esphome/platformio/extra_script.py | 39 ++++++++++++------- esphome/platformio/library.py | 9 ++++- .../test_platformio_extra_script.py | 10 +++-- tests/unit_tests/test_platformio_library.py | 32 +++++---------- 4 files changed, 49 insertions(+), 41 deletions(-) diff --git a/esphome/platformio/extra_script.py b/esphome/platformio/extra_script.py index f29cf32951..bdef357671 100644 --- a/esphome/platformio/extra_script.py +++ b/esphome/platformio/extra_script.py @@ -127,9 +127,14 @@ class _FakeSConsEnv: return self._vars.get(key, default) def __getitem__(self, key: str) -> str: - # Scripts also read env["BOARD_MCU"]; without this the broad - # handler would discard every flag the script captured - return self._vars[key] + # Scripts also read env["BOARD_MCU"]; an unmodelled subscript + # degrades one branch instead of discarding the whole capture + if key not in self._vars and key not in self._warned_gets: + self._warned_gets.add(key) + _LOGGER.warning( + "PIO extra-script env[%r] is not modelled; returning ''", key + ) + return self._vars.get(key, "") def Append(self, **kwargs) -> None: # noqa: N802 (SCons API name) for key, value in kwargs.items(): @@ -155,14 +160,18 @@ class _FakeSConsEnv: # ----- Everything else is a no-op so unsupported scripts don't crash ----- def __getattr__(self, name: str): + if name.startswith("__") and name.endswith("__"): + # Protocol probes (copy, pickle, iteration) are not script calls + raise AttributeError(name) + if name not in self._warned_methods: + # Warn on access, not call: hasattr()/truthiness branches would + # otherwise silently take the wrong path; a script whose whole + # effect is env.Replace() stays diagnosable either way + self._warned_methods.add(name) + _LOGGER.warning("PIO extra-script env.%s is not supported; ignoring", name) + def _noop(*args, **kwargs): - # Once per method: a script whose whole effect is env.Replace() - # must be diagnosable from a normal build log - if name not in self._warned_methods: - self._warned_methods.add(name) - _LOGGER.warning( - "PIO extra-script env.%s(...) is not supported; ignoring", name - ) + return None return _noop @@ -278,10 +287,14 @@ def captured_as_build_flags( for define in result.cppdefines: # SCons also accepts dict/list CPPDEFINES; formatting those blind # would hand the compiler garbage like -D{'FOO': '1'} - if isinstance(define, (tuple, list)) and len(define) == 2: - flags.append(f"-D{define[0]}={define[1]}") + if ( + isinstance(define, (tuple, list)) + and len(define) == 2 + and all(isinstance(part, (str, int)) for part in define) + ): + flags.append(shlex.quote(f"-D{define[0]}={define[1]}")) elif isinstance(define, str): - flags.append(f"-D{define}") + flags.append(shlex.quote(f"-D{define}")) else: _LOGGER.warning("Ignoring unsupported CPPDEFINES entry %r", define) # Each captured entry is one argv token in SCons; quote so the diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index f2a9971814..963e10c255 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -438,6 +438,9 @@ def check_library_data(data: dict, platform: str | None, framework: str): if isinstance(platforms, str): platforms = [a.strip() for a in platforms.split(",")] platforms = ensure_list(platforms) + if not all(isinstance(pf, str) for pf in platforms): + # A real (non-platform) manifest problem; callers warn, not skip + raise InvalidLibrary(f"Malformed platforms value: {platforms!r}") # Check if library supports the target platform valid_platforms = platform is None or "*" in platforms or platform in platforms @@ -449,6 +452,8 @@ def check_library_data(data: dict, platform: str | None, framework: str): if isinstance(frameworks, str): frameworks = [a.strip() for a in frameworks.split(",")] frameworks = ensure_list(frameworks) + if not all(isinstance(fw, str) for fw in frameworks): + raise InvalidLibrary(f"Malformed frameworks value: {frameworks!r}") # Check if library declares the active framework. PIO library manifests # often list only "arduino" even when the library actually compiles fine @@ -1029,8 +1034,8 @@ def convert_libraries( ): if "version" not in dependency: # Cannot resolve from the registry; common for bundled - # names (Wire, SPI) -- add_library() is the fix if real - _LOGGER.info( + # names (Wire, SPI) -- unactionable noise above debug + _LOGGER.debug( "Skip version-less dependency %r of %s", dependency.get("name"), component.name, diff --git a/tests/unit_tests/test_platformio_extra_script.py b/tests/unit_tests/test_platformio_extra_script.py index a4eeb5bf41..e2484408eb 100644 --- a/tests/unit_tests/test_platformio_extra_script.py +++ b/tests/unit_tests/test_platformio_extra_script.py @@ -270,7 +270,7 @@ def test_apply_extra_script_ignores_uncaptured_env_calls(tmp_path, caplog) -> No apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") assert c.data["build"]["flags"] == ["-lsingle"] assert "env.Append(UNCAPTURED=...) is not captured" in caplog.text - assert "env.Replace(...) is not supported" in caplog.text + assert "env.Replace is not supported" in caplog.text def test_apply_extra_script_swallows_script_errors(tmp_path, caplog) -> None: @@ -378,7 +378,7 @@ def test_unsupported_env_method_warns_once(caplog) -> None: ) env.Replace(CC="clang") env.Replace(CC="gcc") - assert caplog.text.count("env.Replace(...) is not supported") == 1 + assert caplog.text.count("env.Replace is not supported") == 1 def test_run_extra_script_sys_exit_is_best_effort(tmp_path, caplog) -> None: @@ -458,10 +458,14 @@ def test_env_get_unknown_key_warns_once(caplog) -> None: def test_spaced_linkflag_survives_relexing(tmp_path) -> None: """A captured argv token with a space stays one token after lexing.""" result = ExtraScriptResult( - linkflags=["-Wl,-T my linker.ld"], cppflags=["-include my hdr.h"] + linkflags=["-Wl,-T my linker.ld"], + cppflags=["-include my hdr.h"], + cppdefines=[("MSG", '"hello world"'), "PLAIN"], ) flags = captured_as_build_flags(result, library_dir=tmp_path) assert lex_build_flags(flags, "test") == [ + '-DMSG="hello world"', + "-DPLAIN", "-Wl,-T my linker.ld", "-include my hdr.h", ] diff --git a/tests/unit_tests/test_platformio_library.py b/tests/unit_tests/test_platformio_library.py index 3eb4f2aa75..da4dba842f 100644 --- a/tests/unit_tests/test_platformio_library.py +++ b/tests/unit_tests/test_platformio_library.py @@ -672,20 +672,15 @@ def test_walk_warns_for_nonplatform_invalid_library( _patch_download_with_manifests( monkeypatch, tmp_path, - {"esphome/A": {"name": "A", "dependencies": [{"name": "B", "version": "1.0"}]}}, + { + "esphome/A": { + "name": "A", + "dependencies": [{"name": "B", "version": "1.0", "platforms": [123]}], + } + }, ) - 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 + assert "Skipping dependency B of esphome/A: Malformed platforms" in caplog.text def test_convert_libraries_warns_for_nonplatform_invalid_dependency_component( @@ -701,20 +696,11 @@ def test_convert_libraries_warns_for_nonplatform_invalid_dependency_component( "name": "A", "dependencies": [{"name": "C", "owner": "esphome", "version": "1.0"}], }, - "esphome/C": {"name": "C"}, + "esphome/C": {"name": "C", "frameworks": [None]}, }, ) - 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 "Malformed frameworks" in caplog.text assert "Skipping dependency" in caplog.text