From 8230c16f1255079cb690e0ee4dae0072483c00be Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 23:21:04 -0500 Subject: [PATCH] Order the launcher strip before the empty guard, never dedupe rsp commands, harden the size summary --- esphome/build_helpers/idedata.py | 28 ++++++++----- esphome/espidf/size_summary.py | 17 +++++++- .../unit_tests/build_helpers/test_idedata.py | 41 +++++++++++++++++-- tests/unit_tests/test_size_summary.py | 22 ++++++++++ 4 files changed, 92 insertions(+), 16 deletions(-) diff --git a/esphome/build_helpers/idedata.py b/esphome/build_helpers/idedata.py index 04f190952b..be1f7fd25c 100644 --- a/esphome/build_helpers/idedata.py +++ b/esphome/build_helpers/idedata.py @@ -161,17 +161,19 @@ def parse_entry( raw = os.path.normpath(directory / raw) return raw.replace("\\", "/") + # A launcher-wrapped command ("ccache g++ ...") names the compiler second + if launcher is not None and tokens[:1] == [launcher]: + tokens = tokens[1:] if not tokens: - # _split_command("") is [] by design; fail like _pick_entry does + # _split_command("") is [] by design, and a command that is only + # the launcher strips to nothing; fail like _pick_entry does # instead of an IndexError traceback raise ValueError(f"empty compile command for {entry.get('file')}") - # A launcher-wrapped command ("ccache g++ ...") names the compiler second - if launcher is not None and tokens[0] == launcher: - tokens = tokens[1:] if _is_launcher(tokens[0]) and len(tokens) > 1 and not tokens[1].startswith("-"): # A stale compile DB built with a launcher the current run no longer - # configures: the real compiler is the next token. - _LOGGER.debug("Stripping unconfigured launcher %s", tokens[0]) + # configures: the real compiler is the next token. Warn: the DB is + # stale and worth regenerating. + _LOGGER.warning("Stripping unconfigured launcher %s", tokens[0]) tokens = tokens[1:] # 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. @@ -332,11 +334,14 @@ def idedata_from_build(compile_commands: Path, launcher: str | None = None) -> d def _shape(entry: dict) -> str: # The command minus its TU-specific paths: entries sharing a shape # carry identical include sets (one ninja rule), so tokenize once - # per shape instead of once per TU - return ( - entry["command"] - .replace(entry.get("file", ""), "") - .replace(entry.get("output", ""), "") + # 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. + command = entry["command"] + if "@" in command: + return f"unique:{entry['file']}" + return command.replace(entry.get("file", ""), "").replace( + entry.get("output", ""), "" ) seen_shapes = {_shape(representative)} @@ -345,6 +350,7 @@ def idedata_from_build(compile_commands: Path, launcher: str | None = None) -> d continue has_esphome_tu = True if (shape := _shape(entry)) in seen_shapes: + _LOGGER.debug("Include union: %s shares a command shape", entry["file"]) continue seen_shapes.add(shape) for inc in parse_entry(entry, launcher)[2]: diff --git a/esphome/espidf/size_summary.py b/esphome/espidf/size_summary.py index 83791ce797..f558e724e2 100644 --- a/esphome/espidf/size_summary.py +++ b/esphome/espidf/size_summary.py @@ -85,6 +85,12 @@ def print_summary(size_json: Path, partitions_csv: Path | None) -> None: _LOGGER.warning("Skipping size summary: %s", e) return + if not isinstance(data, dict): + # Valid JSON that is not an object (truncated tool output) must + # not raise past a build that already linked + _LOGGER.warning("Skipping size summary: unexpected shape in %s", size_json) + return + memory_types = data.get("memory_types", {}) ram_region = memory_types.get("DRAM") or memory_types.get("DIRAM") or {} ram_used = ram_region.get("used") @@ -105,7 +111,16 @@ def print_summary(size_json: Path, partitions_csv: Path | None) -> None: return try: app_size = _find_app_partition_size(partitions_csv) - except ValueError as e: + except (ValueError, OSError) as e: _LOGGER.warning("Skipping Flash summary: %s", e) return + if app_size <= 0: + # A malformed partition row parses to 0; a 0% bar would feed CI's + # memory-impact extraction fabricated data + _LOGGER.warning( + "Skipping Flash summary: app partition size is %s in %s", + app_size, + partitions_csv, + ) + return print_size_line("Flash", image_size, app_size) diff --git a/tests/unit_tests/build_helpers/test_idedata.py b/tests/unit_tests/build_helpers/test_idedata.py index 4294d2e6ab..dd2494a093 100644 --- a/tests/unit_tests/build_helpers/test_idedata.py +++ b/tests/unit_tests/build_helpers/test_idedata.py @@ -151,11 +151,20 @@ def test_is_esphome_src_handles_backslash_paths() -> None: assert not idedata._is_esphome_src(r"C:\b\src\esphome\core\app.h") -def test_parse_entry_empty_command_raises() -> None: - """A blank command fails with a named ValueError, not an IndexError.""" - entry = {"directory": "/b", "file": "/b/src/x.cpp", "command": ""} +@pytest.mark.parametrize( + ("command", "launcher"), + [ + ("", None), + # A command that is only the launcher strips to nothing + ("/usr/bin/ccache", "/usr/bin/ccache"), + ], +) +def test_parse_entry_empty_command_raises(command: str, launcher: str | None) -> None: + """A blank (or launcher-only) command fails with a named ValueError, + not an IndexError.""" + entry = {"directory": "/b", "file": "/b/src/x.cpp", "command": command} with pytest.raises(ValueError, match="empty compile command"): - idedata.parse_entry(entry) + idedata.parse_entry(entry, launcher) def test_idedata_from_build_empty_includes_raises(tmp_path: Path) -> None: @@ -180,6 +189,30 @@ def test_idedata_from_build_empty_includes_raises(tmp_path: Path) -> None: idedata.idedata_from_build(compile_commands) +def test_idedata_from_build_rsp_commands_never_dedupe(tmp_path: Path) -> None: + """Per-object response files strip to one shape while holding different + include sets; @-commands must tokenize per TU.""" + entries = [] + for name in ("a", "b"): + rsp = tmp_path / f"{name}.cpp.o.rsp" + rsp.write_text(f"-I{ABS}inc/{name}") + file = f"{ABS}build/src/esphome/core/{name}.cpp" + entries.append( + { + "directory": str(tmp_path), + "file": file, + "command": f"/tools/g++ @{rsp.name} -c {file} -o {name}.o", + "output": f"{name}.o", + } + ) + compile_commands = tmp_path / "compile_commands.json" + compile_commands.write_text(json.dumps(entries)) + with patch.object(idedata, "get_toolchain_includes", return_value=[]): + data = idedata.idedata_from_build(compile_commands) + joined = " ".join(data["includes"]["build"]) + assert "inc/a" in joined and "inc/b" in joined + + def test_idedata_from_build_dedupes_identical_command_shapes( tmp_path: Path, ) -> None: diff --git a/tests/unit_tests/test_size_summary.py b/tests/unit_tests/test_size_summary.py index 7a536a6838..c5d117b953 100644 --- a/tests/unit_tests/test_size_summary.py +++ b/tests/unit_tests/test_size_summary.py @@ -193,3 +193,25 @@ def test_print_summary_missing_flash_inputs_warn( size_json = _write_size_json(tmp_path, data) print_summary(size_json, partitions_csv=tmp_path / "partitions.cssv") assert "no image_size" in caplog.text + + +def test_print_summary_non_dict_json_warns(tmp_path, caplog) -> None: + """Valid JSON that is not an object must warn, not raise past a build + that already linked.""" + size_json = tmp_path / "size.json" + size_json.write_text("[]") + print_summary(size_json, tmp_path / "partitions.csv") + assert "unexpected shape" in caplog.text + + +def test_print_summary_zero_app_partition_warns(tmp_path, caplog) -> None: + """A malformed partition row parsing to 0 must not render a 0% bar for + CI's memory-impact extraction to ingest.""" + size_json = tmp_path / "size.json" + size_json.write_text( + '{"memory_types": {"DRAM": {"used": 1, "size": 2}}, "image_size": 100}' + ) + partitions = tmp_path / "partitions.csv" + partitions.write_text("app0, app, ota_0, 0x10000, ,\n") + print_summary(size_json, partitions) + assert "app partition size is" in caplog.text