From e681c441d95b430ef65550ce63c7bb8c43ee4360 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 10:48:39 -0500 Subject: [PATCH] Close the residual idedata and size-summary silent-failure findings A list-of-non-dicts compile DB now raises by name inside the best-effort tuple. The launcher rejection moved into parse_entry itself so no caller can record ccache as the compiler, and the shape key includes the entry's directory since relative includes resolve against it. format_bar raises on a non-positive total so every consumer must handle the meaningless bar; the espidf summary validates the RAM region by name (a malformed region can no longer suppress a computable Flash line) and the blanket backstop logs the traceback at debug. A new test pins that the CI trigger paths exist on disk, so a rename fails loudly. --- esphome/build_helpers/idedata.py | 13 ++++--- esphome/build_helpers/size_summary.py | 5 ++- esphome/espidf/size_summary.py | 18 +++++++--- tests/script/test_determine_jobs.py | 10 ++++++ .../unit_tests/build_helpers/test_idedata.py | 12 +++---- .../build_helpers/test_size_summary.py | 8 +++-- tests/unit_tests/test_size_summary.py | 34 +++++++++++++++++-- 7 files changed, 80 insertions(+), 20 deletions(-) diff --git a/esphome/build_helpers/idedata.py b/esphome/build_helpers/idedata.py index 88c70aacab..28700f3071 100644 --- a/esphome/build_helpers/idedata.py +++ b/esphome/build_helpers/idedata.py @@ -178,6 +178,8 @@ def parse_entry( # token0 is the compiler path; the rest of the command already uses forward # slashes on Windows, so normalize it too for a consistent idedata file. cxx_path = tokens[0].replace("\\", "/") + # Enforced here so no caller can record ccache as the compiler + reject_launcher_compiler(cxx_path) defines: list[str] = [] includes: list[str] = [] cxx_flags: list[str] = [] @@ -337,13 +339,12 @@ def idedata_from_build(compile_commands: Path, launcher: str | None = None) -> d project-wide superset (as PlatformIO's idedata provides). """ entries = json.loads(Path(compile_commands).read_text(encoding="utf-8")) - if not isinstance(entries, list): + if not isinstance(entries, list) or not all(isinstance(e, dict) for e in entries): # A TypeError here would escape IDEDATA_BEST_EFFORT_ERRORS raise EsphomeError(f"{compile_commands} is not a compile-command list") representative = _pick_entry(entries) cxx_path, defines, rep_includes, cxx_flags = parse_entry(representative, launcher) - reject_launcher_compiler(cxx_path) # Seed with the representative's includes so it is not parsed twice has_esphome_tu = _is_esphome_src(representative["file"]) @@ -357,12 +358,16 @@ def idedata_from_build(compile_commands: Path, launcher: str | None = None) -> d # per shape instead of once per TU. Response-file commands never # dedupe: per-object .rsp names strip to one shape while the files # may hold different include sets. + # Keyed on directory too: relative -I paths resolve against it, so + # identical commands in different dirs mean different include sets command = entry["command"] + directory = entry.get("directory", "") if "@" in command: - return f"unique:{entry.get('output') or command}" - return command.replace(entry.get("file", ""), "").replace( + return f"unique:{directory}|{entry.get('output') or command}" + stripped = command.replace(entry.get("file", ""), "").replace( entry.get("output", ""), "" ) + return f"{directory}|{stripped}" seen_shapes = {_shape(representative)} for entry in entries: diff --git a/esphome/build_helpers/size_summary.py b/esphome/build_helpers/size_summary.py index b888111044..138ef57a42 100644 --- a/esphome/build_helpers/size_summary.py +++ b/esphome/build_helpers/size_summary.py @@ -5,7 +5,10 @@ from __future__ import annotations def format_bar(used: int, total: int) -> str: """Match PlatformIO's ``_format_availale_bytes`` (sic, pioupload.py) exactly.""" - pct_raw = used / total if total else 0 + if total <= 0: + # A "from 0 bytes" bar is meaningless; make every caller handle it + raise ValueError(f"non-positive size total {total}") + pct_raw = used / total blocks = 10 filled = min(int(round(blocks * pct_raw)), blocks) progress = "=" * filled diff --git a/esphome/espidf/size_summary.py b/esphome/espidf/size_summary.py index 5338cd2135..c22c376dfa 100644 --- a/esphome/espidf/size_summary.py +++ b/esphome/espidf/size_summary.py @@ -79,8 +79,9 @@ def print_summary(size_json: Path, partitions_csv: Path | None) -> None: try: _print_summary(size_json, partitions_csv) except Exception as e: # noqa: BLE001 # pylint: disable=broad-exception-caught - # Backstop for nested shapes the named guards below miss + # Backstop for shapes the named guards below miss _LOGGER.warning("Skipping size summary: %s", e) + _LOGGER.debug("Size summary failure detail", exc_info=True) def _print_summary(size_json: Path, partitions_csv: Path | None) -> None: @@ -99,12 +100,21 @@ def _print_summary(size_json: Path, partitions_csv: Path | None) -> None: _LOGGER.warning("Skipping size summary: unexpected shape in %s", size_json) return - memory_types = data.get("memory_types", {}) + memory_types = data.get("memory_types") + if not isinstance(memory_types, dict): + memory_types = {} ram_region = memory_types.get("DRAM") or memory_types.get("DIRAM") or {} + if not isinstance(ram_region, dict): + ram_region = {} ram_used = ram_region.get("used") ram_total = ram_region.get("size") - if ram_total and ram_used is not None: - print_size_line("RAM", ram_used, ram_total) + # Numeric checks isolate a malformed RAM region from the Flash line below + if ( + isinstance(ram_used, (int, float)) + and isinstance(ram_total, (int, float)) + and ram_total > 0 + ): + print_size_line("RAM", int(ram_used), int(ram_total)) else: _LOGGER.warning( "Skipping RAM summary: no usable DRAM/DIRAM region in %s", size_json diff --git a/tests/script/test_determine_jobs.py b/tests/script/test_determine_jobs.py index 859e320a2d..1a568ca6c6 100644 --- a/tests/script/test_determine_jobs.py +++ b/tests/script/test_determine_jobs.py @@ -1138,6 +1138,16 @@ def test_esp_idf_infra_changed(changed_files: list[str], expected: bool) -> None assert determine_jobs._esp_idf_infra_changed(changed_files) is expected +def test_esp_idf_infra_trigger_paths_exist() -> None: + """A renamed or moved trigger module must fail here, not silently stop + forcing the esp32 IDF compile.""" + repo_root = Path(__file__).resolve().parents[2] + for file in determine_jobs.ESP_IDF_INFRA_TRIGGER_FILES: + assert (repo_root / file).is_file(), f"trigger file {file} moved or renamed" + for prefix in determine_jobs.ESP_IDF_INFRA_TRIGGER_PATH_PREFIXES: + assert (repo_root / prefix).is_dir(), f"trigger dir {prefix} moved or renamed" + + @pytest.mark.parametrize( ("changed_files", "expected_result"), [ diff --git a/tests/unit_tests/build_helpers/test_idedata.py b/tests/unit_tests/build_helpers/test_idedata.py index fec276a92c..fcf9c67086 100644 --- a/tests/unit_tests/build_helpers/test_idedata.py +++ b/tests/unit_tests/build_helpers/test_idedata.py @@ -394,16 +394,16 @@ def test_parse_entry_recovers_from_unconfigured_launcher( assert "Stripping unconfigured launcher" in caplog.text -def test_parse_entry_keeps_launcher_without_program() -> None: - """A launcher followed only by flags (no program to recover) stays as - token zero; the cache layer refuses to persist it.""" +def test_parse_entry_rejects_launcher_without_program() -> None: + """A launcher followed only by flags is rejected in the parser itself, + so no caller can record ccache as the compiler.""" entry = _entry( f"{ABS}build", f"{ABS}build/src/esphome/core/application.cpp", "/opt/homebrew/bin/ccache -c a.cpp -o a.o", ) - cxx_path, _, _, _ = idedata.parse_entry(entry) - assert cxx_path == "/opt/homebrew/bin/ccache" + with pytest.raises(EsphomeError, match="compile database is unusable"): + idedata.parse_entry(entry) def _write_compile_commands(tmp_path: Path) -> Path: @@ -623,7 +623,7 @@ def test_load_or_build_idedata_cache_hit_restamps_prog_path(tmp_path: Path) -> N def test_idedata_from_build_non_list_compile_db_raises(tmp_path: Path) -> None: """Valid JSON that is not a list raises by name, inside the best-effort tuple.""" compile_commands = tmp_path / "compile_commands.json" - for bad in ("{}", "null", '"text"'): + for bad in ("{}", "null", '"text"', '["a", "b"]', "[1, 2]"): compile_commands.write_text(bad) with pytest.raises(EsphomeError, match="not a compile-command list"): idedata.idedata_from_build(compile_commands) diff --git a/tests/unit_tests/build_helpers/test_size_summary.py b/tests/unit_tests/build_helpers/test_size_summary.py index 231ae271b2..ff9d8f077d 100644 --- a/tests/unit_tests/build_helpers/test_size_summary.py +++ b/tests/unit_tests/build_helpers/test_size_summary.py @@ -7,9 +7,11 @@ import pytest from esphome.build_helpers.size_summary import format_bar, print_size_line -def test_format_bar_zero_total() -> None: - """A zero total must not divide by zero.""" - assert format_bar(0, 0) == "[ ] 0.0% (used 0 bytes from 0 bytes)" +def test_format_bar_non_positive_total_raises() -> None: + """A meaningless "from 0 bytes" bar raises so every caller must skip it.""" + for total in (0, -1): + with pytest.raises(ValueError, match="non-positive size total"): + format_bar(0, total) def test_print_size_line_label_padding(capsys: pytest.CaptureFixture[str]) -> None: diff --git a/tests/unit_tests/test_size_summary.py b/tests/unit_tests/test_size_summary.py index 8647eec67f..b92f54028a 100644 --- a/tests/unit_tests/test_size_summary.py +++ b/tests/unit_tests/test_size_summary.py @@ -237,9 +237,39 @@ def test_print_summary_blank_partition_size_warns(tmp_path, caplog) -> None: ), ids=("non-dict-memory-types", "scalar-region", "non-numeric-sizes"), ) -def test_print_summary_nested_shapes_never_raise(tmp_path, caplog, payload) -> None: - """The blanket guard keeps unexpected nested shapes from raising.""" +def test_print_summary_nested_shapes_skip_ram_by_name( + tmp_path, caplog, payload +) -> None: + """Malformed RAM shapes hit the named guard, not the blanket backstop.""" size_json = tmp_path / "size.json" size_json.write_text(payload) print_summary(size_json, tmp_path / "partitions.csv") + assert "Skipping RAM summary" in caplog.text + + +def test_print_summary_bad_ram_region_still_prints_flash( + tmp_path, caplog, capsys +) -> None: + """A malformed RAM region cannot suppress a computable Flash line.""" + size_json = tmp_path / "size.json" + size_json.write_text( + '{"memory_types": {"DRAM": {"used": "x", "size": "y"}}, "image_size": 100}' + ) + partitions = tmp_path / "partitions.csv" + partitions.write_text("app0, app, ota_0, 0x10000, 0x100000,\n") + print_summary(size_json, partitions) + assert "Skipping RAM summary" in caplog.text + assert "Flash:" in capsys.readouterr().out + + +def test_print_summary_blanket_guard_never_raises(tmp_path, caplog) -> None: + """Shapes the named guards miss (non-numeric image_size) warn via the + blanket backstop instead of raising past a linked build.""" + size_json = tmp_path / "size.json" + size_json.write_text( + '{"memory_types": {"DRAM": {"used": 1, "size": 2}}, "image_size": "x"}' + ) + partitions = tmp_path / "partitions.csv" + partitions.write_text("app0, app, ota_0, 0x10000, 0x100000,\n") + print_summary(size_json, partitions) assert "Skipping size summary" in caplog.text