From 3635c05fa5069a6133234f595c33662fc3e23f0c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 15:36:49 -0500 Subject: [PATCH] Non-destructive platform_version check, honest compdb errors, missing-binutils refusal The platform_version pop defaults to the schema spec so a second validation pass over an already-validated dict cannot warn about a key the user never set. An unparsable compile database now fails naming the parse error and the offending output instead of blaming renamed ninja rules. analyze-memory validates the native objdump/readelf exist and fails by tool name rather than silently analyzing with host binutils. The shared smoke-test helper is renamed _toolchain_components_to_test (it serves the esp32 PlatformIO job too), and the decode-dedup state is cleared by an autouse fixture instead of by hand. --- esphome/__main__.py | 13 +++++++ esphome/arduino8266/toolchain.py | 8 +++-- esphome/components/esp8266/__init__.py | 5 ++- script/determine-jobs.py | 6 ++-- .../esp8266/test_toolchain_validation.py | 10 +++--- .../unit_tests/test_arduino8266_toolchain.py | 19 ++++++++--- tests/unit_tests/test_main.py | 34 ++++++++++++++++--- 7 files changed, 75 insertions(+), 20 deletions(-) diff --git a/esphome/__main__.py b/esphome/__main__.py index ffed484699..5dc9403ed9 100644 --- a/esphome/__main__.py +++ b/esphome/__main__.py @@ -2022,6 +2022,19 @@ def command_analyze_memory(args: ArgsProtocol, config: ConfigType) -> int: # Get idedata for analysis idedata = None if native_toolchain is not None: + for tool in ( + native_toolchain.get_objdump_path(), + native_toolchain.get_readelf_path(), + ): + if not tool.is_file(): + # The analyzer would silently fall back to host binutils, + # which cannot read the target ELF + _LOGGER.error( + "%s is missing; the toolchain install may be incomplete " + "(run 'esphome clean-all')", + tool, + ) + return 1 objdump_path = str(native_toolchain.get_objdump_path()) readelf_path = str(native_toolchain.get_readelf_path()) diff --git a/esphome/arduino8266/toolchain.py b/esphome/arduino8266/toolchain.py index a4f5243fac..76cf415748 100644 --- a/esphome/arduino8266/toolchain.py +++ b/esphome/arduino8266/toolchain.py @@ -151,8 +151,12 @@ def _write_compile_commands( raise EsphomeError(f"Could not generate compile_commands.json: {result.stderr}") try: entries = json.loads(result.stdout) - except ValueError: - entries = None + except ValueError as err: + (build_dir / "compile_commands.json").unlink(missing_ok=True) + raise EsphomeError( + f"ninja produced an unparsable compile database: {err} " + f"(output starts {result.stdout[:120]!r})" + ) from err if not entries: # compdb exits 0 with [] for unknown rule names; a renamed compile # rule must fail the build, not silently strand every consumer diff --git a/esphome/components/esp8266/__init__.py b/esphome/components/esp8266/__init__.py index f89cd81ec2..c9720ffeef 100644 --- a/esphome/components/esp8266/__init__.py +++ b/esphome/components/esp8266/__init__.py @@ -131,7 +131,10 @@ def _validate_native_toolchain(config: ConfigType) -> ConfigType: # platform_version is a PlatformIO concept; drop it (as esp32's native # toolchain does), warning when a custom pin is discarded. The floor # above guarantees the schema-derived default is the ARDUINO_4 spec. - if conf.pop(CONF_PLATFORM_VERSION, None) != _ARDUINO_4_PLATFORM_SPEC: + if ( + conf.pop(CONF_PLATFORM_VERSION, _ARDUINO_4_PLATFORM_SPEC) + != _ARDUINO_4_PLATFORM_SPEC + ): _LOGGER.warning( "'platform_version' is ignored by 'toolchain: arduino'; the native " "toolchain downloads the framework and compiler directly" diff --git a/script/determine-jobs.py b/script/determine-jobs.py index 4da68c8eb0..8fd541a6d3 100755 --- a/script/determine-jobs.py +++ b/script/determine-jobs.py @@ -596,12 +596,12 @@ def esp32_platformio_components_to_test(branch: str | None = None) -> list[str]: Returns: Sorted list of component names to compile. """ - return _native_components_to_test( + return _toolchain_components_to_test( branch, ESP32_PLATFORMIO_TEST_COMPONENTS, _esp32_platformio_path_or_file_trigger ) -def _native_components_to_test( +def _toolchain_components_to_test( branch: str | None, test_set: frozenset[str] | set[str], infra_trigger: Callable[[list[str]], bool], @@ -690,7 +690,7 @@ def esp8266_native_components_to_test(branch: str | None = None) -> list[str]: list on core or infrastructure changes, otherwise the intersection with the changed-component dependency closure (empty list skips the job). """ - return _native_components_to_test( + return _toolchain_components_to_test( branch, ESP8266_NATIVE_TEST_COMPONENTS, _esp8266_native_path_or_file_trigger ) diff --git a/tests/unit_tests/components/esp8266/test_toolchain_validation.py b/tests/unit_tests/components/esp8266/test_toolchain_validation.py index 6582180e19..f2f7c4632a 100644 --- a/tests/unit_tests/components/esp8266/test_toolchain_validation.py +++ b/tests/unit_tests/components/esp8266/test_toolchain_validation.py @@ -2,6 +2,7 @@ from __future__ import annotations +from collections.abc import Generator from pathlib import Path from types import SimpleNamespace from unittest.mock import patch @@ -29,9 +30,12 @@ from esphome.types import ConfigType @pytest.fixture(autouse=True) -def _arduino_toolchain() -> None: +def _arduino_toolchain() -> Generator[None]: # The suite-wide reset_core fixture clears CORE.toolchain after each test CORE.toolchain = Toolchain.ARDUINO + esp8266._DECODE_WARNED_AT.clear() + yield + esp8266._DECODE_WARNED_AT.clear() def _config( @@ -123,7 +127,6 @@ def test_decode_pc_native_missing_tools_warns_once( ) -> None: """A stack dump of many addresses produces one missing-tool warning.""" - esp8266._DECODE_WARNED_AT.clear() with ( patch( "esphome.arduino8266.toolchain.get_addr2line_path", @@ -137,7 +140,6 @@ def test_decode_pc_native_missing_tools_warns_once( esp8266._decode_pc({}, "40201234") esp8266._decode_pc({}, "40201238") assert caplog.text.count("Cannot decode crash addresses") == 1 - esp8266._DECODE_WARNED_AT.clear() def test_decode_pc_platformio_missing_tools_warns_once( @@ -147,14 +149,12 @@ def test_decode_pc_platformio_missing_tools_warns_once( warning level as the native one; raw undecoded addresses with no stated reason are undiagnosable at default log level.""" - esp8266._DECODE_WARNED_AT.clear() CORE.toolchain = Toolchain.PLATFORMIO idedata = SimpleNamespace(addr2line_path=None, firmware_elf_path=None) with patch("esphome.platformio.toolchain.get_idedata", return_value=idedata): esp8266._decode_pc({}, "40201234") esp8266._decode_pc({}, "40201238") assert caplog.text.count("Cannot decode crash addresses") == 1 - esp8266._DECODE_WARNED_AT.clear() def test_resolve_toolchain_rejects_unsupported() -> None: diff --git a/tests/unit_tests/test_arduino8266_toolchain.py b/tests/unit_tests/test_arduino8266_toolchain.py index a4182ad905..49363fcff4 100644 --- a/tests/unit_tests/test_arduino8266_toolchain.py +++ b/tests/unit_tests/test_arduino8266_toolchain.py @@ -158,10 +158,19 @@ def test_write_compile_commands(tmp_path: Path) -> None: assert (build_dir / "compile_commands.json").read_text() == entries -@pytest.mark.parametrize("stdout", ["[]\n", "not json"]) -def test_write_compile_commands_empty_db_raises(tmp_path: Path, stdout: str) -> None: - """An empty compile database (compdb exits 0 with [] for unknown rule - names) must fail the build and drop any stale database.""" +@pytest.mark.parametrize( + ("stdout", "match"), + [ + ("[]\n", "empty compile database"), + # A parse failure names its cause, not the rule-name story + ("not json", "unparsable compile database.*not json"), + ], +) +def test_write_compile_commands_bad_db_raises( + tmp_path: Path, stdout: str, match: str +) -> None: + """An empty or unparsable compile database fails the build with its + actual cause and drops any stale database.""" build_dir = tmp_path / "build" build_dir.mkdir() (build_dir / "compile_commands.json").write_text("[stale]") @@ -171,7 +180,7 @@ def test_write_compile_commands_empty_db_raises(tmp_path: Path, stdout: str) -> "run", return_value=MagicMock(returncode=0, stdout=stdout), ), - pytest.raises(EsphomeError, match="empty compile database"), + pytest.raises(EsphomeError, match=match), ): toolchain._write_compile_commands(tmp_path / "ninja", build_dir, {}) assert not (build_dir / "compile_commands.json").exists() diff --git a/tests/unit_tests/test_main.py b/tests/unit_tests/test_main.py index eb54dbad85..ff81e2f9b3 100644 --- a/tests/unit_tests/test_main.py +++ b/tests/unit_tests/test_main.py @@ -7223,9 +7223,15 @@ def test_command_analyze_memory_native_toolchains( CORE.toolchain = toolchain config = {CONF_ESPHOME: {CONF_NAME: "test_device"}} + # The tools must exist: a missing binutils now fails by name instead of + # silently falling back to host tools + objdump = tmp_path / "objdump" + readelf = tmp_path / "readelf" + objdump.write_text("") + readelf.write_text("") with ( - patch(f"{module}.get_objdump_path", return_value=Path("/tc/objdump")), - patch(f"{module}.get_readelf_path", return_value=Path("/tc/readelf")), + patch(f"{module}.get_objdump_path", return_value=objdump), + patch(f"{module}.get_readelf_path", return_value=readelf), patch(f"{module}.get_elf_path", return_value=Path("/build/firmware.elf")), ): result = command_analyze_memory(MockArgs(), config) @@ -7234,13 +7240,33 @@ def test_command_analyze_memory_native_toolchains( # str(Path(...)) so the expectation matches the platform's separators mock_memory_analyzer_cli.assert_called_once_with( str(Path("/build/firmware.elf")), - str(Path("/tc/objdump")), - str(Path("/tc/readelf")), + str(objdump), + str(readelf), set(), idedata=None, ) +def test_command_analyze_memory_missing_binutils_fails_by_name( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A truncated toolchain install fails naming the missing tool instead + of silently analyzing with host binutils.""" + setup_core(platform="esp8266", tmp_path=tmp_path, name="test_device") + CORE.toolchain = Toolchain.ARDUINO + config = {CONF_ESPHOME: {CONF_NAME: "test_device"}} + module = "esphome.arduino8266.toolchain" + with ( + patch(f"{module}.get_objdump_path", return_value=tmp_path / "missing-objdump"), + patch(f"{module}.get_readelf_path", return_value=tmp_path / "readelf"), + patch("esphome.__main__.write_cpp", return_value=0), + patch("esphome.__main__.compile_program", return_value=0), + ): + assert command_analyze_memory(MockArgs(), config) == 1 + assert "missing-objdump" in caplog.text + assert "toolchain install may be incomplete" in caplog.text + + def test_command_idedata_incompatible_toolchain(tmp_path: Path) -> None: """A non-native, non-platformio toolchain errors out cleanly.""" setup_core(platform=PLATFORM_ESP32, tmp_path=tmp_path)