VS Code extension: fix CMake installation and Python venv setup on Windows #1948
Conversation
WalkthroughThe changes update the VSCode extension version, refactor command and Python executable detection logic, and adjust how Python environments are created. The new logic finds the actual Python executable path rather than relying on platform-specific command strings, and updates the CMake installer invocation method on Windows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Extension
participant Tools Module
User->>Extension: Initiate Python environment creation
Extension->>Tools Module: Call findPython()
Tools Module-->>Extension: Return Python executable path or null
Extension->>Extension: Compute pip path based on Python executable
Extension->>Extension: Create virtual environment
Extension->>Extension: Install packages with pip
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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
Documentation and Community
|
![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
🧹 Nitpick comments (1)
tools/vscode-extension/tools.js (1)
464-515: Excellent Windows Python detection logic with minor improvement needed.The comprehensive approach to handling Windows' fake Python executables is well-implemented and documented. The UWP detection and path filtering logic properly addresses the Microsoft Store redirect issue.
Consider adding error handling around
fs.stat()call:for (const fullPath of matches) { - const stats = await fs.stat(fullPath) + let stats + try { + stats = await fs.stat(fullPath) + } catch (error) { + continue + } if (!stats.size && !hasUWPPython) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tools/vscode-extension/package.json(1 hunks)tools/vscode-extension/templates.js(2 hunks)tools/vscode-extension/tools.js(6 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
tools/vscode-extension/tools.js
[notice] 464-513: ✅ No longer an issue: Deep, Nested Complexity
checkPython is no longer above the threshold for nested complexity depth. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
[notice] 464-513: ✅ No longer an issue: Deep, Nested Complexity
checkPython is no longer above the threshold for nested complexity depth. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
⏰ 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). (11)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-openbios
- GitHub Check: aur-build
- GitHub Check: cross-arm64
- GitHub Check: coverage
- GitHub Check: build
- GitHub Check: asan
- GitHub Check: toolchain
- GitHub Check: macos-build-and-test-toolchain
🔇 Additional comments (15)
tools/vscode-extension/tools.js (10)
30-44: LGTM! Clean refactoring improves code modularity.The separation of
checkCommandsintofindCommandandcheckCommandsis well-designed.findCommandreturns the actual command that works (or null), whilecheckCommandsmaintains the boolean interface for backward compatibility.
313-313: Improved CMake installer invocation on Windows.Using
msiexec /idirectly is more reliable than usingstartwith the MSI file, as it ensures proper MSI execution and better error handling.
464-515: Excellent Windows Python detection logic with comprehensive UWP handling.The implementation correctly addresses the Windows-specific issue with fake Python executables that redirect to the Microsoft Store. The approach is well-documented and handles both UWP and traditional Python installations.
Key strengths:
- Checks for UWP Python installations first using PowerShell
- Uses
whichto find all executable paths, not just the first one- Validates file size to detect fake executables when UWP Python isn't present
- Tests actual execution with
--versionto ensure the executable works- Falls back to the simpler
findCommandapproach on non-Windows platforms
613-613: Proper integration with new findPython function.The Python tool's check function correctly uses the new async
findPython()and converts the result to a boolean by checking for null.
709-709: Necessary export for cross-module usage.Exporting
findPythonallows templates.js to use the enhanced Python detection logic.
30-40: LGTM! Clean refactor separating command finding from boolean checking.The new
findCommandfunction properly extracts the logic for finding the first working command from a list, returning the command string or null. This separation of concerns improves code reusability.
42-44: LGTM! Maintains backward compatibility while leveraging the refactor.The updated
checkCommandsfunction correctly wrapsfindCommandto preserve the boolean return interface expected by existing callers.
313-313: LGTM! Proper fix for Windows MSI installation.Using
msiexec /iinstead ofstartis the correct way to programmatically install MSI packages on Windows. This ensures the installer runs properly rather than just opening the file.
613-613: LGTM! Proper integration with the refactored findPython function.The check property correctly adapts to use the new
findPythonfunction, converting its path return value to the expected boolean result.
709-709: LGTM! Necessary export for cross-module integration.Exporting
findPythonallows other modules liketemplates.jsto use the enhanced Python detection logic.tools/vscode-extension/templates.js (3)
7-7: Proper import for enhanced Python detection.Adding the tools import enables access to the improved
findPythonfunction.
528-535: Excellent improvement to Python environment creation.The changes significantly improve Windows compatibility:
- Dynamic Python detection: Uses
await tools.findPython()instead of hardcoded platform-specific commands- Cross-platform pip path: Correctly handles the difference between Windows (
Scripts) and Unix-like systems (bin) for the pip executable location- Better integration: Leverages the robust Python detection logic from tools.js
This addresses the core issue mentioned in the PR objectives where
createPythonEnv()was broken on Windows.
7-7: LGTM! Required import for the enhanced Python detection.The import of the
toolsmodule provides access to thefindPythonfunction needed bycreatePythonEnv.tools/vscode-extension/package.json (2)
5-5: Appropriate version bump for bug fixes and improvements.The version increment from 0.3.9 to 0.3.10 correctly reflects the Windows compatibility fixes and improvements made to the Python environment setup and CMake installation.
5-5: LGTM! Appropriate patch version bump for bug fixes.The version increment from 0.3.9 to 0.3.10 correctly follows semantic versioning for the Windows compatibility fixes in this PR.
| const pythonCommand = await tools.findPython() | ||
| const pipCommand = path.join( | ||
| fullPath, | ||
| name, | ||
| (process.platform === 'win32') ? 'Scripts' : 'bin', | ||
| 'pip' | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Excellent improvement with missing error handling.
The changes properly use the enhanced Python detection and correctly handle platform-specific virtual environment structure. This addresses the core Windows compatibility issue mentioned in the PR.
Add error handling for when Python isn't found:
async function createPythonEnv (fullPath, name, packages, requirementsFiles) {
const pythonCommand = await tools.findPython()
+ if (!pythonCommand) {
+ throw new Error('Python executable not found. Please install Python first.')
+ }
const pipCommand = path.join(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pythonCommand = await tools.findPython() | |
| const pipCommand = path.join( | |
| fullPath, | |
| name, | |
| (process.platform === 'win32') ? 'Scripts' : 'bin', | |
| 'pip' | |
| ) | |
| async function createPythonEnv (fullPath, name, packages, requirementsFiles) { | |
| const pythonCommand = await tools.findPython() | |
| + if (!pythonCommand) { | |
| + throw new Error('Python executable not found. Please install Python first.') | |
| + } | |
| const pipCommand = path.join( | |
| fullPath, | |
| name, | |
| (process.platform === 'win32') ? 'Scripts' : 'bin', | |
| 'pip' | |
| ) |
🤖 Prompt for AI Agents
In tools/vscode-extension/templates.js around lines 528 to 535, add error
handling for the case when the Python executable is not found by
tools.findPython(). Check if pythonCommand is undefined or null after the await
call, and if so, throw an error or return early with a clear message indicating
that Python was not found. This will prevent subsequent code from failing
silently or with unclear errors.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes critical Windows compatibility issues in the PSX.Dev VS Code extension by addressing Python executable detection and virtual environment setup that were broken due to Windows-specific behavior, along with fixing CMake installation.
- Refactored Python detection to avoid Windows Store redirects and properly locate real Python executables
- Fixed virtual environment pip path resolution to account for Windows using 'Scripts' directory instead of 'bin'
- Updated CMake installation to use msiexec instead of start command for proper Windows installation
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/vscode-extension/tools.js | Refactored command checking functions and fixed Python detection logic for Windows compatibility |
| tools/vscode-extension/templates.js | Updated Python virtual environment creation to use proper directory structure and Python executable detection |
| tools/vscode-extension/package.json | Bumped version number to 0.3.10 |
| @@ -306,7 +310,7 @@ async function installCMake () { | |||
| asset.browser_download_url.split('/').pop() | |||
| ) | |||
| await downloader.downloadFile(asset.browser_download_url, filename) | |||
| await execFile('start', [filename]) | |||
| await execFile('msiexec', ['/i', filename]) | |||
There was a problem hiding this comment.
The msiexec call lacks error handling and user interaction considerations. MSI installations typically require user interaction or administrative privileges. Consider adding the '/quiet' flag for silent installation or '/passive' for progress-only UI, and handle potential elevation requirements.
| await execFile('msiexec', ['/i', filename]) | |
| try { | |
| await execFile('msiexec', ['/i', filename, '/passive']); | |
| } catch (error) { | |
| vscode.window.showErrorMessage( | |
| 'Failed to install CMake. Please ensure you have administrative privileges and try again.' | |
| ); | |
| throw error; | |
| } |
Copilot uses AI. Check for mistakes.
| // On Windows "python" and "python3" are aliased to a script that opens the | ||
| // Microsoft Store by default, so the "py" launcher is invoked instead. | ||
| const pythonCommand = (process.platform === 'win32') ? 'py' : 'python3' | ||
| const pythonCommand = await tools.findPython() |
There was a problem hiding this comment.
If findPython() returns null (no Python found), this will cause pythonCommand to be null and the subsequent terminal.run() call will fail with an unclear error. Add validation to check if pythonCommand is null and provide a helpful error message.
| const pythonCommand = await tools.findPython() | |
| const pythonCommand = await tools.findPython() | |
| if (!pythonCommand) { | |
| throw new Error('Python executable not found. Please ensure Python is installed and available in your PATH.'); | |
| } |
Copilot uses AI. Check for mistakes.
|
I haven't had time to try this yet. |
Fix for the
createPythonEnv()function in the PSX.Dev VS Code extension, which is currently broken on Windows due to it not taking into account the "fake" Python executables that redirect to the Microsoft Store as well as virtual environments having a slightly different file structure depending on the platform. I have also fixedinstallCMake()and bumped the extension's version number to 0.3.10.I haven't yet tested these changes on a Windows machine so please review before merging.