From 1483b7c10bba07c541426e713a2bd17653a04ecf Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 20 Aug 2026 16:24:21 -0500 Subject: [PATCH] Detect launchers by a closed denylist instead of guessing compiler names --- esphome/build_helpers/idedata.py | 53 +++++++------------ esphome/platformio/library.py | 6 ++- .../unit_tests/build_helpers/test_idedata.py | 41 +++++++------- 3 files changed, 44 insertions(+), 56 deletions(-) diff --git a/esphome/build_helpers/idedata.py b/esphome/build_helpers/idedata.py index 42dbccfdae..e36c95de30 100644 --- a/esphome/build_helpers/idedata.py +++ b/esphome/build_helpers/idedata.py @@ -15,7 +15,6 @@ import json import logging import os from pathlib import Path -import re import shlex import subprocess @@ -123,27 +122,15 @@ def _pick_entry(entries: list[dict]) -> dict: raise ValueError("no C++ translation unit found in compile_commands.json") -# The compiler basename a compile_commands entry must lead with (an -# optional target-triple prefix ends in one of these) -# The stem must BE a compiler name (optionally versioned), alone or after a -# target-triple separator: "cc", "xtensa-lx106-elf-g++", "gcc-8.4.0" match; -# "ccache" and "distcc" do not. -_COMPILER_STEM = re.compile( - r"(?:^|[-_.])(?:gcc|g\+\+|cc|c\+\+|clang|clang\+\+)(?:-[\d.]+)?$" -) +# Compiler launchers that may prefix a compile command. A closed denylist is +# sturdier than trying to enumerate compiler names: launchers are few and +# stable, while compilers (cross prefixes, versioned names, icx, armcc, ...) +# are an open set. +_LAUNCHER_STEMS = frozenset({"ccache", "sccache", "distcc", "icecc", "buildcache"}) -# Deduplicates the warning across a compile DB of hundreds of entries; -# cleared per idedata build (a module-level functools.cache would suppress -# the warning for the process lifetime in non-forking hosts). -_warned_not_a_compiler: set[str] = set() - - -def _warn_not_a_compiler(token: str) -> None: - if token in _warned_not_a_compiler: - return - _warned_not_a_compiler.add(token) - _LOGGER.warning("compile_commands entry does not start with a compiler: %s", token) +def _is_launcher(token: str) -> bool: + return Path(token).stem.lower() in _LAUNCHER_STEMS def parse_entry( @@ -169,16 +156,11 @@ def parse_entry( # build, so this is a comparison, not a guess by name. if launcher is not None and tokens[0] == launcher: tokens = tokens[1:] - if ( - not _COMPILER_STEM.search(Path(tokens[0]).stem) - and len(tokens) > 1 - and _COMPILER_STEM.search(Path(tokens[1]).stem) - ): + 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]) tokens = tokens[1:] - if not _COMPILER_STEM.search(Path(tokens[0]).stem): - _warn_not_a_compiler(tokens[0]) # 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("\\", "/") @@ -296,15 +278,19 @@ def load_or_build_idedata( data = idedata_from_build(compile_commands, launcher) data["prog_path"] = str(elf_path) - if _COMPILER_STEM.search(Path(data["cxx_path"]).stem): + if _is_launcher(data["cxx_path"]): + # Serve the data for this run but never persist a launcher as the + # compiler path (the cache would outlive the timestamp check and + # hide the fault) + _LOGGER.warning( + "compile_commands names the launcher %s as the compiler; " + "not caching idedata", + data["cxx_path"], + ) + else: cache.parent.mkdir(parents=True, exist_ok=True) # Atomic so a crash mid-write cannot leave a truncated cache write_file(cache, json.dumps(data, indent=2) + "\n") - else: - # parse_entry already warned; serve the data for this run but never - # persist a known-bad compiler path (the cache would outlive the - # timestamp check and hide the fault) - _LOGGER.debug("Not caching idedata with unrecognized compiler path") return data @@ -318,7 +304,6 @@ def idedata_from_build(compile_commands: Path, launcher: str | None = None) -> d ESPHome TU, but union the include dirs across all ESPHome TUs to get a project-wide superset (as PlatformIO's idedata provides). """ - _warned_not_a_compiler.clear() entries = json.loads(Path(compile_commands).read_text(encoding="utf-8")) # ninja-generated compile DBs repeat one identical multi-KB command per diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index ccc4c3d36c..f008451160 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -565,13 +565,17 @@ def split_flag_entry(entry: str, owner: str) -> list[str]: raise EsphomeError(f"Malformed build flag {entry!r} in {owner}: {err}") from err +# Flags whose argument may follow as a separate token; ParseFlags glues them +BARE_ARG_FLAGS = frozenset({"-I", "-L", "-l", "-D"}) + + def join_flag_args(tokens: Iterable[str], owner: str) -> list[str]: """Join a bare ``-I``/``-L``/``-l``/``-D`` with its following token, the way PlatformIO's ParseFlags lexes them.""" out: list[str] = [] it = iter(tokens) for tok in it: - if tok in ("-I", "-L", "-l", "-D"): + if tok in BARE_ARG_FLAGS: arg = next(it, None) if arg is None: _LOGGER.warning("Ignoring trailing '%s' in %s build flags", tok, owner) diff --git a/tests/unit_tests/build_helpers/test_idedata.py b/tests/unit_tests/build_helpers/test_idedata.py index 51f5b2384c..4e403b04a6 100644 --- a/tests/unit_tests/build_helpers/test_idedata.py +++ b/tests/unit_tests/build_helpers/test_idedata.py @@ -292,23 +292,19 @@ def test_parse_entry_recovers_from_unconfigured_launcher( ) cxx_path, _, _, _ = idedata.parse_entry(entry) assert cxx_path == "/tools/xtensa-lx106-elf-g++" - assert "does not start with a compiler" not in caplog.text + assert "WARNING" not in caplog.text -def test_parse_entry_warns_when_first_token_is_not_a_compiler( - caplog: pytest.LogCaptureFixture, -) -> None: - """An unrecoverable non-compiler leading token is warned about, once per - path however many entries the compile DB has.""" - idedata._warned_not_a_compiler.clear() +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.""" entry = _entry( f"{ABS}build", f"{ABS}build/src/esphome/core/application.cpp", - "/usr/bin/python3 wrapper.py -c a.cpp -o a.o", + "/opt/homebrew/bin/ccache -c a.cpp -o a.o", ) - idedata.parse_entry(entry) - idedata.parse_entry(entry) - assert caplog.text.count("does not start with a compiler") == 1 + cxx_path, _, _, _ = idedata.parse_entry(entry) + assert cxx_path == "/opt/homebrew/bin/ccache" def _write_compile_commands(tmp_path: Path) -> Path: @@ -405,12 +401,12 @@ def test_load_or_build_idedata_rebuilds_non_dict_cache(tmp_path: Path) -> None: assert "cc_path" in data -def test_parse_entry_accepts_versioned_compilers() -> None: - """Versioned compiler names (g++-13, gcc-8.4.0) are not warned about.""" - for stem in ("g++-13", "gcc-8.4.0", "clang++-17"): - assert idedata._COMPILER_STEM.search(stem) - assert not idedata._COMPILER_STEM.search("ccache") - assert not idedata._COMPILER_STEM.search("distcc") +def test_is_launcher_matches_only_known_launchers() -> None: + """Compilers of any shape pass; only the closed launcher set matches.""" + for token in ("/t/g++-13", "gcc-8.4.0", "clang++-17", "armcc", "icx", "cc"): + assert not idedata._is_launcher(token) + for token in ("/opt/homebrew/bin/ccache", "CCACHE.EXE", "distcc", "sccache"): + assert idedata._is_launcher(token) def test_load_or_build_idedata_corrupted_cache_is_logged( @@ -429,8 +425,10 @@ def test_load_or_build_idedata_corrupted_cache_is_logged( assert "Discarding unreadable idedata cache" in caplog.text -def test_load_or_build_idedata_never_caches_bad_compiler(tmp_path: Path) -> None: - """Idedata whose compiler path failed the sanity check is served for this +def test_load_or_build_idedata_never_caches_a_launcher( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """Idedata whose compiler path is a known launcher is served for this run but not persisted, so the next build re-parses.""" compile_commands = tmp_path / "compile_commands.json" compile_commands.write_text( @@ -439,7 +437,7 @@ def test_load_or_build_idedata_never_caches_bad_compiler(tmp_path: Path) -> None _entry( f"{ABS}build", f"{ABS}build/src/esphome/core/application.cpp", - "/usr/bin/python3 wrapper.py -c app.cpp -o app.cpp.o", + "/opt/homebrew/bin/ccache -c app.cpp -o app.cpp.o", ) ] ) @@ -449,8 +447,9 @@ def test_load_or_build_idedata_never_caches_bad_compiler(tmp_path: Path) -> None data = idedata.load_or_build_idedata( compile_commands, tmp_path / "f.elf", cache ) - assert data["cxx_path"] == "/usr/bin/python3" + assert data["cxx_path"] == "/opt/homebrew/bin/ccache" assert not cache.exists() + assert "not caching idedata" in caplog.text def test_load_or_build_idedata_cache_hit_skips_rebuild(tmp_path: Path) -> None: