Remove use of pip in espidf.py to create the venv #265
Conversation
WalkthroughSwitches ESP-IDF venv and dependency handling to use the penv "uv" executable via penv_setup.get_executable_path; adds PLATFORMIO_DIR constant; locates LIB_SOURCE via platform.get_dir(); changes venv location to PLATFORMIO_DIR/penv/.espidf-{version} and verifies venv by resolving its Python executable. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Build as Build System
participant ESP as builder/frameworks/espidf.py
participant Platform as Platform API
participant Penv as penv_setup
participant UV as uv CLI
Build->>ESP: trigger ESP-IDF setup
ESP->>Platform: platform.get_dir()
Platform-->>ESP: platform dir (for LIB_SOURCE)
ESP->>ESP: set PLATFORMIO_DIR, penv_dir, venv_dir
ESP->>Penv: get_executable_path(penv_dir, "uv")
Penv-->>ESP: uv_exe path
ESP->>UV: uv_exe venv --clear --quiet --python "<py>" "<venv_dir>"
UV-->>ESP: exit status
ESP->>Penv: get_executable_path(venv_dir, "python")
Penv-->>ESP: python_in_venv (found / not found)
alt python found
ESP-->>Build: venv verified
else
ESP-->>Build: venv verification failed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
builder/frameworks/espidf.py (1)
1609-1642: Harden venv creation & unifyuvresolution across ESP-IDF builderI’ve confirmed two distinct ways of locating the
uvexecutable inbuilder/frameworks/espidf.py:
- In
install_python_deps()(around line 1490),UV_EXEis built fromenv.subst("$PYTHONEXE") + os.path.dirname()- In
_create_venv()(lines 1614–1642),uv_pathcomes fromget_executable_path(penv_dir, "uv")To avoid silent failures and ensure consistent behavior:
In
_create_venv():
- Immediately verify
uv_pathexists before invoking it.- Replace the
asserton the Python executable with an explicitenv.Exit(1)and stderr message.def _create_venv(venv_dir): penv_setup_path = os.path.join(platform.get_dir(), "builder") sys.path.insert(0, penv_setup_path) from penv_setup import get_executable_path penv_dir = os.path.join(PLATFORMIO_DIR, "penv") uv_path = get_executable_path(penv_dir, "uv")
if not uv_path or not os.path.isfile(uv_path):sys.stderr.write("Error: 'uv' executable not found in PlatformIO penv. ""Ensure penv_setup has installed uv or reinstall the platform.\n")env.Exit(1) if os.path.isdir(venv_dir): … env.Execute( env.VerboseAction( '"%s" venv --clear --quiet "%s"' % (uv_path, venv_dir), "Creating a new virtual environment for IDF Python dependencies using uv", ) ) python_path = get_executable_path(venv_dir, "python")
assert os.path.isfile(python_path), \"Error: Failed to create a proper virtual environment. Missing the Python executable!"
if not python_path or not os.path.isfile(python_path):sys.stderr.write("Error: Failed to create a proper virtual environment ""(Python executable missing).\n")env.Exit(1)
In
install_python_deps():
- Replace the custom
UV_EXEderivation with the sameget_executable_path()approach.- Import
get_executable_pathfrompenv_setupand point it at the sharedpenvdirectory.def install_python_deps():
PYTHON_EXE = env.subst("$PYTHONEXE")UV_EXE = os.path.join(os.path.dirname(PYTHON_EXE),"uv" + (".exe" if IS_WINDOWS else ""))
from penv_setup import get_executable_pathPYTHON_EXE = env.subst("$PYTHONEXE")penv_dir = os.path.join(PLATFORMIO_DIR, "penv")UV_EXE = get_executable_path(penv_dir, "uv") def _get_installed_uv_packages(python_exe_path): …By unifying both call sites on
get_executable_path()you eliminate path drift, and by adding explicit existence checks plus consistent error exits you prevent opaque failures in CI or user environments.
🧹 Nitpick comments (2)
builder/frameworks/espidf.py (2)
85-85: Use PLATFORMIO_DIR consistently throughout the fileGood addition. For consistency and to avoid duplicate env lookups, reuse PLATFORMIO_DIR (introduced here) when computing the IDF venv path in get_idf_venv_dir() instead of calling env.subst("$PROJECT_CORE_DIR") again.
Proposed change outside this range (for get_idf_venv_dir):
def get_idf_venv_dir(): idf_version = get_framework_version() return os.path.join(PLATFORMIO_DIR, "penv", f".espidf-{idf_version}")
1491-1523: Drop “uv” from the IDF venv dependenciesThe PyPI “uv” package isn’t needed inside the IDF virtual environment—
uvis a standalone CLI and is provided alongside thepythonexecutable rather than installed viapipin the target venv. You can remove it from thedepsdict without impacting functionality.• In builder/frameworks/espidf.py (around lines 1513–1516), update the
depsblock:deps = { - "uv": ">=0.1.0", # not needed inside the IDF venv "urllib3": "<2", "cryptography": "~=44.0.0", "pyparsing": ">=3.1.0,<4", "idf-component-manager": "~=2.2", "esp-idf-kconfig": "~=2.5.0" }• To confirm no other
uventries remain in thedepslist, run:rg -nP "^[ \t]*['\"]uv['\"][ \t]*:" builder/frameworks/espidf.py
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
builder/frameworks/espidf.py(4 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:265-275
Timestamp: 2025-08-10T16:59:15.533Z
Learning: In the ESP32 platform builder's penv_setup.py, the esptool_repo_path used in subprocess calls comes from PlatformIO's internal package management system (platform.get_package_dir()) which is a trusted source. Command injection concerns are not applicable here since this is build infrastructure code with controlled inputs, not user-facing code accepting arbitrary input.
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#225
File: builder/main.py:99-108
Timestamp: 2025-07-13T20:03:29.695Z
Learning: In the ESP32 platform builder, the subprocess call to get site-packages directory in setup_python_paths() function is intentionally left without error handling because if this basic Python operation fails, it indicates a fundamental system issue that would cause the entire build process to fail anyway.
📚 Learning: 2025-08-20T14:13:13.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-10T19:12:17.988Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:19-19
Timestamp: 2025-08-10T19:12:17.988Z
Learning: In PlatformIO ESP32 platform builds, `semantic_version` is a dependency provided by PlatformIO itself and is guaranteed to be available when the build scripts run. The module is already imported at the top level in existing files like `builder/frameworks/espidf.py`, so lazy-loading it is unnecessary.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-10T19:13:25.599Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:26-40
Timestamp: 2025-08-10T19:13:25.599Z
Learning: In the pioarduino/platform-espressif32 repository's builder/penv_setup.py file, semantic_version is imported but intentionally not listed in python_deps because it's a transitive dependency of platformio-core (version 2.10.*). When platformio is installed from the GitHub URL, semantic_version is automatically installed as one of its dependencies.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-10T16:59:15.533Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:265-275
Timestamp: 2025-08-10T16:59:15.533Z
Learning: In the ESP32 platform builder's penv_setup.py, the esptool_repo_path used in subprocess calls comes from PlatformIO's internal package management system (platform.get_package_dir()) which is a trusted source. Command injection concerns are not applicable here since this is build infrastructure code with controlled inputs, not user-facing code accepting arbitrary input.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
builder/frameworks/espidf.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (macos-15, examples/espidf-ulp-lp)
- GitHub Check: build (macos-15, examples/espidf-storage-sdcard)
- GitHub Check: build (windows-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (macos-15, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (macos-15, examples/arduino-rmt-blink)
- GitHub Check: build (macos-15, examples/tasmota)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (windows-latest, examples/espidf-coap-server)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/espidf-http-request)
- GitHub Check: build (windows-latest, examples/espidf-exceptions)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-switch)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-blink)
- GitHub Check: build (windows-latest, examples/arduino-rmt-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (1)
builder/frameworks/espidf.py (1)
351-366: LIB_SOURCE resolution via platform.get_dir() looks correctSwitching to platform.get_dir() for builder/build_lib resolves the path deterministically from the platform package root and decouples it from ProjectConfig. The flow that reshapes the environment for Arduino-only builds remains intact.
Refactor Python dependency installation to use a virtual environment and update UV executable path retrieval.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
builder/frameworks/espidf.py (5)
85-85: Rename PLATFORMIO_DIR to PIO_CORE_DIR to avoid confusion with PlatformIO’s own “PLATFORMIO_DIR” conceptThis variable actually holds $PROJECT_CORE_DIR (PlatformIO core dir). Naming it PLATFORMIO_DIR is easy to conflate with PlatformIO’s “home” dir in docs and other code. Rename for clarity and update references.
Apply these focused diffs:
-PLATFORMIO_DIR = env.subst("$PROJECT_CORE_DIR") +PIO_CORE_DIR = env.subst("$PROJECT_CORE_DIR")- penv_dir = os.path.join(PLATFORMIO_DIR, "penv") + penv_dir = os.path.join(PIO_CORE_DIR, "penv")- return os.path.join(PLATFORMIO_DIR, "penv", f".espidf-{idf_version}") + return os.path.join(PIO_CORE_DIR, "penv", f".espidf-{idf_version}")- penv_dir = os.path.join(PLATFORMIO_DIR, "penv") + penv_dir = os.path.join(PIO_CORE_DIR, "penv")Also applies to: 1495-1495, 1568-1568, 1615-1615
1491-1496: Centralize uv resolution; avoid duplicating sys.path mutationsYou resolve uv here and again in _create_venv by pushing builder/ onto sys.path and importing penv_setup each time. Centralizing this avoids path pollution and repetition. Also guarantees a single place to tweak bootstrap logic later.
Apply this minimal diff in-place:
- penv_setup_path = os.path.join(platform.get_dir(), "builder") - sys.path.insert(0, penv_setup_path) - from penv_setup import get_executable_path - - penv_dir = os.path.join(PLATFORMIO_DIR, "penv") - UV_EXE = get_executable_path(penv_dir, "uv") + UV_EXE = _get_uv_exe()And add this helper once (outside this hunk), which safely avoids duplicate sys.path entries:
def _get_uv_exe(): penv_setup_path = os.path.join(platform.get_dir(), "builder") if penv_setup_path not in sys.path: sys.path.insert(0, penv_setup_path) from penv_setup import get_executable_path penv_dir = os.path.join(PIO_CORE_DIR if 'PIO_CORE_DIR' in globals() else PLATFORMIO_DIR, "penv") return get_executable_path(penv_dir, "uv")
1610-1617: Use the centralized uv resolver and prefer explicit error handling over assert for venv verification
- Reuse the _get_uv_exe() helper here.
- Replace assert with a proper error path; asserts can be stripped with -O and don’t print stderr context in SCons.
Apply this diff:
- penv_setup_path = os.path.join(platform.get_dir(), "builder") - sys.path.insert(0, penv_setup_path) - - from penv_setup import get_executable_path - - penv_dir = os.path.join(PLATFORMIO_DIR, "penv") - uv_path = get_executable_path(penv_dir, "uv") + uv_path = _get_uv_exe()- env.Execute( + env.Execute( env.VerboseAction( '"%s" venv --clear --quiet --python "%s" "%s"' % (uv_path, env.subst("$PYTHONEXE"), venv_dir), "Creating a new virtual environment for IDF Python dependencies using uv", ) )- python_path = get_executable_path(venv_dir, "python") - - assert os.path.isfile( - python_path - ), "Error: Failed to create a proper virtual environment. Missing the Python executable!" + python_path = get_executable_path(venv_dir, "python") + if not os.path.isfile(python_path): + sys.stderr.write("Error: Failed to create a proper IDF virtual environment; Python not found at %s\n" % python_path) + env.Exit(1)Also applies to: 1630-1634, 1637-1643
1497-1512: Compatibility of uv pip list JSON parsing is acceptable; guard for future format deviationParsing name/version from uv pip list --format=json is fine. You already catch JSONDecodeError/OSError. Optional: log UV_EXE version when the parse fails to make debugging easier if uv changes output format.
1568-1568: New venv location under core penv is fine; add optional detection/cleanup for legacy venvsThe change to install IDF’s Python virtualenv under
$PROJECT_CORE_DIR/penv/.espidf‐<version>
is safe and will be recreated on first use. As a small quality-of-life improvement, you can add a one-time check for legacy venvs (which were previously created directly under the core dir) and log a hint so users can delete them and reclaim disk space.Here’s an example you could drop into your
penv_setuplogic:import os, glob, logging core_dir = PLATFORMIO_DIR # env.subst("$PROJECT_CORE_DIR") # Find any “.espidf-…” dirs not under the new penv folder legacy = [ d for d in glob.glob(os.path.join(core_dir, ".espidf-*")) if not d.startswith(os.path.join(core_dir, "penv")) ] if legacy: logging.info( "Found legacy IDF venv(s): %s; consider removing them to reclaim disk space", legacy )Or from the shell (adjust
CORE_DIRas needed for your setup):#!/usr/bin/env bash # Replace with your actual core dir if not ~/.platformio CORE_DIR="${PLATFORMIO_DIR:-$(platformio settings get project_core_dir)}" # List any hidden .espidf-* dirs outside of penv/ fd -HI '^\.espidf-' "$CORE_DIR" -t d \ ! -path "$CORE_DIR/penv/*" \ -x echo {}This will accurately locate any leftover
.espidf-*venvs outside the newpenvdirectory so users can clean them up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
builder/frameworks/espidf.py(6 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:265-275
Timestamp: 2025-08-10T16:59:15.533Z
Learning: In the ESP32 platform builder's penv_setup.py, the esptool_repo_path used in subprocess calls comes from PlatformIO's internal package management system (platform.get_package_dir()) which is a trusted source. Command injection concerns are not applicable here since this is build infrastructure code with controlled inputs, not user-facing code accepting arbitrary input.
📚 Learning: 2025-08-20T14:13:13.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-10T19:12:17.988Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:19-19
Timestamp: 2025-08-10T19:12:17.988Z
Learning: In PlatformIO ESP32 platform builds, `semantic_version` is a dependency provided by PlatformIO itself and is guaranteed to be available when the build scripts run. The module is already imported at the top level in existing files like `builder/frameworks/espidf.py`, so lazy-loading it is unnecessary.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-10T19:13:25.599Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:26-40
Timestamp: 2025-08-10T19:13:25.599Z
Learning: In the pioarduino/platform-espressif32 repository's builder/penv_setup.py file, semantic_version is imported but intentionally not listed in python_deps because it's a transitive dependency of platformio-core (version 2.10.*). When platformio is installed from the GitHub URL, semantic_version is automatically installed as one of its dependencies.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-10T16:59:15.533Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:265-275
Timestamp: 2025-08-10T16:59:15.533Z
Learning: In the ESP32 platform builder's penv_setup.py, the esptool_repo_path used in subprocess calls comes from PlatformIO's internal package management system (platform.get_package_dir()) which is a trusted source. Command injection concerns are not applicable here since this is build infrastructure code with controlled inputs, not user-facing code accepting arbitrary input.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
builder/frameworks/espidf.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/espidf-coap-server)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-switch)
- GitHub Check: build (windows-latest, examples/arduino-usb-keyboard)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-riscv)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-hello-world)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-lp)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
🔇 Additional comments (3)
builder/frameworks/espidf.py (3)
353-365: LIB_SOURCE via platform.get_dir() is a good moveSwitching to platform.get_dir() to resolve builder/build_lib removes reliance on ProjectConfig and is more robust. The copy-tree guard looks sane.
1541-1551: Nice: installing missing packages only; quotes/constraints look correctSelective install using uv pip --python and quoting each "pkg{spec}" looks good and OS-safe. This aligns with the repo’s pattern of executing Python via explicit paths (per past learnings).
1552-1559: Windows-only windows-curses install via uv is correctGood conditional and reuse of uv pip to keep all installs within the IDF venv.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
builder/frameworks/espidf.py (4)
24-60: Dynamic import of penv_setup: add defensive checks to fail fast if the module file/spec/loader is missingGood call switching to importlib and centralizing get_executable_path. Add explicit checks so failures surface with actionable messages instead of AttributeError later.
_penv_setup_file = os.path.join(platform.get_dir(), "builder", "penv_setup.py") -_spec = importlib.util.spec_from_file_location("penv_setup", _penv_setup_file) -_penv_setup = importlib.util.module_from_spec(_spec) -_spec.loader.exec_module(_penv_setup) # type: ignore[attr-defined] -get_executable_path = _penv_setup.get_executable_path +if not os.path.isfile(_penv_setup_file): + sys.stderr.write(f"Error: Missing penv_setup.py at {_penv_setup_file}\n") + env.Exit(1) +_spec = importlib.util.spec_from_file_location("penv_setup", _penv_setup_file) +if _spec is None or _spec.loader is None: + sys.stderr.write("Error: Failed to load spec for penv_setup module\n") + env.Exit(1) +_penv_setup = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(_penv_setup) # type: ignore[attr-defined] +get_executable_path = _penv_setup.get_executable_path
1498-1500: Fail fast if uv executable is not resolvableSince subsequent steps rely on uv, surface a clear error early rather than failing deeper in subprocess calls.
-def _get_uv_exe(): - return get_executable_path(os.path.join(PLATFORMIO_DIR, "penv"), "uv") +def _get_uv_exe(): + uv_path = get_executable_path(os.path.join(PLATFORMIO_DIR, "penv"), "uv") + if not uv_path or not os.path.isfile(uv_path): + sys.stderr.write( + "Error: 'uv' executable not found in PlatformIO penv. " + "Ensure builder/penv_setup.py installed it correctly.\n" + ) + env.Exit(1) + return uv_path
1503-1568: Normalize package-name keys to avoid case-sensitivity mismatchesuv pip list can return names with varying case. Normalize to lower-case once to make membership checks robust (especially for windows-curses).
- for p in packages: - result[p["name"]] = pepver_to_semver(p["version"]) + for p in packages: + result[p["name"].lower()] = pepver_to_semver(p["version"])
1618-1645: Venv creation and verification: prefer explicit error handling over assertAsserts can be stripped with Python -O. Use an explicit check and env.Exit to guarantee the error is raised. Also, the user-facing message already mentions uv; good clarity.
- # Verify that the venv was created successfully by checking for Python executable - python_path = get_executable_path(venv_dir, "python") - - assert os.path.isfile( - python_path - ), "Error: Failed to create a proper virtual environment. Missing the Python executable!" + # Verify that the venv was created successfully by checking for Python executable + python_path = get_executable_path(venv_dir, "python") + if not python_path or not os.path.isfile(python_path): + sys.stderr.write( + "Error: Failed to create a proper virtual environment. " + "Missing the Python executable!\n" + ) + env.Exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
builder/frameworks/espidf.py(8 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:265-275
Timestamp: 2025-08-10T16:59:15.533Z
Learning: In the ESP32 platform builder's penv_setup.py, the esptool_repo_path used in subprocess calls comes from PlatformIO's internal package management system (platform.get_package_dir()) which is a trusted source. Command injection concerns are not applicable here since this is build infrastructure code with controlled inputs, not user-facing code accepting arbitrary input.
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:26-40
Timestamp: 2025-08-10T19:13:25.599Z
Learning: In the pioarduino/platform-espressif32 repository's builder/penv_setup.py file, semantic_version is imported but intentionally not listed in python_deps because it's a transitive dependency of platformio-core (version 2.10.*). When platformio is installed from the GitHub URL, semantic_version is automatically installed as one of its dependencies.
📚 Learning: 2025-08-10T19:13:25.599Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:26-40
Timestamp: 2025-08-10T19:13:25.599Z
Learning: In the pioarduino/platform-espressif32 repository's builder/penv_setup.py file, semantic_version is imported but intentionally not listed in python_deps because it's a transitive dependency of platformio-core (version 2.10.*). When platformio is installed from the GitHub URL, semantic_version is automatically installed as one of its dependencies.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-20T14:13:13.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
builder/frameworks/espidf.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build (macos-15, examples/espidf-peripherals-uart)
- GitHub Check: build (macos-15, examples/arduino-blink)
- GitHub Check: build (macos-15, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/arduino-usb-keyboard)
- GitHub Check: build (macos-15, examples/espidf-arduino-C6-ULP-blink)
- GitHub Check: build (macos-15, examples/espidf-coap-server)
- GitHub Check: build (windows-latest, examples/espidf-coap-server)
- GitHub Check: build (windows-latest, examples/espidf-http-request)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/espidf-arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (windows-latest, examples/tasmota)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/arduino-blink)
🔇 Additional comments (5)
builder/frameworks/espidf.py (5)
92-92: PLATFORMIO_DIR constant: LGTMCentralizing the core dir via env.subst("$PROJECT_CORE_DIR") improves readability and reuse downstream.
1575-1577: Venv location under PLATFORMIO_DIR/penv/.espidf-{version}: LGTMKeeps IDF envs co-located with PlatformIO penv. Matches prior conventions and avoids user-global pollution.
1503-1519: uv-based deps install: approach looks solid
- Single-shot install per batch is efficient.
- Constraints for urllib3/cryptography/pyparsing/idf-component-manager/esp-idf-kconfig are explicit and reproducible.
- Windows-specific windows-curses handled.
No blockers from me here.
Also applies to: 1550-1567
513-528: IDF_TOOLS_PATH sanitization aligns with prior preferenceUnsetting IDF_TOOLS_PATH in the IDF env avoids leakage of user overrides, matching the project preference captured in prior PRs.
361-374: Verified:builder/build_libdirectory exists; LIB_SOURCE resolution is correctConfirmed that
builder/build_lib/is present under the platform root, so the switch toplatform.get_dir()for locatingbuild_libis valid and robust against varying install layouts.env.Replace uses this path appropriately—no further changes needed.
Checklist:
Summary by CodeRabbit
Refactor
Bug Fixes
Chores