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.
This commit is contained in:
J. Nick Koston
2026-08-23 13:34:19 -05:00
parent a478ae4a46
commit eb958bd0a0
4 changed files with 49 additions and 41 deletions
+26 -13
View File
@@ -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
+7 -2
View File
@@ -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,
@@ -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",
]
+9 -23
View File
@@ -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