simplify venv setup #246
simplify venv setup #246
Conversation
by using fixed path and addsitedir
|
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 WalkthroughThis change refactors the management of Python virtual environment and executable paths in Changes
Sequence Diagram(s)sequenceDiagram
participant MainScript
participant VirtualEnv
participant Subprocess
MainScript->>VirtualEnv: get_executable_path("python" or "pip" or "uv")
VirtualEnv-->>MainScript: Return full path to executable
MainScript->>VirtualEnv: setup_pipenv_in_package()
MainScript->>MainScript: site.addsitedir() for penv and site-packages
MainScript->>Subprocess: Call executable (pip/uv) using path from get_executable_path()
Subprocess-->>MainScript: Execution result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–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 comments)
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: 1
🔭 Outside diff range comments (1)
builder/main.py (1)
284-292: Update docstring to reflect function changes.The docstring states "Returns: str: Path to esptool executable", but the function now returns
None. This should be updated to reflect the actual behavior.def install_esptool(): """ Install esptool from package folder "tool-esptoolpy" using uv package manager. - Returns: - str: Path to esptool executable - Raises: SystemExit: If esptool installation fails """
🧹 Nitpick comments (2)
builder/main.py (2)
185-186: Misleading comments about environment usage.The comments state "Use modified environment with venv Python", but
os.environis the original process environment. The virtual environment Python is used becausePYTHON_EXEand tool paths have been updated to point to venv executables, not because the environment is modified.Consider updating the comments to be more accurate:
env=os.environ # Pass current environment (venv paths are used via executable paths)Also applies to: 219-220, 262-263
309-309: Remove trailing whitespace.Line 309 contains trailing whitespace that should be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
builder/main.py(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
builder/main.py (1)
builder/frameworks/espidf.py (1)
install_python_deps(1488-1552)
⏰ 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-peripherals-usb)
- GitHub Check: build (windows-latest, examples/espidf-exceptions)
- GitHub Check: build (macos-15, examples/arduino-zigbee-light)
- GitHub Check: build (macos-15, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/espidf-http-request)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-switch)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-light)
- GitHub Check: build (windows-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-lp)
- GitHub Check: build (ubuntu-latest, examples/espidf-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-C6-ULP-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/espidf-coap-server)
- GitHub Check: build (ubuntu-latest, examples/espidf-hello-world)
- GitHub Check: build (ubuntu-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (ubuntu-latest, examples/arduino-rmt-blink)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (8)
builder/main.py (8)
19-19: LGTM!The
sitemodule import is correctly added and necessary for the new path management approach usingsite.addsitedir().
67-81: Well-designed path helper function.The new
get_executable_path()function effectively consolidates the logic from the removed helper functions and correctly handles platform-specific differences for executable paths.
95-95: Correct usage of the new helper function.The assertion correctly uses
get_executable_path("pip")to verify the virtual environment was created properly.
99-105: Proper virtual environment activation.The code correctly sets up the virtual environment and updates both the SCons environment and global Python executable paths to use the venv Python.
133-137: Correct tool path initialization.The code properly initializes the Python paths and sets up the executable paths for esptool and uv using the new helper function.
324-327: Clear installation sequence.The comments helpfully clarify the installation order, ensuring dependencies are installed before esptool.
710-714: Good handling of paths with spaces.The code correctly wraps the esptool path in quotes when it contains spaces, preventing command-line execution issues.
1001-1001: Modern f-string usage.Good use of f-string for better readability.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
builder/main.py (1)
278-284: Update docstring to reflect function behavior.The docstring states "Returns: str: Path to esptool executable" but the function no longer returns anything.
def install_esptool(): """ Install esptool from package folder "tool-esptoolpy" using uv package manager. - Returns: - str: Path to esptool executable - Raises: SystemExit: If esptool installation fails """
♻️ Duplicate comments (1)
builder/main.py (1)
111-125: Excellent improvement using site.addsitedir().The refactored path setup properly addresses the previous concern about multiple Python versions by using
sys.version_infoto construct the directory name. The use ofsite.addsitedir()is a more robust approach than manual path manipulation as it handles .pth files and other site-specific configurations correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
builder/main.py(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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: in the esp32 platform builder, the subprocess call to get site-packages directory in setup_python_pa...
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.
Applied to files:
builder/main.py
🧬 Code Graph Analysis (1)
builder/main.py (1)
builder/frameworks/espidf.py (1)
install_python_deps(1488-1552)
⏰ 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 (windows-latest, examples/arduino-rmt-blink)
- GitHub Check: build (macos-15, examples/espidf-hello-world)
- GitHub Check: build (macos-15, examples/espidf-arduino-C6-ULP-blink)
- GitHub Check: build (windows-latest, examples/arduino-matter-light)
- GitHub Check: build (windows-latest, examples/espidf-coap-server)
- GitHub Check: build (windows-latest, examples/espidf-hello-world)
- GitHub Check: build (windows-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-light)
- GitHub Check: build (macos-15, examples/arduino-usb-keyboard)
- GitHub Check: build (macos-15, examples/espidf-arduino-matter-light)
- GitHub Check: build (macos-15, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/espidf-http-request)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-switch)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/espidf-exceptions)
- GitHub Check: build (windows-latest, examples/espidf-arduino-blink)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (7)
builder/main.py (7)
19-19: LGTM!The addition of the
sitemodule import is appropriate for the new path setup approach usingsite.addsitedir().
67-81: Clean consolidation of executable path logic.The new
get_executable_path()function effectively consolidates the previously scattered path construction logic into a single, well-documented function. The platform-specific handling is correct.
83-97: Good use of the new helper function.The refactoring to use
get_executable_path("pip")simplifies the code and maintains consistency across the codebase.
99-108: LGTM!The virtual environment setup and Python executable replacement logic is clear and properly sequenced.
127-132: Consistent executable path setup.Good use of the centralized
get_executable_path()function for all tool executables.
318-322: LGTM!The setup order is logical with helpful comments explaining the sequence.
704-708: Good handling of paths with spaces.The code properly quotes the esptool path when it contains spaces, which is important for cross-platform compatibility.
by using fixed path and addsitedir
Description:
Related issue (if applicable): fixes #
Checklist:
Summary by CodeRabbit