-
Notifications
You must be signed in to change notification settings - Fork 55
Add generic environment-creation utilities (PEP 723 PR 5a/16) #1651
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 |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import * as crypto from 'crypto'; | ||
| import * as fsapi from 'fs-extra'; | ||
| import * as path from 'path'; | ||
|
|
||
| /** Acquire an atomic lock released only explicitly; interrupted operations remain locked. */ | ||
| export async function acquireFileLock(filePath: string, options: AcquireFileLockOptions): Promise<AcquiredFileLock> { | ||
| const lockPath = `${path.resolve(filePath)}.lock`; | ||
| const ownerMarker = path.join(lockPath, `owner-${process.pid}-${crypto.randomBytes(16).toString('hex')}`); | ||
| const retainedMarker = path.join(lockPath, 'retained'); | ||
| const deadline = Date.now() + options.timeoutMs; | ||
|
|
||
| while (true) { | ||
| try { | ||
| await fsapi.mkdir(lockPath); | ||
| try { | ||
| await fsapi.writeFile(ownerMarker, '', { flag: 'wx' }); | ||
| } catch (error) { | ||
| try { | ||
| await fsapi.rmdir(lockPath); | ||
| } catch { | ||
| throw createLockError( | ||
| 'Lock initialization failed and left an owner-less lock directory', | ||
| 'ELOCKORPHANED', | ||
| lockPath, | ||
| ); | ||
| } | ||
| throw error; | ||
| } | ||
|
|
||
| let state: LockState = 'held'; | ||
| return { | ||
| retain: async () => { | ||
| if (state !== 'held') { | ||
| return; | ||
| } | ||
| state = 'retained'; | ||
| try { | ||
| await fsapi.writeFile(retainedMarker, '', { flag: 'wx' }); | ||
| } catch (error) { | ||
| if (isAlreadyExistsError(error)) { | ||
| return; | ||
| } | ||
| try { | ||
| await fsapi.rename(ownerMarker, retainedMarker); | ||
| } catch (renameError) { | ||
| if (!isAlreadyExistsError(renameError)) { | ||
| throw createLockError( | ||
| 'Failed to mark the lock as retained', | ||
| 'ERETAINFAILED', | ||
| lockPath, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| release: async () => { | ||
| if (state !== 'held') { | ||
| return; | ||
| } | ||
| state = 'released'; | ||
| try { | ||
| await fsapi.unlink(ownerMarker); | ||
| } catch (error) { | ||
| if (isFileNotFoundError(error)) { | ||
| throw createLockError('Lock ownership was compromised', 'ECOMPROMISED', lockPath); | ||
| } | ||
| throw error; | ||
| } | ||
| await fsapi.rmdir(lockPath); | ||
| }, | ||
| }; | ||
| } catch (error) { | ||
| if (!isAlreadyExistsError(error)) { | ||
| throw error; | ||
| } | ||
| if (await isRetainedLock(lockPath)) { | ||
| throw createLockError('Lock was retained after an interrupted operation', 'ELOCKRETAINED', lockPath); | ||
| } | ||
| if (Date.now() >= deadline) { | ||
| throw createLockError('Lock is already being held', 'ELOCKED', lockPath); | ||
| } | ||
| await delay(Math.min(options.retryIntervalMs, Math.max(0, deadline - Date.now()))); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function isAlreadyExistsError(error: unknown): boolean { | ||
| return hasErrorCode(error, 'EEXIST'); | ||
| } | ||
|
|
||
| function isFileNotFoundError(error: unknown): boolean { | ||
| return hasErrorCode(error, 'ENOENT'); | ||
| } | ||
|
|
||
|
Comment on lines
+90
to
+97
Contributor
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. If these are not used anywhere else, I would just add the
Contributor
Author
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. yeah sure it's only used within the file, I can make it inline. |
||
| async function isRetainedLock(lockPath: string): Promise<boolean> { | ||
| try { | ||
| await fsapi.lstat(path.join(lockPath, 'retained')); | ||
| return true; | ||
| } catch (error) { | ||
| if (isFileNotFoundError(error)) { | ||
| return false; | ||
| } | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| function hasErrorCode(error: unknown, code: string): boolean { | ||
| return ( | ||
| typeof error === 'object' && error !== null && 'code' in error && (error as NodeJS.ErrnoException).code === code | ||
| ); | ||
| } | ||
|
|
||
| function createLockError(message: string, code: string, lockPath: string): NodeJS.ErrnoException { | ||
| return Object.assign(new Error(message), { code, path: lockPath }); | ||
| } | ||
|
|
||
| async function delay(milliseconds: number): Promise<void> { | ||
| return new Promise((resolve) => setTimeout(resolve, milliseconds)); | ||
| } | ||
|
|
||
| export interface AcquireFileLockOptions { | ||
| readonly timeoutMs: number; | ||
| readonly retryIntervalMs: number; | ||
| } | ||
|
|
||
| export interface AcquiredFileLock { | ||
| readonly release: () => Promise<void>; | ||
| /** Keep the lock and make later acquisition attempts fail immediately. */ | ||
| readonly retain: () => Promise<void>; | ||
| } | ||
|
|
||
| type LockState = 'held' | 'released' | 'retained'; | ||
|
Comment on lines
+124
to
+135
Contributor
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. Maybe move interfaces and types to the top of the file
Contributor
Author
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. sure, will do that. |
||
|
Contributor
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. I feel we must have something like this somewhere in the codebase, though I could be wrong
Contributor
Author
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. There's one line code in src\managers\conda\condaUtils.ts that does something similar but it's not a helper, and the windows layouts are different, so I have this separate helper. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import * as path from 'path'; | ||
| import { isWindows } from './platformUtils'; | ||
|
|
||
| export function getVenvPythonPath(envPath: string): string { | ||
| return isWindows() | ||
| ? path.join(envPath, 'Scripts', 'python.exe') | ||
| : path.join(envPath, 'bin', 'python'); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,40 +66,14 @@ export async function runUV( | |
| token?: CancellationToken, | ||
| timeout?: number, | ||
| ): Promise<string> { | ||
| log?.info(`Running: uv ${args.join(' ')}`); | ||
| return new Promise<string>((resolve, reject) => { | ||
| const spawnOptions: { cwd?: string; timeout?: number } = { cwd }; | ||
| if (timeout !== undefined) { | ||
| spawnOptions.timeout = timeout; | ||
| } | ||
| const proc = spawnProcess('uv', args, spawnOptions); | ||
| token?.onCancellationRequested(() => { | ||
| proc.kill(); | ||
| reject(new CancellationError()); | ||
| }); | ||
|
|
||
| proc.on('error', (err) => { | ||
| log?.error(`Error spawning uv: ${err}`); | ||
| reject(new Error(`Error spawning uv: ${err.message}`)); | ||
| }); | ||
|
|
||
| let builder = ''; | ||
| proc.stdout?.on('data', (data) => { | ||
| const s = data.toString('utf-8'); | ||
| builder += s; | ||
| log?.append(s); | ||
| }); | ||
| proc.stderr?.on('data', (data) => { | ||
| log?.append(data.toString('utf-8')); | ||
| }); | ||
| proc.on('close', () => { | ||
| resolve(builder); | ||
| }); | ||
| proc.on('exit', (code) => { | ||
| if (code !== 0) { | ||
| reject(new Error(`Failed to run uv ${args.join(' ')}`)); | ||
| } | ||
| }); | ||
| return runProcess('uv', args, { | ||
| cwd, | ||
| displayName: 'uv', | ||
| log, | ||
| token, | ||
| timeout, | ||
| collectStderr: false, | ||
| logPrefix: '', | ||
|
Comment on lines
+69
to
+76
Contributor
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. This is also used by other places of the codebase. Is there a reason for this change and the removal of the listeners and exceptions?
Contributor
Author
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. It's just refactoring to get rid of the duplicate code. The error handling stuff is moved to the share runProcess. Maybe I shouldn't include refactor of a hot path in this pr, let me think about it. |
||
| }); | ||
| } | ||
|
|
||
|
|
@@ -111,37 +85,75 @@ export async function runPython( | |
| token?: CancellationToken, | ||
| timeout?: number, | ||
| ): Promise<string> { | ||
| log?.info(`Running: ${python} ${args.join(' ')}`); | ||
| return runProcess(python, args, { | ||
| cwd, | ||
| displayName: 'python', | ||
| log, | ||
| token, | ||
| timeout, | ||
| collectStderr: true, | ||
| logPrefix: 'python: ', | ||
| }); | ||
| } | ||
|
|
||
| function runProcess(executable: string, args: string[], options: RunProcessOptions): Promise<string> { | ||
| options.log?.info(`Running: ${executable} ${args.join(' ')}`); | ||
| return new Promise<string>((resolve, reject) => { | ||
| const proc = spawnProcess(python, args, { cwd: cwd, timeout }); | ||
| token?.onCancellationRequested(() => { | ||
| proc.kill(); | ||
| const spawnOptions: { cwd?: string; timeout?: number } = { cwd: options.cwd }; | ||
| if (options.timeout !== undefined) { | ||
| spawnOptions.timeout = options.timeout; | ||
| } | ||
| const proc = spawnProcess(executable, args, spawnOptions); | ||
| let cancellationRequested = false; | ||
| options.token?.onCancellationRequested(() => { | ||
| cancellationRequested = true; | ||
| try { | ||
| proc.kill(); | ||
| } catch { | ||
| // Preserve cancellation when signaling fails. | ||
| } | ||
| reject(new CancellationError()); | ||
| }); | ||
|
|
||
| proc.on('error', (err) => { | ||
| log?.error(`Error spawning python: ${err}`); | ||
| reject(new Error(`Error spawning python: ${err.message}`)); | ||
| if (cancellationRequested) { | ||
| reject(new CancellationError()); | ||
| return; | ||
| } | ||
| options.log?.error(`Error spawning ${options.displayName}: ${err}`); | ||
| reject(new Error(`Error spawning ${options.displayName}: ${err.message}`)); | ||
| }); | ||
|
|
||
| let builder = ''; | ||
| proc.stdout?.on('data', (data) => { | ||
| const s = data.toString('utf-8'); | ||
| builder += s; | ||
| log?.append(`python: ${s}`); | ||
| options.log?.append(`${options.logPrefix}${s}`); | ||
| }); | ||
| proc.stderr?.on('data', (data) => { | ||
| const s = data.toString('utf-8'); | ||
| builder += s; | ||
| log?.append(`python: ${s}`); | ||
| if (options.collectStderr) { | ||
| builder += s; | ||
| } | ||
| options.log?.append(`${options.logPrefix}${s}`); | ||
| }); | ||
| proc.on('close', () => { | ||
| resolve(builder); | ||
| }); | ||
| proc.on('exit', (code) => { | ||
| if (code !== 0) { | ||
| reject(new Error(`Failed to run python ${args.join(' ')}`)); | ||
| reject(new Error(`Failed to run ${options.displayName} ${args.join(' ')}`)); | ||
| } | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| interface RunProcessOptions { | ||
| readonly cwd?: string; | ||
| readonly displayName: 'python' | 'uv'; | ||
| readonly log?: LogOutputChannel; | ||
| readonly token?: CancellationToken; | ||
| readonly timeout?: number; | ||
| readonly collectStderr: boolean; | ||
| readonly logPrefix: string; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,16 @@ | ||
| import * as fsapi from 'fs-extra'; | ||
| import * as os from 'os'; | ||
| import * as path from 'path'; | ||
| import { l10n, LogOutputChannel, ProgressLocation, QuickPickItem, QuickPickItemKind, ThemeIcon, Uri } from 'vscode'; | ||
| import { | ||
| CancellationError, | ||
| l10n, | ||
| LogOutputChannel, | ||
| ProgressLocation, | ||
| QuickPickItem, | ||
| QuickPickItemKind, | ||
| ThemeIcon, | ||
| Uri, | ||
| } from 'vscode'; | ||
| import { EnvironmentManager, PythonEnvironment, PythonEnvironmentApi, PythonEnvironmentInfo } from '../../api'; | ||
| import { ENVS_EXTENSION_ID } from '../../common/constants'; | ||
| import { Common, VenvManagerStrings } from '../../common/localize'; | ||
|
|
@@ -10,6 +19,7 @@ import { getWorkspacePersistentState } from '../../common/persistentState'; | |
| import { EventNames } from '../../common/telemetry/constants'; | ||
| import { sendTelemetryEvent } from '../../common/telemetry/sender'; | ||
| import { normalizePath } from '../../common/utils/pathUtils'; | ||
| import { getVenvPythonPath } from '../../common/utils/virtualEnvironment'; | ||
| import { | ||
| showErrorMessage, | ||
| showOpenDialog, | ||
|
|
@@ -52,6 +62,14 @@ export interface CreateEnvironmentResult { | |
| * Exists if error occurred while installing packages and includes error description. | ||
| */ | ||
| pkgInstallationErr?: string; | ||
|
|
||
| /** Cancellation may leave package processes running. */ | ||
| pkgInstallationCancelled?: boolean; | ||
| } | ||
|
|
||
| export interface CreateWithProgressOptions { | ||
| /** Whether to record uv-created environments in workspace state. Defaults to true. */ | ||
| readonly trackUvEnvironment?: boolean; | ||
| } | ||
|
|
||
| export async function clearVenvCache(): Promise<void> { | ||
|
|
@@ -340,9 +358,9 @@ export async function createWithProgress( | |
| venvRoot: Uri, | ||
| envPath: string, | ||
| packages?: PipPackages, | ||
| options?: CreateWithProgressOptions, | ||
|
Contributor
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. Why do we need this to be another object instead of just passing a
Contributor
Author
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. It's mainly because so that the caller is more explicit about what's passed to the function and also it's open to extend to other options in the future. I could also make it a boolean value and add comment clarifying what that is in the caller. |
||
| ): Promise<CreateEnvironmentResult | undefined> { | ||
| const pythonPath = | ||
| os.platform() === 'win32' ? path.join(envPath, 'Scripts', 'python.exe') : path.join(envPath, 'bin', 'python'); | ||
| const pythonPath = getVenvPythonPath(envPath); | ||
|
|
||
| return await withProgress( | ||
| { | ||
|
|
@@ -383,6 +401,7 @@ export async function createWithProgress( | |
| const env = api.createPythonEnvironmentItem(await getPythonInfo(resolved), manager); | ||
|
|
||
| if ( | ||
| options?.trackUvEnvironment !== false && | ||
| useUv && | ||
| (resolved.kind === NativePythonEnvironmentKind.venvUv || | ||
| resolved.kind === NativePythonEnvironmentKind.uvWorkspace) | ||
|
|
@@ -401,6 +420,7 @@ export async function createWithProgress( | |
| } catch (e) { | ||
| // error occurred while installing packages | ||
| result.pkgInstallationErr = e instanceof Error ? e.message : String(e); | ||
| result.pkgInstallationCancelled = e instanceof CancellationError; | ||
| } | ||
| } | ||
| result.environment = env; | ||
|
|
||
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.
Just a question here: why do we need the while loop? Not saying it is wrong, just curious in case I am not understanding correctly
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.
The loop prevents two VS Code processes from creating the same environment simultaneously.
For example:
The while(true) does not run forever. It either gets the lock, detects a retained lock, or reaches the timeout. Without the loop, Process B would fail immediately merely because Process A was still working.