From c9b4086a928d201250360813d49618900483ddcc Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 26 Aug 2026 09:33:06 -0500 Subject: [PATCH 1/2] Trim comments and docstrings to repo standard --- esphome/build_gen/arduino8266.py | 30 ++++-------- esphome/build_helpers/idedata.py | 8 ++- esphome/build_helpers/pch.py | 35 +++++--------- esphome/platformio/pch.py.script | 83 ++++++++++++-------------------- 4 files changed, 54 insertions(+), 102 deletions(-) diff --git a/esphome/build_gen/arduino8266.py b/esphome/build_gen/arduino8266.py index 5f377ab2c2..38f488efea 100644 --- a/esphome/build_gen/arduino8266.py +++ b/esphome/build_gen/arduino8266.py @@ -1218,37 +1218,30 @@ def write_project(paths: InstalledPaths, ccache: str | None) -> bool: lines.append(f"srcflags = {' '.join(src_other + include_flags)}") src_cxx_override = None if pch_enabled() and any(tok.startswith("-include") for tok in cxxflags): - # GCC only loads a .gch while no tokens precede it, and the cxx rule - # expands $cxxflags before $flags: a user -include in build_flags - # means every TU would silently skip the .gch + # $cxxflags expands first, so a user -include there means GCC would + # never load the .gch _LOGGER.warning( "A -include in build_flags prevents the precompiled header from " "loading; compiling without it" ) elif pch_enabled(): # C++ src edges swap the force-includes for one precompiled prefix - # header holding the same content plus defines.h; C and assembly - # edges keep srcflags (a .gch is a C++ artifact) - # The opt-out hint matters when a toolchain rejects its own .gch: - # the build stays correct but every TU warns via -Winvalid-pch + # header (same content plus defines.h); C/assembly keep srcflags pch_header = build_dir / PCH_HEADER_NAME pch_includes = (*src_includes, PCH_CORE_HEADER) pch_text = pch_header_text(pch_includes) checksum = None try: if ccache: - # The .sum sidecar only exists for CCACHE_PCH_EXTSUM; ninja's - # depfile handles staleness. Mirror CCACHE_BASEDIR: strip the - # per-device build path so identically-configured devices - # produce identical .sum files and share cache entries - # Raw path too: a symlinked build dir resolves differently + # The .sum exists only for CCACHE_PCH_EXTSUM; ninja's depfile + # handles staleness. Strip resolved and raw build paths + # (symlinks) so identical configs share cache entries flags_id = ( " ".join(cxxflags) .replace(effective_ccache_basedir(), "") .replace(str(CORE.build_path), "") ) - # The header text covers include order, which the sorted - # closure alone does not + # The header text covers include order checksum = pch_checksum( src_dir, pch_includes, @@ -1271,9 +1264,8 @@ def write_project(paths: InstalledPaths, ccache: str | None) -> bool: ) write_file_if_changed(pch_header, pch_text) if checksum is not None: - # Valid only for a ninja run started by write_project: a - # direct ninja invocation can rebuild the .gch via its - # depfile while this generate-time .sum lags behind + # Generate-time stamp: a hand-run ninja can rebuild the .gch + # while this .sum lags write_file_if_changed( build_dir / f"{PCH_HEADER_NAME}.gch.sum", checksum + "\n" ) @@ -1281,9 +1273,7 @@ def write_project(paths: InstalledPaths, ccache: str | None) -> bool: lines.append(f"build {gch}: pch {_e(pch_header)}") if src_other: lines.append(f" flags = {' '.join(src_other)}") - # Relative -include (resolved from the ninja cwd, where the header - # lives): an absolute path would put the per-device build path on - # every compile command and defeat cross-device ccache sharing + # Relative -include: absolute would break cross-device ccache. # -Wno-error keeps a rejected .gch a warning under user -Werror cxx_parts = src_other + [ f"-Winvalid-pch -Wno-error=invalid-pch -include {PCH_HEADER_NAME}" diff --git a/esphome/build_helpers/idedata.py b/esphome/build_helpers/idedata.py index 033d2bea0d..c5acab40d6 100644 --- a/esphome/build_helpers/idedata.py +++ b/esphome/build_helpers/idedata.py @@ -202,9 +202,8 @@ def parse_entry( if tok in ("-c", "-o"): next(it, None) # drop the flag and its argument (input/output) elif tok == "-include": - # -include searches the compile cwd first, then the -I chain, so - # only re-anchor paths that really live next to the compile (the - # pch); a name meant for the -I chain must stay untouched + # Re-anchor only names next to the compile (the pch); a name + # meant for the -I chain must stay untouched raw = next(it, "") if not raw: _LOGGER.warning("Dropping -include with no argument") @@ -232,8 +231,7 @@ def parse_entry( else: cxx_flags.append(tok) for raw in unresolved_force_includes: - # A deleted build artifact (clean_build removes esphome_pch.h) would - # otherwise surface only as an opaque downstream tooling error + # A deleted build artifact would otherwise surface only downstream if not any((Path(inc) / raw).is_file() for inc in includes): _LOGGER.warning( "-include %s found neither next to the compile nor on the " diff --git a/esphome/build_helpers/pch.py b/esphome/build_helpers/pch.py index 4bf16eb9ff..3b9be56a21 100644 --- a/esphome/build_helpers/pch.py +++ b/esphome/build_helpers/pch.py @@ -1,11 +1,8 @@ """Shared precompiled-header policy for the build backends. -Safe by construction when the prefix header mirrors what the TUs already -include first (ESP8266); a backend may instead inject a curated set of -self-contained core headers (ESP-IDF). User sources from ``esphome: -includes:`` also receive the prefix; the Arduino.h visibility this gives -them on Arduino platforms is intended behavior (see esphome#8693, which -made defines.h -> macros.h include it everywhere). +The prefix either mirrors the TUs' own force-includes (ESP8266) or is a +curated core-header set (ESP-IDF). ``esphome: includes:`` sources receive +it too; Arduino.h visibility there is intended (esphome#8693). """ from __future__ import annotations @@ -66,13 +63,10 @@ def ccache_pch_env() -> dict[str, str]: missing := [ t for t in ("pch_defines", "time_macros") - # Set membership: substring matching could be fooled by a token - # that merely contains one of ours if t not in {tok.strip() for tok in user_sloppiness.split(",")} ] ): - # Without these ccache declines every pch-consuming compile; union - # rather than override so the user's own tokens survive + # Without these ccache declines every pch-consuming compile env["CCACHE_SLOPPINESS"] = ",".join((user_sloppiness, *missing)) _LOGGER.warning( "Adding %s to CCACHE_SLOPPINESS so ccache can cache compiles " @@ -94,14 +88,11 @@ def pch_header_text(include_headers: Iterable[str]) -> str: def _include_closure(src_dir: Path, roots: Iterable[str]) -> dict[str, bytes]: - """Quoted-include closure of ``roots``: src-relative name -> contents. + """Include closure of ``roots``: src-relative name -> contents. - Resolves each include against the includer's directory first, then the - src root, matching the compiler's quoted-include lookup. Names that do - not resolve under ``src_dir`` end the walk; they live in versioned - framework/toolchain installs the caller identifies separately. - Over-approximates (no #ifdef evaluation) — the safe direction for - cache invalidation. + Resolution mirrors the compiler (includer's dir, then src root); names + outside ``src_dir`` end the walk and are versioned by the caller. No + #ifdef evaluation: over-approximating is the safe direction. """ seen: dict[str, bytes] = {} stack: list[tuple[str, str]] = [(name, "") for name in roots] @@ -118,18 +109,15 @@ def _include_closure(src_dir: Path, roots: Iterable[str]) -> dict[str, bytes]: try: data = (src_dir / rel).read_bytes() except OSError as err: - # mtime/size keep a changed-but-unreadable header shifting the - # digest without device paths in it; if stat also fails the - # header's identity is unknown and the OSError propagates so - # callers compile without a pch + # mtime/size still shift the digest; a stat failure propagates + # so callers compile without a pch _LOGGER.warning("Could not read %s for the pch checksum: %s", rel, err) st = (src_dir / rel).stat() data = f"".encode() seen[rel] = data parent = posixpath.dirname(rel) stack.extend( - # surrogateescape: a non-UTF-8 include name must not abort the - # build; it simply will not resolve and ends the walk + # surrogateescape: a non-UTF-8 name just fails to resolve (inc.decode(errors="surrogateescape"), parent) for inc in _INCLUDE_RE.findall(data) ) @@ -146,7 +134,6 @@ def pch_checksum( digest = hashlib.sha256() closure = _include_closure(src_dir, include_headers) for name in sorted(closure): - # surrogateescape round-trips names from non-UTF-8 filesystems digest.update(name.encode(errors="surrogateescape")) digest.update(closure[name]) digest.update(b"\0") diff --git a/esphome/platformio/pch.py.script b/esphome/platformio/pch.py.script index 13efa3ff21..d8f2980747 100644 --- a/esphome/platformio/pch.py.script +++ b/esphome/platformio/pch.py.script @@ -16,14 +16,10 @@ except Exception as err: # noqa: BLE001 -- not exported under -t nobuild projenv = None _projenv_error = err -# Precompile the src force-includes plus defines.h (which pulls in -# Arduino.h on Arduino platforms) and force-include the result into C++ src -# compiles only; those TUs already include this content first, so their -# preprocessed output is unchanged. Post script: the final src flags exist, -# nothing compiled yet. Registration is gated host-side (the platform's -# __init__.py, pch_enabled()). Keep the header tail, ccache values, -# include-closure recipe, and the checksum/failed-marker stamp flow in sync -# with build_helpers/pch.py. +# Precompile the src force-includes plus defines.h and force-include the +# result into C++ src compiles only; their preprocessed output is unchanged. +# Registration is gated host-side (pch_enabled()). Keep the closure, ccache +# values, and stamp flow in sync with build_helpers/pch.py. # Compiler failures that clear on their own must not latch the .failed marker _TRANSIENT_ERRORS = ("No space left", "Cannot allocate", "Resource temporarily") @@ -50,8 +46,7 @@ def _include_closure(src_dir: Path, roots: list) -> dict: try: data = (src_dir / rel).read_bytes() except OSError as err: - # If stat also fails the identity is unknown: the OSError - # propagates to the outer handler, which skips the pch + # A stat failure propagates and the outer handler skips the pch print(f"ESPHome: could not read {rel} for the pch checksum: {err}") st = (src_dir / rel).stat() data = f"".encode() @@ -66,9 +61,8 @@ def _include_closure(src_dir: Path, roots: list) -> dict: def _shell_arg(element) -> str: """One compiler argv from one SCons element, matching the real spawn: - SCons whole-quotes spaced elements, the shell unquotes the rest. On - Windows there is no POSIX shell pass and shlex would eat path - backslashes.""" + spaced elements pass whole, the rest get one shell unquote (skipped on + Windows, where shlex would eat path backslashes).""" arg = str(element) if " " in arg or os.name == "nt": return arg.replace('\\"', '"') @@ -80,8 +74,7 @@ def _shell_arg(element) -> str: print(f"ESPHome: could not lex flag {arg!r} for the pch: {err}") return arg if len(tokens) != 1: - # The shell-quoting model is wrong for this element; say so rather - # than surfacing only as downstream pch warnings + # The quoting model is wrong for this element; leave a trail print(f"ESPHome: passing flag {arg!r} through unlexed for the pch") return arg return tokens[0] @@ -89,9 +82,8 @@ def _shell_arg(element) -> str: def _compile_gch(cxx, flags, header: Path, gch: Path, proj_dir: Path): """Compile the .gch, then probe that the toolchain can load it back - (GCC 10 on macOS arm64 builds one it then rejects per-process: "had - text segment at different address"). Returns a deterministic error - string or None; OSError propagates for transient handling.""" + (GCC 10 on macOS arm64 rejects its own per-process). Returns an error + string or None; OSError propagates as transient.""" result = subprocess.run( # noqa: PLW1510 [cxx, "-x", "c++-header", *flags, "-c", str(header), "-o", str(gch)], cwd=proj_dir, @@ -101,8 +93,7 @@ def _compile_gch(cxx, flags, header: Path, gch: Path, proj_dir: Path): text=True, ) if result.returncode < 0: - # Signal-killed (OOM, ^C) is environmental; raising OSError routes - # it to the transient no-marker path + # Signal-killed (OOM, ^C): route to the transient no-marker path raise OSError(f"compiler killed by signal {-result.returncode}") if result.returncode != 0: return result.stderr @@ -143,10 +134,8 @@ def _setup_pch() -> None: # Expected under -t nobuild; anything else must leave a trail print(f"ESPHome: projenv unavailable ({_projenv_error}); skipping pch") return - # Project root, not $BUILD_DIR: SCons compiles run with the project dir - # as cwd, so "-include esphome_pch.h" resolves here as a relative path. - # An absolute path would put the per-device build path on every compile - # command and defeat cross-device ccache sharing. + # Project root: SCons compiles run here, so the relative -include + # resolves; an absolute path would break cross-device ccache sharing. proj_dir = Path(env.subst("$PROJECT_DIR")) # noqa: F821 src_dir = Path(env.subst("$PROJECT_SRC_DIR")) # noqa: F821 header = proj_dir / "esphome_pch.h" @@ -170,14 +159,12 @@ def _setup_pch() -> None: if any(not name for name in include_headers): print("ESPHome: build_src_flags has a trailing -include; skipping pch") return - # Fold only names that resolve under src/: consumers keep their own - # -include entries, so an unguarded user header folded here would be - # included twice. An unfolded header simply stays consumer-only and - # ccache hashes it directly off the command line. + # Fold only relative names resolving under src/: consumers keep their + # own -include entries, so folding an unguarded user header would + # include it twice; unfolded ones stay consumer-only. folded = [ name for name in include_headers - # An absolute name would sneak past src_dir /: keep it consumer-only if not Path(name).is_absolute() and (src_dir / name).is_file() ] if unfolded := [n for n in include_headers if n not in folded]: @@ -200,15 +187,13 @@ def _setup_pch() -> None: try: version = platform.get_package_version(package) except KeyError: - # Only trust KeyError as "absent" when the package really is not - # installed; an unresolved manifest must not hash as a constant + # An unresolved manifest must not hash as a constant if platform.get_package(package) is not None: print(f"ESPHome: skipping precompiled header: no version for {package}") return version = None # absent optional package except Exception as err: # noqa: BLE001 - # Without trustworthy package identity a stale .gch could be - # reused across upgrades; skip the pch instead + # No trustworthy package identity: a stale .gch could survive print(f"ESPHome: skipping precompiled header: {err}") return digest.update(f"{package}={version}".encode()) @@ -218,9 +203,8 @@ def _setup_pch() -> None: digest.update(rel.encode(errors="surrogateescape")) digest.update(closure[rel]) digest.update(b"\0") - # Project-local include dirs outside src (e.g. rp2's lwip_override) - # hold generated headers the src closure cannot see; hash them so an - # ESPHome-side change invalidates an existing build dir + # Project-local -I dirs (e.g. rp2's lwip_override) hold generated + # headers the src closure cannot see; hash them too prev = "" for tok in flags: inc = tok[2:] if tok.startswith("-I") and len(tok) > 2 else "" @@ -234,9 +218,8 @@ def _setup_pch() -> None: inc_dir.is_dir() and inc_dir.is_relative_to(proj_dir) and not inc_dir.is_relative_to(src_dir) - # lib_deps trees are not part of the prefix closure today (the - # roots resolve under src/ only); walking them would read every - # library file on every build for nothing + # Library trees never enter the prefix closure; walking them + # would read every library file each build and not inc_dir.is_relative_to(proj_dir / ".piolibdeps") and not inc_dir.is_relative_to(proj_dir / ".pioenvs") ): @@ -250,8 +233,7 @@ def _setup_pch() -> None: try: data = local.read_bytes() except OSError as err: - # mtime/size keep a changed-but-unreadable header shifting - # the digest; a stat failure propagates and skips the pch + # mtime/size still shift the digest; stat failure skips the pch print(f"ESPHome: could not read {local} for the pch checksum: {err}") st = local.stat() data = f"".encode() @@ -297,9 +279,8 @@ def _setup_pch() -> None: failed_marker.unlink(missing_ok=True) sum_path.write_text(checksum + "\n", encoding="utf-8") - # projenv["ENV"] aliases os.environ under PlatformIO, so these reach - # framework/library TUs too; only time_macros affects non-pch TUs (the - # trade-off ccache_pch_env documents). User-set values win. + # projenv["ENV"] aliases os.environ, so these reach all TUs; only + # time_macros affects non-pch TUs. User-set values win. for key, value in ( ("CCACHE_SLOPPINESS", "pch_defines,time_macros"), ("CCACHE_PCH_EXTSUM", "true"), @@ -311,18 +292,15 @@ def _setup_pch() -> None: tokens = {tok.strip() for tok in sloppiness.split(",")} missing = [t for t in ("pch_defines", "time_macros") if t not in tokens] if missing: - # Without these ccache declines every pch-consuming compile; - # union rather than override so the user's own tokens survive + # Without these ccache declines every pch-consuming compile projenv["ENV"]["CCACHE_SLOPPINESS"] = ",".join((sloppiness, *missing)) # noqa: F821 print(f"ESPHome: adding {','.join(missing)} to CCACHE_SLOPPINESS for the pch") - # Prepended so it is processed before the build_src_flags -include - # entries: GCC only uses a .gch while no other tokens have been seen. - # The relative name also reaches "pio run -t idedata" output; external - # consumers replaying cxx_flags must run from the project dir. + # Prepended: GCC only uses a .gch while no other tokens precede it. + # The relative name also reaches "pio run -t idedata" output. + # -Wno-error: the per-process probe can pass while a later cc1plus + # rejects the .gch; that must stay a warning under user -Werror. projenv.Prepend( # noqa: F821 - # -Wno-error: the probe is per-process, so a later cc1plus can still - # reject the .gch; that must stay a warning under user -Werror CXXFLAGS=["-Winvalid-pch", "-Wno-error=invalid-pch", "-include", header.name] ) print("ESPHome: Compiling with precompiled header") @@ -331,6 +309,5 @@ def _setup_pch() -> None: try: _setup_pch() except Exception: # noqa: BLE001 -- a speedup must never break the build - # Stable marker: an unexpected error, unlike the expected skip prints print("ESPHome: pch internal error; compiling without it") traceback.print_exc() From c10fb884302d649e5db683fb732c59b82616198a Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 26 Aug 2026 09:34:37 -0500 Subject: [PATCH 2/2] Trim comments to repo standard --- esphome/build_gen/espidf.py | 17 +++++++---------- esphome/build_helpers/pch.py | 13 +++++-------- esphome/espidf/toolchain.py | 3 +-- 3 files changed, 13 insertions(+), 20 deletions(-) diff --git a/esphome/build_gen/espidf.py b/esphome/build_gen/espidf.py index 3a8e6ac6b0..e26b3a9408 100644 --- a/esphome/build_gen/espidf.py +++ b/esphome/build_gen/espidf.py @@ -292,17 +292,15 @@ target_link_options(${{COMPONENT_LIB}} PUBLIC def _pch_cmake() -> str: """The src component's precompiled-header block (C++ TUs only). - The -include stays relative (resolved from the compilers' cwd, the - build dir, where prepare_pch() puts the header and .gch); an absolute - path would poison ccache keys with the per-device build path. + The -include stays relative (resolved from the compiler cwd, the build + dir); an absolute path would poison ccache keys. """ if not pch_enabled(): return "" return f""" -# ESPHome precompiled header (see esphome/build_helpers/pch.py). The -# OBJECT_DEPENDS edge is on the header, not the .gch: headers baked into -# a .gch drop out of the TU depfiles, and prepare_pch() touches the -# header whenever it rebuilds the .gch so consumers recompile. +# ESPHome precompiled header (see esphome/build_helpers/pch.py). +# OBJECT_DEPENDS is on the header, not the .gch: pch-baked headers drop +# out of TU depfiles, and prepare_pch() touches the header on rebuild. target_compile_options(${{COMPONENT_LIB}} PRIVATE "$<$:-Winvalid-pch>" "$<$:-include>" @@ -329,9 +327,8 @@ def prepare_pch() -> None: try: sdkconfig = sdkconfig_path.read_text(encoding="utf-8") except OSError as err: - # Fail closed: the sdkconfig is the only config-awareness the .sum - # has for options that surface via sdkconfig.h, and any stand-in - # marker would collide across devices + # Fail closed: the sdkconfig is the .sum's only config identity for + # sdkconfig.h-only options; a stand-in marker would collide _LOGGER.warning( "Could not read %s; compiling without the pch: %s", sdkconfig_path, err ) diff --git a/esphome/build_helpers/pch.py b/esphome/build_helpers/pch.py index 26545762d1..bf57a157e3 100644 --- a/esphome/build_helpers/pch.py +++ b/esphome/build_helpers/pch.py @@ -171,9 +171,8 @@ def pch_checksum( return digest.hexdigest() -# Compile-command tokens dropped when retargeting a TU's flags at the -# prefix header: source/output/depfile flags with an argument, and the -# argument-less depfile flags (the pch compile must not touch depfiles) +# Tokens dropped when retargeting a TU's flags at the prefix header +# (the pch compile must not touch depfiles) _PCH_STRIP_FLAGS_WITH_ARG = frozenset({"-o", "-c", "-MT", "-MF", "-MQ"}) _PCH_STRIP_FLAGS = frozenset({"-MD", "-MMD", "-MP", "-MM", "-M"}) @@ -225,8 +224,7 @@ def pch_compile_command( if tokens and is_launcher(tokens[0]): tokens = tokens[1:] if not tokens: - # An "arguments"-style or empty entry must skip cleanly, not spawn - # a compiler-less argv that warns on every build + # "arguments"-style or empty entries must skip, not spawn "-x ..." _LOGGER.warning("Compile database entry has no usable command, skipping pch") return None args: list[str] = [] @@ -302,9 +300,8 @@ def prepare_pch( discard_pch(build_dir) return cmd, cmd_dir = cmd_and_dir - # Stripped like ccache's own rewriting (a user CCACHE_BASEDIR wins) so - # identical configs hash identically across devices; the raw build path - # covers unresolved spellings in the compile DB + # Strip like ccache's rewriting (user CCACHE_BASEDIR wins); the raw + # build path covers unresolved (symlinked) spellings cmd_id = ( " ".join(cmd) .replace(effective_ccache_basedir(), "") diff --git a/esphome/espidf/toolchain.py b/esphome/espidf/toolchain.py index 0eccf5af79..82f64defca 100644 --- a/esphome/espidf/toolchain.py +++ b/esphome/espidf/toolchain.py @@ -536,8 +536,7 @@ def run_compile(config, verbose: bool) -> int: try: prepare_pch() except Exception: # noqa: BLE001 # pylint: disable=broad-exception-caught - # Discard so an unexpected error can never leave a stale .gch that - # GCC would silently consume; exc_info keeps the failure diagnosable + # Discard so a stale .gch can never be consumed with suppress(OSError): discard_pch() _LOGGER.warning(