Use uv for creating penv when available #268
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces a uv-first virtualenv creation (derives OS-aware uv path near the Python executable, falls back to system uv or python -m venv), returns the used uv path or None, updates dependency installation to ensure uv in the penv (bootstraps if needed), removes uv from python_deps, and adds certifi and certifi-based env vars. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant penv as penv_setup.py
participant Python as python exec
participant UV_ext as external uv
participant FS as FileSystem
rect rgba(230,245,255,0.5)
note over penv: Create penv (uv-first)
Caller->>penv: setup_pipenv_in_package(python_exe, penv_dir)
penv->>FS: derive uv path next to python_exe (win=.exe)
alt local uv exists
penv->>UV_ext: uv venv --clear --python=<python_exe> <penv_dir>
UV_ext-->>penv: success
penv-->>Caller: return used_uv_path
else local uv missing or fails
penv->>Python: python -m venv --clear <penv_dir>
penv-->>Caller: return None
end
end
rect rgba(230,255,230,0.45)
note over penv: Install deps (ensure uv in penv)
Caller->>penv: install_python_deps(python_exe, external_uv_executable)
penv->>FS: locate penv uv executable
alt penv uv present
penv->>penv: _get_installed_uv_packages(penv_uv)
else penv uv absent
alt external_uv_executable provided
penv->>UV_ext: use external_uv to pip install "uv>=0.1.0" into penv
else
penv->>Python: python -m pip install "uv>=0.1.0" into penv
end
end
penv->>UV_ext: uv pip install <computed deps> (timeout 120s)
penv->>UV_ext: uv pip install esptool (timeout 60s)
penv-->>Caller: return True/False
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 4
🧹 Nitpick comments (3)
builder/penv_setup.py (3)
180-253: Consolidate duplicate error handling codeThe error handling for uv installation via external uv and pip contains duplicate code patterns. Consider extracting common error handling logic.
def _install_uv_with_command(cmd, timeout=120, error_prefix=""): """Helper function to install uv with proper error handling.""" try: result = subprocess.run( cmd, capture_output=True, text=True, timeout=timeout ) if result.returncode != 0: if result.stderr: print(f"Error output: {result.stderr.strip()}") return False return True except subprocess.TimeoutExpired: print(f"Error: {error_prefix}installation timed out") return False except FileNotFoundError: print(f"Error: {error_prefix}executable not found") return False except Exception as e: print(f"Error {error_prefix}installing uv: {e}") return FalseThen use it in both branches:
- try: - result = subprocess.run( - [external_uv_executable, "pip", "install", "uv>=0.1.0", f"--python={python_exe}", "--quiet"], - capture_output=True, - text=True, - timeout=120 - ) - if result.returncode != 0: - if result.stderr: - print(f"Error output: {result.stderr.strip()}") - return False - except subprocess.TimeoutExpired: - print("Error: uv installation timed out") - return False - except FileNotFoundError: - print("Error: External uv executable not found") - return False - except Exception as e: - print(f"Error installing uv package manager into penv: {e}") - return False + if not _install_uv_with_command( + [external_uv_executable, "pip", "install", "uv>=0.1.0", f"--python={python_exe}", "--quiet"], + error_prefix="External uv " + ): + return False
270-271: Increase timeout for uv pip list operationThe timeout of 120 seconds might be insufficient for slow systems or large environments. Consider making this configurable or increasing it.
- timeout=120 + timeout=int(os.environ.get('PIOARDUINO_UV_TIMEOUT', '180'))
319-319: Consider dynamic timeout based on package countInstalling multiple packages might require more than 120 seconds on slower systems. Consider adjusting the timeout based on the number of packages being installed.
- timeout=120 + timeout=max(120, 30 * len(packages_list))
📜 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/penv_setup.py(9 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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.
📚 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/penv_setup.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/penv_setup.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-coap-server)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-light)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/arduino-usb-keyboard)
- GitHub Check: build (windows-latest, examples/arduino-matter-light)
- GitHub Check: build (windows-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 (windows-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (ubuntu-latest, examples/espidf-hello-world)
- GitHub Check: build (ubuntu-latest, examples/espidf-http-request)
- GitHub Check: build (ubuntu-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-C6-ULP-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-littlefs)
🔇 Additional comments (3)
builder/penv_setup.py (3)
393-393: Good addition of timeout to esptool installationAdding a 60-second timeout to the esptool installation prevents hanging installations.
447-447: Verify parameter naming consistencyThe parameter name change from
uv_executabletoused_uv_executableimproves clarity by indicating it's the uv used to create the virtual environment.
256-295: LGTM: Well-structured helper functionThe
_get_installed_uv_packages()helper function is well-implemented with proper error handling for various failure scenarios.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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/penv_setup.py (1)
383-388: Fix: esptool install fails offline when uv isn’t present in the penvIf there’s no internet,
install_python_depsis skipped, sopenv/bin/uvmight not exist.install_esptoolthen attempts to use a non-existent penv uv and raises FileNotFoundError. Add a resilient fallback to pip in the penv, and optionally try the system uv first per maintainer preference to “trust the system”.- try: - subprocess.check_call([ - uv_executable, "pip", "install", "--quiet", "--force-reinstall", - f"--python={python_exe}", - "-e", esptool_repo_path - ], timeout=60) - - except subprocess.CalledProcessError as e: - sys.stderr.write( - f"Error: Failed to install esptool from {esptool_repo_path} (exit {e.returncode})\n" - ) - sys.exit(1) + try: + # Prefer penv uv; fall back to system 'uv' if penv uv is missing + uv_cmd = uv_executable if os.path.isfile(uv_executable) else "uv" + subprocess.check_call([ + uv_cmd, "pip", "install", "--quiet", "--force-reinstall", + f"--python={python_exe}", + "-e", esptool_repo_path + ], timeout=60) + except (FileNotFoundError, subprocess.CalledProcessError): + # Fallback to pip inside the penv (works offline with local package path) + try: + subprocess.check_call([ + python_exe, "-m", "pip", "install", "--quiet", "--force-reinstall", + "-e", esptool_repo_path + ], timeout=120) + except subprocess.CalledProcessError as e: + sys.stderr.write( + f"Error: Failed to install esptool from {esptool_repo_path} (exit {e.returncode})\n" + ) + sys.exit(1)
🧹 Nitpick comments (3)
builder/penv_setup.py (3)
83-107: uv-first venv creation: consider dropping the --clear flag and placing the path before optionsTwo nits to improve robustness:
- --clear is unnecessary because the code only runs when penv_dir doesn’t exist; it may also be unsupported by some uv versions.
- Most uv examples place the target directory before options, which improves CLI compatibility across versions.
Apply this diff:
- subprocess.check_call( - [uv_cmd, "venv", "--clear", f"--python={python_exe}", penv_dir], + subprocess.check_call( + [uv_cmd, "venv", penv_dir, f"--python={python_exe}"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, timeout=90 )Please verify locally that your uv version accepts the revised ordering (run
uv venv --help).
195-206: Slightly increase uv --version timeoutOn slow CI or cold caches, 10s can be tight. 30s is safer without impacting UX.
- timeout=10 + timeout=30
450-464: Propagate certifi vars to SCons env for subprocesses launched via env.ExecuteSetting os.environ is helpful; additionally propagate into SCons’ ENV so any future
env.Executecalls inherit the CA settings even if the OS env is modified elsewhere.def setup_certifi_env(): try: import certifi except ImportError: print("Info: certifi not available; skipping CA environment setup.") return cert_path = certifi.where() os.environ["CERTIFI_PATH"] = cert_path os.environ["SSL_CERT_FILE"] = cert_path os.environ["REQUESTS_CA_BUNDLE"] = cert_path os.environ["CURL_CA_BUNDLE"] = cert_path + # Also propagate to SCons environment for future env.Execute calls + env_vars = dict(env.get("ENV", {})) + env_vars.update({ + "CERTIFI_PATH": cert_path, + "SSL_CERT_FILE": cert_path, + "REQUESTS_CA_BUNDLE": cert_path, + "CURL_CA_BUNDLE": cert_path, + }) + env.Replace(ENV=env_vars)
📜 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/penv_setup.py(9 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.020Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.
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.
📚 Learning: 2025-08-26T14:51:33.727Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:43-43
Timestamp: 2025-08-26T14:51:33.727Z
Learning: The certifi Python package version 2025.8.3 is available on PyPI as of August 3, 2025, contrary to previous assumptions about it being a future version.
Applied to files:
builder/penv_setup.py
📚 Learning: 2025-08-26T14:52:29.020Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.020Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.
Applied to files:
builder/penv_setup.py
📚 Learning: 2025-08-26T14:53:03.841Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:192-194
Timestamp: 2025-08-26T14:53:03.841Z
Learning: Jason2866 prefers to trust the system for path handling in penv_setup.py and doesn't want additional path validation for security concerns around penv directory derivation, indicating confidence in the controlled environment and internal path generation.
Applied to files:
builder/penv_setup.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/penv_setup.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/penv_setup.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/penv_setup.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/penv_setup.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-arduino-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/espidf-peripherals-usb)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/espidf-exceptions)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-switch)
- GitHub Check: build (windows-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (windows-latest, examples/arduino-rmt-blink)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/espidf-exceptions)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/arduino-zigbee-light)
- GitHub Check: build (ubuntu-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-rmt-blink)
🔇 Additional comments (12)
builder/penv_setup.py (12)
43-43: Certifi requirement looks goodAcknowledging the maintainer note: certifi>=2025.8.3 is available. No action needed.
76-81: Good: documented uv-first behavior and return contractClear docstring on fallback and return value reduces ambiguity for callers.
110-118: Good: sane fallback to python -m venvThis ensures penv creation succeeds even without uv.
121-126: Good: post-creation verification and return of uv usageThe assert ensures correctness early; returning uv_cmd or None communicates the chosen path upstream.
190-196: penv uv path resolution is correct across OSesUsing get_executable_path keeps Windows vs POSIX differences contained.
207-251: Flow to ensure uv in penv is sound; verify the --python targeting flag with uv pipUsing external uv to bootstrap uv into the penv is a nice touch. Please confirm your uv version supports
uv pip install ... --python=<path>as used here; CLI flags have evolved over time.Would you like me to adjust the code to gracefully fall back to
"$PYTHONEXE" -m pipif the flag is not supported at runtime?
262-269: Approach for capturing installed packages via uv is solidJSON parsing with a generous timeout and defensive error handling is appropriate here.
306-317: Package installation command looks correct
- Handles URL and specifiers.
- Uses --upgrade and targets the penv Python explicitly.
319-331: Good: practical error handling and diagnosticsCoverage for CalledProcessError, timeouts, and FileNotFoundError is well thought out.
423-424: Nice: plumbs uv usage info upstreamCapturing the uv path used at creation time enables smarter decisions later.
441-444: Good: dependency install gated on connectivity or CIReasonable balance to avoid unnecessary errors for offline users while ensuring CI remains deterministic.
179-186: install_python_deps call sites are consistentAll invocations of the renamed install_python_deps in builder/penv_setup.py have been updated to the new signature, and the similarly named function in espidf.py is a separate, no-arg definition and remains unaffected.
• builder/penv_setup.py – call at line 441: install_python_deps(penv_python, used_uv_executable)
• builder/frameworks/espidf.py – local def install_python_deps() and its call at line 1650 remain unchangedNo further action required.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores