force explicit install of venv in .platformio folder
#241
Conversation
WalkthroughThis update introduces a dedicated Python virtual environment ("penv") within the PlatformIO core directory to manage dependencies. It adds functions to set up and use this environment, updates global Python executable handling, modifies Windows path logic, and adjusts Python path setup to ensure correct interpreter and package resolution. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainScript
participant VirtualEnv
participant SystemEnv
User->>MainScript: Start build process
MainScript->>VirtualEnv: setup_pipenv_in_package()
VirtualEnv-->>MainScript: Ensure "penv" exists, pip available
MainScript->>SystemEnv: Update PATH and sys.path
MainScript->>MainScript: Update PYTHON_EXE to penv Python
MainScript->>MainScript: setup_python_paths()
MainScript->>MainScript: Install Python dependencies (with penv Python)
MainScript->>MainScript: Install esptool (with penv Python)
MainScript-->>User: Continue build process
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: 0
🧹 Nitpick comments (2)
builder/main.py (2)
69-89: Consider adding more robust error handling for venv creation.While the assertion checks for pip existence, the venv creation itself could fail for various reasons (permissions, disk space, corrupted Python installation). Consider wrapping the venv creation in a try-except block to provide more informative error messages.
def setup_pipenv_in_package(): """ Checks if 'penv' folder exists in platformio dir and creates virtual environment if not. """ if not os.path.exists(penv_dir): - env.Execute( - env.VerboseAction( - '"$PYTHONEXE" -m venv --clear "%s"' % penv_dir, - "Creating a new virtual environment for Python dependencies", + try: + env.Execute( + env.VerboseAction( + '"$PYTHONEXE" -m venv --clear "%s"' % penv_dir, + "Creating a new virtual environment for Python dependencies", + ) ) - ) + except Exception as e: + sys.stderr.write(f"Error: Failed to create virtual environment at {penv_dir}: {e}\n") + env.Exit(1) assert os.path.isfile( pip_path ), "Error: Failed to create a proper virtual environment. Missing the `pip` binary!"
103-121: Consider using the Python interpreter to determine site-packages path.While the current directory scanning approach works, it could be more robust to ask the Python interpreter directly for its site-packages location, similar to what's done in
setup_python_paths().- if IS_WINDOWS: - penv_site_packages = os.path.join(penv_dir, "Lib", "site-packages") - else: - # Find the actual site-packages directory in the venv - penv_lib_dir = os.path.join(penv_dir, "lib") - if os.path.isdir(penv_lib_dir): - for python_dir in os.listdir(penv_lib_dir): - if python_dir.startswith("python"): - penv_site_packages = os.path.join(penv_lib_dir, python_dir, "site-packages") - break - else: - penv_site_packages = None - else: - penv_site_packages = None + # Get site-packages directly from the Python interpreter + try: + result = subprocess.run( + [PYTHON_EXE, "-c", "import site; print(site.getsitepackages()[0])"], + capture_output=True, + text=True, + timeout=5 + ) + if result.returncode == 0: + penv_site_packages = result.stdout.strip() + else: + penv_site_packages = None + except Exception: + penv_site_packages = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
builder/main.py(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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
📚 Learning: in platform.py, the `tool-esp_install` package intentionally transitions from mandatory to optional ...
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#237
File: platform.py:200-201
Timestamp: 2025-07-29T11:35:26.182Z
Learning: In platform.py, the `tool-esp_install` package intentionally transitions from mandatory to optional status after successful installation. Initially it's needed and mandatory for installation, but becomes optional once installed since it's no longer required for subsequent operations. Setting `self.packages[tl_install_name]["optional"] = True` after version verification is the intended design pattern.
Applied to files:
builder/main.py
🔇 Additional comments (8)
builder/main.py (8)
37-37: Good use of centralized platform detection.Using
IS_WINDOWSfromplatformio.compatinstead of direct platform checks improves maintainability and consistency.
60-68: Proper cross-platform path handling for virtual environment.The code correctly constructs platform-specific paths for the virtual environment and pip executable.
94-99: Correct PATH manipulation for virtual environment.The code properly updates the PATH environment variable to ensure subprocess calls use the virtual environment's Python interpreter, with appropriate duplicate checking.
100-102: Good verification of Python executable existence.The assertion with a clear error message helps catch configuration issues early.
149-149: Minor formatting cleanup.
286-286: Consistent use of IS_WINDOWS constant.
424-428: Clear installation sequence with helpful comments.The installation order (dependencies first, then esptool) is logical and the comments improve code clarity.
183-184: Ignore Scripts Directory ConcernThe updated
_get_executable_pathno longer hard-codes “Scripts” for Windows; instead it usespython_dir = os.path.dirname(python_exe)Since in a standard Windows venv
python_exelives under theScriptsfolder, executables are still found there. This dynamic lookup is actually more robust and does not break existing environments. No changes are needed.Likely an incorrect or invalid review comment.
Description:
to avoid pioarduino tools install issues (especially with Windows)
Checklist:
Summary by CodeRabbit
New Features
Improvements
Other