From 1ba8b838dafd6b9527c7b43269f44e16973914a6 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 11 May 2026 10:58:17 -0500 Subject: [PATCH] [cli] Address Copilot review on --prebuilt-dir - get_ltchiptool_path: use 'Scripts/' on Windows, 'bin/' elsewhere when falling back to PlatformIO's libretiny penv. CPython venvs put scripts under Scripts/ on win32 and bin/ on POSIX; the prior hardcoded 'bin' would never have found ltchiptool on Windows. - _load_idedata: wrap json.loads on the prebuilt idedata.json in a try/except and re-raise as EsphomeError with a one-line diagnostic so the failure mode is a clean error instead of an unhandled JSONDecodeError stack trace. Update the surrounding comment to match the new behavior. - CORE.prebuilt_dir docstring: drop the dead 'docs/architecture/...' pointer (no docs/ tree in this repo); point at esphome-docs#6600 and device-builder#572 instead. New regression test: - test_load_idedata_prebuilt_malformed_json_raises_esphomeerror Updated test: - test_get_ltchiptool_path_pio_penv now uses Scripts/ on win32 to match the new platform-aware lookup. Issue: esphome/device-builder#572 Issue: esphome/device-builder#570 --- esphome/core/__init__.py | 4 +++- esphome/platformio/toolchain.py | 16 +++++++++++---- esphome/util.py | 15 ++++++++++---- tests/unit_tests/test_platformio_toolchain.py | 20 +++++++++++++++++++ tests/unit_tests/test_util.py | 14 ++++++++++--- 5 files changed, 57 insertions(+), 12 deletions(-) diff --git a/esphome/core/__init__.py b/esphome/core/__init__.py index 17269a6fdb3..7bbd225589d 100644 --- a/esphome/core/__init__.py +++ b/esphome/core/__init__.py @@ -570,7 +570,9 @@ class EsphomeCore: # Directory of prebuilt artifacts for `esphome upload --prebuilt-dir`. # When set, firmware/partition/bootloader resolution and the idedata # cache prefer files under this directory over the local build tree. - # See docs/architecture/upload-prebuilt-dir. + # User-facing docs live in esphome/esphome-docs#6600 + # (`guides/cli.mdx`); see esphome/device-builder#572 for the + # cross-platform layout contract that motivates the flag. self.prebuilt_dir: Path | None = None # The validated configuration, this is None until the config has been validated self.config: ConfigType | None = None diff --git a/esphome/platformio/toolchain.py b/esphome/platformio/toolchain.py index 83faec84aa9..f86bf75781c 100644 --- a/esphome/platformio/toolchain.py +++ b/esphome/platformio/toolchain.py @@ -161,13 +161,21 @@ def _load_idedata(config): # idedata.json with absolute paths on every install. # # No schema validation or referenced-path existence check happens here; - # a malformed prebuilt idedata.json will surface as a downstream - # "file not found" from esptool / picotool. That's an acceptable trade - # for keeping this path zero-cost when the dashboard's contract is met. + # a missing path inside the idedata will surface later as a "file not + # found" from esptool / picotool. A malformed JSON file is caught here + # and surfaced as EsphomeError so the failure mode is a one-line + # diagnostic instead of an unhandled JSONDecodeError stack trace. if CORE.prebuilt_dir is not None: prebuilt_idedata = CORE.prebuilt_dir / "idedata.json" if prebuilt_idedata.is_file(): - data = json.loads(prebuilt_idedata.read_text(encoding="utf-8")) + try: + data = json.loads(prebuilt_idedata.read_text(encoding="utf-8")) + except json.JSONDecodeError as err: + raise EsphomeError( + f"Failed to parse {prebuilt_idedata}: {err}. The dashboard " + "must stage a syntactically valid idedata.json under " + "--prebuilt-dir; the upload cannot proceed without it." + ) from err _resolve_prebuilt_idedata_paths(data, CORE.prebuilt_dir) return data diff --git a/esphome/util.py b/esphome/util.py index 97428af2330..7c60594a3fb 100644 --- a/esphome/util.py +++ b/esphome/util.py @@ -413,8 +413,10 @@ def get_ltchiptool_path() -> Path | None: Order: 1. ``ltchiptool`` on PATH (pip-installed system-wide or in a virtualenv). - 2. PlatformIO's libretiny penv at ``~/.platformio/penv/.libretiny/bin`` - (where platform-libretiny installs it during its package init). + 2. PlatformIO's libretiny penv (where platform-libretiny installs it + during its package init). The script subdirectory follows the + CPython venv convention: ``Scripts/`` on Windows, ``bin/`` + elsewhere. Returns None if neither is available; callers should surface an actionable error pointing the user at one of those install paths. @@ -422,13 +424,18 @@ def get_ltchiptool_path() -> Path | None: on_path = shutil.which("ltchiptool") if on_path is not None: return Path(on_path) - binary_name = "ltchiptool.exe" if sys.platform == "win32" else "ltchiptool" + if sys.platform == "win32": + bin_subdir = "Scripts" + binary_name = "ltchiptool.exe" + else: + bin_subdir = "bin" + binary_name = "ltchiptool" pio_penv = ( Path.home() / ".platformio" / "penv" / LTCHIPTOOL_PIO_PENV_NAME - / "bin" + / bin_subdir / binary_name ) if pio_penv.is_file(): diff --git a/tests/unit_tests/test_platformio_toolchain.py b/tests/unit_tests/test_platformio_toolchain.py index 8d4b17ca7e6..bd43f20d141 100644 --- a/tests/unit_tests/test_platformio_toolchain.py +++ b/tests/unit_tests/test_platformio_toolchain.py @@ -336,6 +336,26 @@ def test_load_idedata_absolute_paths_in_prebuilt_pass_through( assert result["extra"]["flash_images"][0]["path"] == abs_bootloader +def test_load_idedata_prebuilt_malformed_json_raises_esphomeerror( + setup_core: Path, mock_run_platformio_cli_run: Mock +) -> None: + """A malformed prebuilt idedata.json must surface as a one-line + EsphomeError, not an unhandled JSONDecodeError stack trace; the + dashboard's transparent-install flow needs a clean diagnostic to + bubble back to the operator, not the contents of the broken file.""" + CORE.build_path = str(setup_core / "build" / "test") + CORE.name = "test" + + prebuilt_dir = setup_core / "prebuilt" + prebuilt_dir.mkdir() + (prebuilt_dir / "idedata.json").write_text("{not valid json") + CORE.prebuilt_dir = prebuilt_dir + + with pytest.raises(EsphomeError, match="Failed to parse"): + toolchain._load_idedata({"name": "test"}) + mock_run_platformio_cli_run.assert_not_called() + + def test_load_idedata_falls_back_when_prebuilt_idedata_missing( setup_core: Path, mock_run_platformio_cli_run: Mock ) -> None: diff --git a/tests/unit_tests/test_util.py b/tests/unit_tests/test_util.py index 689df1719d6..ac0a1572047 100644 --- a/tests/unit_tests/test_util.py +++ b/tests/unit_tests/test_util.py @@ -595,10 +595,18 @@ def test_get_ltchiptool_path_on_path(tmp_path: Path) -> None: def test_get_ltchiptool_path_pio_penv(tmp_path: Path) -> None: """Fall back to PlatformIO's libretiny penv when ltchiptool isn't on - PATH; this is the install location platform-libretiny uses.""" + PATH; this is the install location platform-libretiny uses. The + script subdir follows the CPython venv convention -- ``Scripts/`` + on Windows, ``bin/`` elsewhere -- which matches what PlatformIO + creates.""" fake_home = tmp_path / "home" - binary_name = "ltchiptool.exe" if sys.platform == "win32" else "ltchiptool" - pio_bin = fake_home / ".platformio" / "penv" / ".libretiny" / "bin" + if sys.platform == "win32": + bin_subdir = "Scripts" + binary_name = "ltchiptool.exe" + else: + bin_subdir = "bin" + binary_name = "ltchiptool" + pio_bin = fake_home / ".platformio" / "penv" / ".libretiny" / bin_subdir pio_bin.mkdir(parents=True) expected = pio_bin / binary_name expected.touch()