Add generic environment-creation utilities (PEP 723 PR 5a/16)#1651
Add generic environment-creation utilities (PEP 723 PR 5a/16)#1651StellaHuang95 wants to merge 1 commit into
Conversation
b1e9b95 to
f005e16
Compare
f005e16 to
d36561b
Compare
There was a problem hiding this comment.
I feel we must have something like this somewhere in the codebase, though I could be wrong
There was a problem hiding this comment.
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.
| return runProcess('uv', args, { | ||
| cwd, | ||
| displayName: 'uv', | ||
| log, | ||
| token, | ||
| timeout, | ||
| collectStderr: false, | ||
| logPrefix: '', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Cross-process file lock, venv Python-path helper, cancellation-safe process runner, and createWithProgress tracking options that inline-script environment creation builds on. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39dcc6a3-0fbd-4f36-9d0f-68677de49c27
| function isAlreadyExistsError(error: unknown): boolean { | ||
| return hasErrorCode(error, 'EEXIST'); | ||
| } | ||
|
|
||
| function isFileNotFoundError(error: unknown): boolean { | ||
| return hasErrorCode(error, 'ENOENT'); | ||
| } | ||
|
|
There was a problem hiding this comment.
If these are not used anywhere else, I would just add the hasErrorCode(error, 'XYZ') inline
There was a problem hiding this comment.
yeah sure it's only used within the file, I can make it inline.
| 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'; |
There was a problem hiding this comment.
Maybe move interfaces and types to the top of the file
There was a problem hiding this comment.
sure, will do that.
| const retainedMarker = path.join(lockPath, 'retained'); | ||
| const deadline = Date.now() + options.timeoutMs; | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
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.
The loop prevents two VS Code processes from creating the same environment simultaneously.
For example:
- Process A creates the lock and starts building the environment.
- Process B tries to create the same lock but cannot because A owns it.
- B waits briefly, then the while (true) loop tries again.
- When A finishes, it removes the lock.
- B’s next attempt succeeds. B can then reuse the environment A created.
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.
| venvRoot: Uri, | ||
| envPath: string, | ||
| packages?: PipPackages, | ||
| options?: CreateWithProgressOptions, |
There was a problem hiding this comment.
Why do we need this to be another object instead of just passing a trackUvEnvironment property?
There was a problem hiding this comment.
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.
d36561b to
104166d
Compare
Roadmap context
This is the first slice of PR 5 of 16 in the PEP 723 inline-script roadmap. The full plan lives in #1602.
meta.jsonsidecarrequires-pythonto interpreter selectionInlineScriptEnvManagerskeletoncreate()happy path (manager + wiring)create()uv-install fallbackget,set, and MementoWhy this PR
PR 5c implements
InlineScriptEnvManager.create(). Before touching the manager, this PR lands the generic, reusable primitives it relies on — a cross-process file lock, a venv Python-path helper, a cancellation-hardened process runner, and two smallcreateWithProgressoptions. None of this code is inline-script-specific, so it is reviewed on its own.What this PR adds
Cross-process file lock (
src/common/lockfile.apis.ts, new):acquireFileLockuses an atomicmkdirof a<path>.lockdirectory plus a per-owner marker file, returningAcquiredFileLock { release, retain }.retain()writes aretainedmarker so a later acquirer fails fast withELOCKRETAINEDinstead of waiting out the 5-minute timeout — used when a build is cancelled mid-flight. Distinct error codes (ELOCKED,ELOCKRETAINED,ELOCKORPHANED,ECOMPROMISED,ERETAINFAILED) separate contention from corruption.Shared
getVenvPythonPath(src/common/utils/virtualEnvironment.ts, new): returnsScripts\python.exeon Windows, elsebin/python. Replaces an inline copy invenvUtilsand is reused by 5b/5c.Hardened process helper (
src/managers/builtin/helpers.ts):runUVandrunPythonnow share onerunProcessimplementation whose cancellation guardskill()intry/catchand still emits a cleanCancellationErrorif the process errors after a cancel. Per-caller options preserve existing behavior (collectStderr,logPrefix).venvUtils.ts:createWithProgressgainsCreateWithProgressOptions { trackUvEnvironment }, andCreateEnvironmentResultgainspkgInstallationCancelledso a caller can tell cancellation apart from a real install failure. Existing callers are unaffected (both are optional / additive).Tests
lockfile.apis.unit.test.ts— 9 tests: contention, retain/fail-fast, orphaned and compromised locks, and timeout.virtualEnvironment.unit.test.ts— 2 tests forgetVenvPythonPathon Windows and POSIX.helpers.cancellation.unit.test.ts— 4 tests forrunProcesscancellation safety.venvUtils.createWithProgress.unit.test.ts— 3 tests fortrackUvEnvironmentandpkgInstallationCancelled.On this branch alone
npm run compile-testsis clean andnpm run unittestreports 1447 passing, 0 failing, 4 pending.User impact
None. These are internal primitives with no new user-visible behavior. The refactors to
helpers.tsandvenvUtils.tsare behavior-preserving for existing callers.