diff --git a/esphome/build_gen/arduino8266.py b/esphome/build_gen/arduino8266.py index 1ed1404db3..fa06982141 100644 --- a/esphome/build_gen/arduino8266.py +++ b/esphome/build_gen/arduino8266.py @@ -14,6 +14,7 @@ from the build flags with the same precedence as the PlatformIO builder. from __future__ import annotations +from contextlib import suppress from dataclasses import dataclass import hashlib import logging @@ -49,11 +50,7 @@ from esphome.framework_helpers import ( strip_win_long_path_prefix, ) from esphome.helpers import mkdir_p, write_file_if_changed -from esphome.platformio.library import ( - SOURCE_KIND_FOR_SUFFIX, - lex_build_flags, - raise_on_empty_arg_flags, -) +from esphome.platformio.library import SOURCE_KIND_FOR_SUFFIX, lex_build_flags if TYPE_CHECKING: from esphome.arduino8266.framework import InstalledPaths @@ -286,10 +283,8 @@ def _lexed_build_flags() -> list[str]: Lex once per build; consumers share the tokens. """ - tokens = lex_build_flags(sorted(CORE.build_flags), "esphome") - # Raises for every consumer of the shared token list - raise_on_empty_arg_flags(tokens, "build_flags") - return tokens + # The funnel warns and drops empty glued arguments (-D "") itself + return lex_build_flags(sorted(CORE.build_flags), "esphome") def _flag_defines(unflags: set[str], tokens: list[str]) -> dict[str, str]: @@ -568,8 +563,19 @@ def _project_flags( # Plain-form linker flags rejected by _project_flags: inert on a -c compile # line, so the firmware would silently lack the requested link behavior -_PLAIN_LINKER_FLAGS = ("-u", "-e", "-s", "-static", "-nostartfiles") -_PLAIN_LINKER_PREFIXES = ("-T", "-Xlinker") +# Best-effort, not exhaustive: an unlisted link-only spelling still falls +# through to the compile line +_PLAIN_LINKER_FLAGS = ( + "-u", + "-e", + "-s", + "-static", + "-nostartfiles", + "-nodefaultlibs", + "-nostdlib", + "-rdynamic", +) +_PLAIN_LINKER_PREFIXES = ("-T", "-Xlinker", "-fuse-ld=", "--specs=") def _collect_sources(root: Path, exclude: set[str] = frozenset()) -> list[Path]: @@ -602,17 +608,21 @@ def _stat_sig(path: Path) -> str: return f"unreadable:{os.urandom(8).hex()}" -def _write_note(path: Path, text: str, *, warn: bool = False) -> None: +def _write_note(path: Path, text: str, *, warn: bool = False) -> bool: """Best-effort bookkeeping write; a failure never fails the build. ``warn`` marks notes whose loss drops a diagnostic on later cached builds; a lost stamp only costs a cache miss and stays at debug. + Returns whether the write persisted, so a lost warn note can veto + the cache stamp and keep the diagnostic re-derivable. """ try: path.write_text(text, encoding="utf-8") except OSError as err: log = _LOGGER.warning if warn else _LOGGER.debug log("Could not write %s: %s", path, err) + return False + return True def generate_ld_scripts( @@ -691,13 +701,15 @@ def generate_ld_scripts( raise EsphomeError(f"Could not run {gcc}: {err}; {_CLEAN_HINT}") from err if result.returncode != 0: raise EsphomeError(f"Generating the linker script failed:\n{result.stderr}") + note_persisted = True if result.stderr.strip(): # Preprocessor warnings on the success path must reach the user # on this and every later cached build (see the re-emit below) _LOGGER.warning("Linker-script preprocessor: %s", result.stderr.strip()) - _write_note(stderr_note, result.stderr.strip(), warn=True) + note_persisted = _write_note(stderr_note, result.stderr.strip(), warn=True) else: - stderr_note.unlink(missing_ok=True) + with suppress(OSError): + stderr_note.unlink(missing_ok=True) if "SECTIONS" not in result.stdout: # A degenerate zero-exit run must not be stamped as a good cache raise EsphomeError( @@ -709,10 +721,13 @@ def generate_ld_scripts( build_surgery.apply_testing_memory_patches, content, ("iram1_0_seg",) ) write_file_if_changed(output, content) - _write_note( - stamp, - f"{stamp_content} content={hashlib.sha256(content.encode('utf-8')).hexdigest()}", - ) + if note_persisted: + # An unstamped cache re-runs -E next build, re-deriving the + # diagnostic the lost note would have re-emitted + _write_note( + stamp, + f"{stamp_content} content={hashlib.sha256(content.encode('utf-8')).hexdigest()}", + ) elif stderr_note.is_file(): # Re-emit cached preprocessor warnings on cache hits try: diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index e0c05ac207..fb4aa934a0 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -661,27 +661,14 @@ def lex_build_flags(entries: str | list[str], owner: str) -> list[str]: BARE_ARG_FLAGS = frozenset({"-I", "-L", "-l", "-D"}) -def raise_on_empty_arg_flags(tokens: list[str], owner: str) -> None: - """Reject bare ``-I``/``-D``/``-L``/``-l`` tokens left by an empty glued - argument (``-D ""``). - - Consumed by the ESP8266 native build generator (later in this chain) - for user build_flags; library manifests deliberately stay warn-and-drop. - - Lives next to ``join_flag_args`` because the bare token is its - postcondition: a trailing bare flag is warned and dropped there, so a - surviving one always means an empty argument. gcc would eat the next - flag as the argument (or add the CWD for ``-L``); always a typo. - """ - if empty := sorted({tok for tok in tokens if tok in BARE_ARG_FLAGS}): - raise EsphomeError( - f"{owner} contain empty-argument flag(s): {', '.join(empty)}" - ) - - 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.""" + the way PlatformIO's ParseFlags lexes them. + + A trailing or empty argument (``-D ""``) is warned and dropped: the + bare flag would make gcc eat the next flag as its argument (or add + the CWD for ``-L``); always a typo. + """ out: list[str] = [] it = iter(tokens) for tok in it: @@ -690,6 +677,11 @@ def join_flag_args(tokens: Iterable[str], owner: str) -> list[str]: if arg is None: _LOGGER.warning("Ignoring trailing '%s' in %s build flags", tok, owner) break + if not arg: + _LOGGER.warning( + "Ignoring '%s' with empty argument in %s build flags", tok, owner + ) + continue tok += arg out.append(tok) return out diff --git a/tests/unit_tests/build_gen/test_arduino8266.py b/tests/unit_tests/build_gen/test_arduino8266.py index 358202e613..8f0035cb3e 100644 --- a/tests/unit_tests/build_gen/test_arduino8266.py +++ b/tests/unit_tests/build_gen/test_arduino8266.py @@ -939,12 +939,15 @@ def test_vtables_conflicting_raises() -> None: _resolve("-DVTABLES_IN_DRAM", "-DVTABLES_IN_IRAM") -def test_empty_lib_flags_raise() -> None: - """A bare -L would silently add the CWD to the search path; the shared - lex point raises for every consumer.""" - CORE.build_flags = {'-L ""', '-l ""'} - with pytest.raises(EsphomeError, match=r"empty-argument flag\(s\): -L, -l"): - arduino8266._lexed_build_flags() +def test_empty_lib_flags_warned_and_dropped( + caplog: pytest.LogCaptureFixture, +) -> None: + """A bare -L would silently add the CWD to the search path; the lex + funnel warns and drops it for every consumer.""" + CORE.build_flags = {'-L ""', '-l ""', "-DFOO"} + assert arduino8266._lexed_build_flags() == ["-DFOO"] + assert "Ignoring '-L' with empty argument" in caplog.text + assert "Ignoring '-l' with empty argument" in caplog.text def test_generate_ld_scripts_surfaces_preprocessor_warnings( @@ -970,6 +973,38 @@ def test_generate_ld_scripts_surfaces_preprocessor_warnings( _run_generate_ld_scripts(paths) +def test_generate_ld_scripts_lost_warn_note_vetoes_the_stamp( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A warn note that could not persist skips the stamp, so the next build + re-runs -E and re-derives the diagnostic instead of losing it.""" + paths = _make_framework(tmp_path) + _set_flags() + result = MagicMock( + returncode=0, stdout=_COMMON_LD_H_OUTPUT, stderr="warning: something" + ) + real_write_text = Path.write_text + + def fail_note_writes(self: Path, text: str, encoding: str = "utf-8") -> int: + if self.name.endswith(".stderr"): + raise OSError("read-only build dir") + return real_write_text(self, text, encoding=encoding) + + with ( + patch.object(arduino8266.subprocess, "run", return_value=result) as run1, + patch.object(Path, "write_text", fail_note_writes), + ): + _run_generate_ld_scripts(paths) + run1.assert_called_once() + assert "Could not write" in caplog.text + + # Unstamped: the second build re-runs the preprocessor + with patch.object(arduino8266.subprocess, "run", return_value=result) as run2: + _run_generate_ld_scripts(paths) + run2.assert_called_once() + assert caplog.text.count("Linker-script preprocessor: warning: something") == 2 + + def test_build_config_mmu_knob_with_raw_mmu_flag_raises() -> None: """A variant knob plus a raw MMU_* define would split the compile line from the linker script; refuse like the no-knob case.""" @@ -1079,20 +1114,6 @@ def test_generate_ld_scripts_invalid_flash_ld_name_raises(tmp_path: Path) -> Non arduino8266.generate_ld_scripts(paths, config, "../evil.ld") -def test_write_generated_replaces_damaged_file( - tmp_path: Path, caplog: pytest.LogCaptureFixture -) -> None: - """A non-UTF-8 existing copy is logged and overwritten; the write path - still raises for real failures (now the shared helper's contract).""" - from esphome.helpers import write_file_if_changed - - target = tmp_path / "gen.ld" - target.write_bytes(b"\xff\xfe") - write_file_if_changed(target, "SECTIONS { }") - assert target.read_text(encoding="utf-8") == "SECTIONS { }" - assert "Replacing damaged file" in caplog.text - - def test_generate_ld_scripts_edited_output_regenerates(tmp_path: Path) -> None: """The stamp records the content hash, so an externally edited cached script regenerates instead of linking untrusted content.""" @@ -1395,12 +1416,15 @@ def test_board_tables_are_equal() -> None: assert set(BOARDS) == set(ESP8266_BOARD_BUILD) -def test_bare_include_and_define_raise() -> None: +def test_bare_include_and_define_dropped( + caplog: pytest.LogCaptureFixture, +) -> None: """An empty-argument -I or -D would make gcc eat the next flag as the - argument; the shared lex point raises for every consumer.""" + argument; the lex funnel warns and drops both.""" CORE.build_flags = {'-I ""', '-D ""'} - with pytest.raises(EsphomeError, match=r"empty-argument flag\(s\): -D, -I"): - arduino8266._lexed_build_flags() + assert arduino8266._lexed_build_flags() == [] + assert "Ignoring '-I' with empty argument" in caplog.text + assert "Ignoring '-D' with empty argument" in caplog.text def test_generate_ld_scripts_gcc_change_invalidates_stamp(tmp_path: Path) -> None: diff --git a/tests/unit_tests/test_platformio_library.py b/tests/unit_tests/test_platformio_library.py index cd51a592c6..c703bd1044 100644 --- a/tests/unit_tests/test_platformio_library.py +++ b/tests/unit_tests/test_platformio_library.py @@ -723,11 +723,12 @@ def test_prefetch_wave_unknown_size_falls_back_to_sequential( assert "No Content-Length for https://x/b.tar.gz" in caplog.text -def test_raise_on_empty_arg_flags() -> None: - """A surviving bare flag means an empty glued argument; reject by name.""" - with pytest.raises(EsphomeError, match=r"build_flags contain empty-argument"): - lib.raise_on_empty_arg_flags(["-DFOO", "-D", "-l"], "build_flags") - lib.raise_on_empty_arg_flags(["-DFOO", "-Iinc"], "build_flags") +def test_join_flag_args_empty_argument_warns_and_drops( + caplog: pytest.LogCaptureFixture, +) -> None: + """An empty glued argument is dropped: a bare -D would eat the next flag.""" + assert lib.lex_build_flags('-D "" -DFOO', "build_flags") == ["-DFOO"] + assert "Ignoring '-D' with empty argument in build_flags" in caplog.text def test_content_lengths_head_requests(monkeypatch: pytest.MonkeyPatch) -> None: