From dceeee69d3cab35414e973b7c2a8973280541e4b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 16:25:11 -0500 Subject: [PATCH] Restore the size summary to its dev shape The defensive spiral (#18668, closed) guarded inputs that cannot occur: gen_esp32part validates partitions.csv before a build can succeed, and esp_idf_size.json's realistic failures do not produce the handled shapes. This PR's real change stands alone again: the bar moves to build_helpers as print_size_line, with dev behavior everywhere else. --- esphome/build_helpers/size_summary.py | 5 +- esphome/espidf/size_summary.py | 140 +++-------- esphome/espidf/toolchain.py | 4 +- .../build_helpers/test_size_summary.py | 8 +- tests/unit_tests/test_size_summary.py | 224 +----------------- 5 files changed, 37 insertions(+), 344 deletions(-) diff --git a/esphome/build_helpers/size_summary.py b/esphome/build_helpers/size_summary.py index 138ef57a42..b888111044 100644 --- a/esphome/build_helpers/size_summary.py +++ b/esphome/build_helpers/size_summary.py @@ -5,10 +5,7 @@ from __future__ import annotations def format_bar(used: int, total: int) -> str: """Match PlatformIO's ``_format_availale_bytes`` (sic, pioupload.py) exactly.""" - 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 + pct_raw = used / total if total else 0 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 10eb6598f7..2be3634c69 100644 --- a/esphome/espidf/size_summary.py +++ b/esphome/espidf/size_summary.py @@ -26,7 +26,6 @@ from __future__ import annotations import csv import json import logging -import math from pathlib import Path from esphome.build_helpers.size_summary import print_size_line @@ -37,6 +36,8 @@ _SIZE_SUFFIXES = {"K": 1024, "M": 1024 * 1024} def _parse_size(token: str) -> int: token = token.strip() + if not token: + return 0 if token.startswith(("0x", "0X")): return int(token, 16) suffix = token[-1].upper() @@ -45,19 +46,16 @@ def _parse_size(token: str) -> int: return int(token) -def _find_app_partition_size(partitions_csv: Path) -> int | None: - """The firmware's app partition size; None when there is nothing to find. +def _find_app_partition_size(partitions_csv: Path) -> int: + """Return the size of the firmware's app partition. Mirrors PlatformIO's ``platform-espressif32/builder/main.py:: _update_max_upload_size``: take the first ``app``-type partition whose subtype is ``factory`` or ``ota_0``. Order matters because layouts like Adafruit's ``partitions-4MB-tinyuf2.csv`` repurpose ``factory`` for a UF2 bootloader before the real OTA slot, so a - naive "prefer factory" rule would pick the wrong row. No qualifying - row is legitimate absence (None); a build cannot succeed with a - missing or malformed table (gen_esp32part consumes it first), so - those states belong to the backstop -- the missing-file raise just - names that one cleanly. + naive "prefer factory" rule would pick the wrong row. Raises + ``ValueError`` if no qualifying partition is present. """ if not partitions_csv.is_file(): raise ValueError(f"partitions.csv not found at {partitions_csv}") @@ -68,115 +66,37 @@ def _find_app_partition_size(partitions_csv: Path) -> int | None: ptype, psubtype, psize = cells[1], cells[2], cells[4] if ptype in ("app", "0") and psubtype in ("factory", "ota_0"): return _parse_size(psize) - return None + raise ValueError(f"No app+factory or app+ota_0 partition in {partitions_csv}") def print_summary(size_json: Path, partitions_csv: Path | None) -> None: - """Print PlatformIO-shaped RAM and Flash one-liners; never fails the build.""" - try: - _print_summary(size_json, partitions_csv) - except Exception as e: # noqa: BLE001 # pylint: disable=broad-exception-caught - # Backstop for shapes the named guards below miss; warning so a - # regression here cannot go missing indefinitely - _LOGGER.warning( - "Skipping size summary for %s: %s: %s", - size_json, - type(e).__name__, - e, - exc_info=True, - ) + """Print PlatformIO-shaped RAM and Flash one-liners. - -def _print_summary(size_json: Path, partitions_csv: Path | None) -> None: - # The build's own POST_BUILD step writes this file; its absence or an - # unexpected shape is a regression signal, so these skips warn. - # FileNotFoundError lands in the OSError arm with the path in its text. + Failures are non-fatal: the build has already succeeded, we just couldn't + summarize. Logs the cause at debug level. + """ + if not size_json.is_file(): + _LOGGER.debug("Skipping size summary: %s not found", size_json) + return try: data = json.loads(size_json.read_text(encoding="utf-8")) - except (OSError, ValueError) as e: - # ValueError covers JSONDecodeError and a non-UTF-8 (truncated) file - _LOGGER.warning("Skipping size summary: cannot read %s: %s", size_json, e) - return - if not isinstance(data, dict): - # Non-object JSON has no .get - _LOGGER.warning("Skipping size summary: unexpected shape in %s", size_json) + except (OSError, json.JSONDecodeError) as e: + _LOGGER.debug("Skipping size summary: %s", e) return - if (ram := _ram_bar(data, size_json)) is not None: - print_size_line("RAM", *ram) - if (flash := _flash_bar(data, size_json, partitions_csv)) is not None: - print_size_line("Flash", *flash) + memory_types = data.get("memory_types", {}) + ram_region = memory_types.get("DRAM") or memory_types.get("DIRAM") or {} + 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) - -def _dict_get(mapping: object, key: str) -> object: - """dict.get that reads None from any non-dict.""" - return mapping.get(key) if isinstance(mapping, dict) else None - - -def _present_but_not_dict(value: object) -> bool: - return value is not None and not isinstance(value, dict) - - -def _is_number(value: object) -> bool: - # bool subclasses int; NaN/Infinity are valid JSON for json.loads - return ( - isinstance(value, (int, float)) - and not isinstance(value, bool) - and math.isfinite(value) - ) - - -def _ram_bar(data: dict, size_json: Path) -> tuple[int, int] | None: - """The RAM bar's (used, total), or None (already logged) to skip it.""" - memory_types = data.get("memory_types") - ram_region = None - if isinstance(memory_types, dict): - # Key presence, not truthiness: a falsy DRAM value is corrupt, not - # absent, and must not fall through to DIRAM - for key in ("DRAM", "DIRAM"): - if key in memory_types: - ram_region = memory_types[key] - break - used = _dict_get(ram_region, "used") - total = _dict_get(ram_region, "size") - if _is_number(used) and _is_number(total) and total > 0: - return int(used), int(total) - malformed = ( - _present_but_not_dict(memory_types) - or _present_but_not_dict(ram_region) - or any(v is not None and not _is_number(v) for v in (used, total)) - ) - if malformed: - # A structurally corrupt report, not a variant without the region - _LOGGER.warning("Skipping RAM summary: malformed memory_types in %s", size_json) - else: - # A variant may name its RAM region differently; healthy builds - # must not warn - _LOGGER.debug( - "Skipping RAM summary: no usable DRAM/DIRAM region in %s", size_json - ) - return None - - -def _flash_bar( - data: dict, size_json: Path, partitions_csv: Path | None -) -> tuple[int, int] | None: - """The Flash bar's (used, total), or None (already logged) to skip it. - - Owns both sides of the bar, so nothing after a print can raise: the - blanket guard is left for genuinely unforeseen shapes. - """ image_size = data.get("image_size") - if not _is_number(image_size): - _LOGGER.warning("Skipping Flash summary: no usable image_size in %s", size_json) - return None - if partitions_csv is None: - _LOGGER.debug("Skipping Flash summary: no partition table given") - return None - app_size = _find_app_partition_size(partitions_csv) - if not app_size: - # No qualifying row (a zero-size row has nothing to report either): - # legitimate for non-app layouts - _LOGGER.debug("Skipping Flash summary: no app partition in %s", partitions_csv) - return None - return int(image_size), app_size + if image_size is None or partitions_csv is None: + return + try: + app_size = _find_app_partition_size(partitions_csv) + except ValueError as e: + _LOGGER.debug("Skipping Flash summary: %s", e) + return + print_size_line("Flash", image_size, app_size) diff --git a/esphome/espidf/toolchain.py b/esphome/espidf/toolchain.py index 1fb51128bd..3c5c4803c2 100644 --- a/esphome/espidf/toolchain.py +++ b/esphome/espidf/toolchain.py @@ -456,8 +456,8 @@ def run_compile(config, verbose: bool) -> int: rc = run_idf_py(*args, jobs=config[CONF_ESPHOME].get(CONF_COMPILE_PROCESS_LIMIT)) if rc == 0: size_json = CORE.relative_build_path("build", "esp_idf_size.json") - # size_summary owns the missing-table policy - print_summary(size_json, CORE.relative_build_path("partitions.csv")) + partitions = CORE.relative_build_path("partitions.csv") + print_summary(size_json, partitions if partitions.is_file() else None) return rc diff --git a/tests/unit_tests/build_helpers/test_size_summary.py b/tests/unit_tests/build_helpers/test_size_summary.py index ff9d8f077d..231ae271b2 100644 --- a/tests/unit_tests/build_helpers/test_size_summary.py +++ b/tests/unit_tests/build_helpers/test_size_summary.py @@ -7,11 +7,9 @@ import pytest from esphome.build_helpers.size_summary import format_bar, print_size_line -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_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_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 658328d3aa..933be88476 100644 --- a/tests/unit_tests/test_size_summary.py +++ b/tests/unit_tests/test_size_summary.py @@ -3,9 +3,7 @@ from __future__ import annotations import json -import logging from pathlib import Path -from unittest.mock import patch import pytest @@ -71,18 +69,6 @@ def _s3_size_data() -> dict: } -def _dram_size_data(image_size: int = 100) -> dict: - return {"memory_types": {"DRAM": {"used": 1, "size": 2}}, "image_size": image_size} - - -def _write_partitions( - tmp_path: Path, size: str, ptype: str = "app", subtype: str = "ota_0" -) -> Path: - partitions = tmp_path / "partitions.csv" - partitions.write_text(f"app0, {ptype}, {subtype}, 0x10000, {size},\n") - return partitions - - def test_print_summary_esp32_uses_dram( tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: @@ -126,15 +112,11 @@ def test_print_summary_skips_when_diram_total_collapses( def test_print_summary_handles_missing_json( - tmp_path: Path, - capsys: pytest.CaptureFixture[str], - caplog: pytest.LogCaptureFixture, + tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: """Missing size json is non-fatal and prints nothing.""" print_summary(tmp_path / "does_not_exist.json", partitions_csv=None) assert capsys.readouterr().out == "" - assert "cannot read" in caplog.text - assert "Skipping size summary for" not in caplog.text def test_print_summary_handles_no_memory_types( @@ -144,207 +126,3 @@ def test_print_summary_handles_no_memory_types( size_json = _write_size_json(tmp_path, {"image_size": 0}) print_summary(size_json, partitions_csv=None) assert capsys.readouterr().out == "" - - -def test_print_summary_non_dict_json_is_skipped( - tmp_path: Path, capsys: pytest.CaptureFixture[str] -) -> None: - """Valid JSON that is not an object must not raise past a linked build.""" - size_json = tmp_path / "size.json" - size_json.write_text("[]") - print_summary(size_json, tmp_path / "partitions.csv") - assert capsys.readouterr().out == "" - - -def test_print_summary_unreadable_partitions_is_skipped( - tmp_path: Path, capsys: pytest.CaptureFixture[str], caplog: pytest.LogCaptureFixture -) -> None: - """An OSError reading the partition table skips the summary, not the build.""" - size_json = _write_size_json(tmp_path, _dram_size_data()) - partitions = _write_partitions(tmp_path, "1M") - real_read_text = Path.read_text - - def fail_partitions_read(self: Path, *args: object, **kwargs: object) -> str: - if self == partitions: - raise OSError("permission denied") - return real_read_text(self, *args, **kwargs) - - with patch.object(Path, "read_text", fail_partitions_read): - print_summary(size_json, partitions) - # An impossible post-build state is the backstop's business - out = capsys.readouterr().out - assert "RAM:" in out and "Flash:" not in out - assert "Skipping size summary for" in caplog.text - - -def test_print_summary_happy_path_prints_both_bars( - tmp_path: Path, capsys: pytest.CaptureFixture[str] -) -> None: - """A well-formed size report and partition table print both bars.""" - size_json = tmp_path / "size.json" - size_json.write_text( - '{"memory_types": {"DRAM": {"used": 1000, "size": 2000}}, "image_size": 100000}' - ) - partitions = _write_partitions(tmp_path, "0x180000") - print_summary(size_json, partitions) - out = capsys.readouterr().out - assert "RAM:" in out and "Flash:" in out - - -@pytest.mark.parametrize( - "payload", - [ - {"memory_types": []}, - {"memory_types": {"DRAM": 5}}, - {"memory_types": {"DRAM": {"used": "x", "size": "y"}}, "image_size": 1}, - {"memory_types": {"DRAM": []}}, - {"memory_types": {"DRAM": {"used": True, "size": True}}}, - ], -) -def test_print_summary_nested_bad_shapes_never_raise( - tmp_path: Path, - capsys: pytest.CaptureFixture[str], - caplog: pytest.LogCaptureFixture, - payload: dict, -) -> None: - """Corrupt nested shapes hit the named malformed guard, not the blanket.""" - size_json = _write_size_json(tmp_path, payload) - print_summary(size_json, None) - # No half-formed bar for CI to scrape; every payload fails before printing - assert capsys.readouterr().out == "" - assert "malformed memory_types" in caplog.text - assert "Skipping size summary for" not in caplog.text - - -def test_print_summary_absent_region_stays_quiet( - tmp_path: Path, - capsys: pytest.CaptureFixture[str], - caplog: pytest.LogCaptureFixture, -) -> None: - """A well-shaped report without DRAM/DIRAM is a variant difference, not - a broken artifact: debug, never a per-build warning.""" - size_json = _write_size_json(tmp_path, {"memory_types": {}, "image_size": 1}) - with caplog.at_level(logging.DEBUG, logger="esphome.espidf.size_summary"): - print_summary(size_json, None) - assert "RAM:" not in capsys.readouterr().out - assert "no usable DRAM/DIRAM region" in caplog.text - assert not [ - r for r in caplog.records if r.levelno >= logging.WARNING and "RAM" in r.message - ] - - -def test_print_summary_non_numeric_image_size_warns_by_name( - tmp_path: Path, - capsys: pytest.CaptureFixture[str], - caplog: pytest.LogCaptureFixture, -) -> None: - """A non-numeric image_size hits the named guard, not the blanket.""" - size_json = _write_size_json( - tmp_path, - {"memory_types": {"DRAM": {"used": 1, "size": 2}}, "image_size": "x"}, - ) - print_summary(size_json, _write_partitions(tmp_path, "0x100000")) - assert "Flash:" not in capsys.readouterr().out - assert "no usable image_size" in caplog.text - assert "Skipping size summary for" not in caplog.text - - -def test_print_summary_blanket_guard_catches_the_rest( - tmp_path: Path, - capsys: pytest.CaptureFixture[str], - caplog: pytest.LogCaptureFixture, -) -> None: - """A genuinely unforeseen failure warns via the blanket backstop and - never raises past a linked build.""" - size_json = _write_size_json(tmp_path, _dram_size_data()) - with patch( - "esphome.espidf.size_summary._flash_bar", - side_effect=RuntimeError("unforeseen"), - ): - print_summary(size_json, None) - assert "Skipping size summary for" in caplog.text - - -@pytest.mark.parametrize("cell", ["1M", "1048576"], ids=["suffixed", "decimal"]) -def test_print_summary_suffixed_size_cell( - tmp_path: Path, capsys: pytest.CaptureFixture[str], cell: str -) -> None: - """K/M suffixes and plain decimals parse like PlatformIO's rule.""" - size_json = _write_size_json(tmp_path, _dram_size_data()) - partitions = tmp_path / "partitions.csv" - partitions.write_text( - f"# comment row\nshort,row\napp0, app, ota_0, 0x10000, {cell},\n" - ) - print_summary(size_json, partitions) - assert "from 1048576 bytes" in capsys.readouterr().out - - -def test_print_summary_missing_or_appless_partitions_stay_quiet( - tmp_path: Path, - capsys: pytest.CaptureFixture[str], - caplog: pytest.LogCaptureFixture, -) -> None: - """A table without a qualifying app row is a legitimate layout: the - Flash line drops at debug, never at warning.""" - size_json = _write_size_json(tmp_path, _dram_size_data()) - partitions = _write_partitions(tmp_path, "0x1000", ptype="data", subtype="spiffs") - with caplog.at_level(logging.DEBUG, logger="esphome.espidf.size_summary"): - print_summary(size_json, partitions) - out = capsys.readouterr().out - assert "Flash:" not in out - # Quiet means debug-logged, not unlogged - assert "Skipping Flash summary: no app partition" in caplog.text - assert not [r for r in caplog.records if r.levelno >= logging.WARNING] - - -def test_print_summary_corrupt_size_json_warns( - tmp_path: Path, - capsys: pytest.CaptureFixture[str], - caplog: pytest.LogCaptureFixture, -) -> None: - """The build's own size report failing to parse is a regression signal.""" - size_json = tmp_path / "size.json" - size_json.write_text("not json {{{") - print_summary(size_json, None) - size_json.write_bytes(b"\xff\xfe\x00") - print_summary(size_json, None) - assert capsys.readouterr().out == "" - # The named arm, not the blanket, for both damage classes - assert caplog.text.count("cannot read") == 2 - assert "Skipping size summary for" not in caplog.text - - -def test_print_summary_flash_line_matches_ci_extraction( - tmp_path: Path, capsys: pytest.CaptureFixture[str] -) -> None: - """The exact padded shape script/ci_memory_impact_extract.py greps.""" - size_json = _write_size_json(tmp_path, _dram_size_data(image_size=888511)) - print_summary(size_json, _write_partitions(tmp_path, "0x1C0000")) - out = capsys.readouterr().out - assert "Flash: [===== ] 48.4% (used 888511 bytes from 1835008 bytes)" in out - - -def test_print_summary_bad_ram_region_still_prints_flash( - tmp_path: Path, capsys: pytest.CaptureFixture[str], caplog: pytest.LogCaptureFixture -) -> None: - """A malformed RAM region cannot suppress a computable Flash line.""" - size_json = _write_size_json( - tmp_path, {"memory_types": {"DRAM": 5}, "image_size": 100} - ) - print_summary(size_json, _write_partitions(tmp_path, "0x100000")) - out = capsys.readouterr().out - assert "Flash:" in out and "RAM:" not in out - assert "malformed memory_types" in caplog.text - - -def test_print_summary_missing_partitions_named_in_backstop( - tmp_path: Path, - capsys: pytest.CaptureFixture[str], - caplog: pytest.LogCaptureFixture, -) -> None: - """A vanished table is an impossible post-build state; the backstop - reports it by name instead of a bare FileNotFoundError.""" - size_json = _write_size_json(tmp_path, _dram_size_data()) - print_summary(size_json, tmp_path / "nope.csv") - assert "Flash:" not in capsys.readouterr().out - assert "partitions.csv not found" in caplog.text