-
-
Notifications
You must be signed in to change notification settings - Fork 127
VS Code extension: fix CMake installation and Python venv setup on Windows #1948
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -4,6 +4,7 @@ const path = require('node:path') | |||||||||||||||||||||||||||||||||||||
const fs = require('fs-extra') | ||||||||||||||||||||||||||||||||||||||
const Mustache = require('mustache') | ||||||||||||||||||||||||||||||||||||||
const { simpleGit } = require('simple-git') | ||||||||||||||||||||||||||||||||||||||
const tools = require('./tools.js') | ||||||||||||||||||||||||||||||||||||||
const terminal = require('./terminal.js') | ||||||||||||||||||||||||||||||||||||||
const progressNotification = require('./progressnotification.js') | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
@@ -524,13 +525,17 @@ async function createGitRepository (fullPath, template, progressReporter) { | |||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
async function createPythonEnv (fullPath, name, packages, requirementsFiles) { | ||||||||||||||||||||||||||||||||||||||
// 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() | ||||||||||||||||||||||||||||||||||||||
const pipCommand = path.join( | ||||||||||||||||||||||||||||||||||||||
fullPath, | ||||||||||||||||||||||||||||||||||||||
name, | ||||||||||||||||||||||||||||||||||||||
(process.platform === 'win32') ? 'Scripts' : 'bin', | ||||||||||||||||||||||||||||||||||||||
'pip' | ||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
Comment on lines
+528
to
+535
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||||||||||
await terminal.run(pythonCommand, ['-m', 'venv', name], { | ||||||||||||||||||||||||||||||||||||||
cwd: fullPath | ||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||
const pipCommand = path.join(fullPath, name, 'bin', 'pip') | ||||||||||||||||||||||||||||||||||||||
if (packages && packages.length) { | ||||||||||||||||||||||||||||||||||||||
await terminal.run(pipCommand, ['install', ...packages], { | ||||||||||||||||||||||||||||||||||||||
cwd: fullPath | ||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,4 +1,4 @@ | ||||||||||||||||||||
'use strict' | ||||||||||||||||||||
Check notice on line 1 in tools/vscode-extension/tools.js
|
||||||||||||||||||||
|
||||||||||||||||||||
const vscode = require('vscode') | ||||||||||||||||||||
const util = require('node:util') | ||||||||||||||||||||
|
@@ -27,16 +27,20 @@ | |||||||||||||||||||
return tools[name].installed | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
async function checkCommands (commands, args) { | ||||||||||||||||||||
async function findCommand (commands, args) { | ||||||||||||||||||||
for (const command of commands) { | ||||||||||||||||||||
try { | ||||||||||||||||||||
await execFile(command, args) | ||||||||||||||||||||
} catch (error) { | ||||||||||||||||||||
continue | ||||||||||||||||||||
} | ||||||||||||||||||||
return true | ||||||||||||||||||||
return command | ||||||||||||||||||||
} | ||||||||||||||||||||
return false | ||||||||||||||||||||
return null | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
async function checkCommands (commands, args) { | ||||||||||||||||||||
return (await findCommand(commands, args)) !== null | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
let mipsInstalling = false | ||||||||||||||||||||
|
@@ -306,7 +310,7 @@ | |||||||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||||
requiresReboot = true | ||||||||||||||||||||
break | ||||||||||||||||||||
case 'linux': | ||||||||||||||||||||
|
@@ -457,56 +461,56 @@ | |||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
async function checkPython () { | ||||||||||||||||||||
async function findPython () { | ||||||||||||||||||||
switch (process.platform) { | ||||||||||||||||||||
case 'win32': | ||||||||||||||||||||
/* | ||||||||||||||||||||
* We cannot simply run 'python --version' here as Windows ships by | ||||||||||||||||||||
* default with fake (zero-byte) 'python' and 'python3' executables in its | ||||||||||||||||||||
* PATH. These files are actually links to UWP apps, implemented using a | ||||||||||||||||||||
* specific NTFS reparse tag. If Python is installed from the Microsoft | ||||||||||||||||||||
* Store they behave as if they were symlinks to the actual executables; | ||||||||||||||||||||
* if not, however, attempting to run them will result in the store | ||||||||||||||||||||
* popping up and prompting the user to install Python. | ||||||||||||||||||||
* | ||||||||||||||||||||
* A kludge is thus needed here in order to prevent this from happening. | ||||||||||||||||||||
* We'll first check for any UWP Python installations, then search PATH | ||||||||||||||||||||
* manually and skip the fake binaries if none was found. This ensures | ||||||||||||||||||||
* both UWP and non-UWP installs will be detected somewhat reliably. | ||||||||||||||||||||
* | ||||||||||||||||||||
* IMPORTANT: this assumes that the project's build system will also be | ||||||||||||||||||||
* able to detect and ignore the fake executables (rather than e.g. | ||||||||||||||||||||
* blindly executing 'python'). This is currently the case for the | ||||||||||||||||||||
* CMake-based templates. | ||||||||||||||||||||
*/ | ||||||||||||||||||||
let hasUWPPython | ||||||||||||||||||||
try { | ||||||||||||||||||||
const result = await execFile('powershell', [ | ||||||||||||||||||||
'-c', | ||||||||||||||||||||
'Get-AppxPackage -Name PythonSoftwareFoundation.Python.*' | ||||||||||||||||||||
]) | ||||||||||||||||||||
hasUWPPython = (result.stdout.trim() !== '') | ||||||||||||||||||||
} catch (error) { | ||||||||||||||||||||
hasUWPPython = false | ||||||||||||||||||||
} | ||||||||||||||||||||
for (const command of ['python3', 'python', 'py']) { | ||||||||||||||||||||
const matches = await which(command, { all: true }) | ||||||||||||||||||||
for (const fullPath of matches) { | ||||||||||||||||||||
const stats = await fs.stat(fullPath) | ||||||||||||||||||||
if (!stats.size && !hasUWPPython) { | ||||||||||||||||||||
continue | ||||||||||||||||||||
} | ||||||||||||||||||||
try { | ||||||||||||||||||||
await execFile(fullPath, ['--version']) | ||||||||||||||||||||
} catch (error) { | ||||||||||||||||||||
continue | ||||||||||||||||||||
} | ||||||||||||||||||||
return true | ||||||||||||||||||||
return fullPath | ||||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
return false | ||||||||||||||||||||
return null | ||||||||||||||||||||
default: | ||||||||||||||||||||
return checkCommands(['python3', 'python'], ['--version']) | ||||||||||||||||||||
return await findCommand(['python3', 'python'], ['--version']) | ||||||||||||||||||||
Check notice on line 513 in tools/vscode-extension/tools.js
|
||||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
|
@@ -606,7 +610,7 @@ | |||||||||||||||||||
'Python language runtime, required to run some project templates\' scripts', | ||||||||||||||||||||
homepage: 'https://python.org/', | ||||||||||||||||||||
install: installPython, | ||||||||||||||||||||
check: checkPython | ||||||||||||||||||||
check: async () => (await findPython()) !== null | ||||||||||||||||||||
}, | ||||||||||||||||||||
clangd: { | ||||||||||||||||||||
type: 'extension', | ||||||||||||||||||||
|
@@ -702,6 +706,8 @@ | |||||||||||||||||||
globalStorageUri = uri | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
exports.findPython = findPython | ||||||||||||||||||||
|
||||||||||||||||||||
exports.install = async (toInstall, force) => { | ||||||||||||||||||||
if (requiresReboot) { | ||||||||||||||||||||
return true | ||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot uses AI. Check for mistakes.