-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: symlink Python venvs in worktrees for instant setup #1832
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: develop
Are you sure you want to change the base?
Changes from 1 commit
a95a777
844841c
990df2b
fd09f87
b5208f0
cc66634
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 |
|---|---|---|
|
|
@@ -50,6 +50,10 @@ def debug_warning(*args, **kwargs): | |
|
|
||
| MODULE = "workspace.setup" | ||
|
|
||
| # Marker file written inside a recreated venv to indicate setup completed successfully. | ||
| # If the marker is absent, the venv is treated as incomplete and will be rebuilt. | ||
| VENV_SETUP_COMPLETE_MARKER = ".setup_complete" | ||
|
|
||
|
|
||
| def choose_workspace( | ||
| project_dir: Path, | ||
|
|
@@ -627,6 +631,39 @@ def setup_worktree_dependencies( | |
| performed = True | ||
| if config.strategy == DependencyStrategy.SYMLINK: | ||
| performed = _apply_symlink_strategy(project_dir, worktree_path, config) | ||
| # For venvs, verify the symlink is usable — fall back to recreate | ||
This comment was marked as outdated.
Sorry, something went wrong. |
||
| if performed and config.dep_type in ("venv", ".venv"): | ||
| venv_path = worktree_path / config.source_rel_path | ||
| if is_windows(): | ||
| python_bin = str(venv_path / "Scripts" / "python.exe") | ||
| else: | ||
| python_bin = str(venv_path / "bin" / "python") | ||
| try: | ||
| subprocess.run( | ||
| [python_bin, "-c", "import sys; print(sys.prefix)"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=10, | ||
| check=True, | ||
| ) | ||
|
||
| debug( | ||
| MODULE, | ||
| f"Symlinked venv health check passed: {config.source_rel_path}", | ||
| ) | ||
| except (subprocess.SubprocessError, OSError): | ||
| debug_warning( | ||
| MODULE, | ||
| f"Symlinked venv health check failed, falling back to recreate: {config.source_rel_path}", | ||
| ) | ||
| # Remove the broken symlink and recreate | ||
| symlink_path = worktree_path / config.source_rel_path | ||
| if symlink_path.is_symlink(): | ||
| symlink_path.unlink() | ||
| elif symlink_path.exists(): | ||
| shutil.rmtree(symlink_path, ignore_errors=True) | ||
| performed = _apply_recreate_strategy( | ||
| project_dir, worktree_path, config | ||
| ) | ||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| elif config.strategy == DependencyStrategy.RECREATE: | ||
| performed = _apply_recreate_strategy(project_dir, worktree_path, config) | ||
| elif config.strategy == DependencyStrategy.COPY: | ||
|
|
@@ -707,6 +744,40 @@ def _apply_symlink_strategy( | |
| return False | ||
|
|
||
|
|
||
| def _popen_with_cleanup( | ||
| cmd: list[str], | ||
| timeout: int, | ||
| label: str, | ||
| ) -> tuple[int, str, str]: | ||
| """Run a command via Popen with proper process cleanup on timeout. | ||
|
|
||
| On timeout: terminate → wait(10) → kill → wait(5) to ensure file locks | ||
| are released before any cleanup (e.g. shutil.rmtree). | ||
|
|
||
| Returns (returncode, stdout, stderr). | ||
| Raises subprocess.TimeoutExpired if the process could not be stopped. | ||
| """ | ||
| proc = subprocess.Popen( | ||
| cmd, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| text=True, | ||
| ) | ||
| try: | ||
| stdout, stderr = proc.communicate(timeout=timeout) | ||
| return proc.returncode, stdout, stderr | ||
| except subprocess.TimeoutExpired: | ||
| debug_warning(MODULE, f"{label} timed out, terminating process") | ||
| proc.terminate() | ||
| try: | ||
| proc.wait(timeout=10) | ||
| except subprocess.TimeoutExpired: | ||
| debug_warning(MODULE, f"{label} did not terminate, killing process") | ||
| proc.kill() | ||
| proc.wait(timeout=5) | ||
|
||
| raise | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| def _apply_recreate_strategy( | ||
| project_dir: Path, | ||
| worktree_path: Path, | ||
|
|
@@ -717,10 +788,18 @@ def _apply_recreate_strategy( | |
| Returns True if the venv was successfully created, False if skipped or failed. | ||
| """ | ||
| venv_path = worktree_path / config.source_rel_path | ||
| marker_path = venv_path / VENV_SETUP_COMPLETE_MARKER | ||
|
|
||
| if venv_path.exists(): | ||
| debug(MODULE, f"Skipping recreate {config.source_rel_path} - already exists") | ||
| return False | ||
| if marker_path.exists(): | ||
| debug( | ||
| MODULE, | ||
| f"Skipping recreate {config.source_rel_path} - already complete (marker present)", | ||
| ) | ||
| return False | ||
| # Venv exists but marker is missing — incomplete, remove and rebuild | ||
| debug(MODULE, f"Removing incomplete venv {config.source_rel_path} (no marker)") | ||
| shutil.rmtree(venv_path, ignore_errors=True) | ||
|
|
||
| # Detect Python executable from the source venv or fall back to sys.executable | ||
| source_venv = project_dir / config.source_rel_path | ||
|
|
@@ -737,29 +816,25 @@ def _apply_recreate_strategy( | |
| # Create the venv | ||
| try: | ||
| debug(MODULE, f"Creating venv at {venv_path}") | ||
| result = subprocess.run( | ||
| returncode, _, stderr = _popen_with_cleanup( | ||
| [python_exec, "-m", "venv", str(venv_path)], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=120, | ||
| label=f"venv creation ({config.source_rel_path})", | ||
| ) | ||
| if result.returncode != 0: | ||
| debug_warning(MODULE, f"venv creation failed: {result.stderr}") | ||
| if returncode != 0: | ||
| debug_warning(MODULE, f"venv creation failed: {stderr}") | ||
| print_status( | ||
| f"Warning: Could not create venv at {config.source_rel_path}", | ||
| "warning", | ||
| ) | ||
| # Clean up partial venv so retries aren't blocked | ||
| if venv_path.exists(): | ||
| shutil.rmtree(venv_path, ignore_errors=True) | ||
| return False | ||
|
Comment on lines
861
to
863
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. The logic to clean up a partially created virtual environment is repeated multiple times in this function (here and in the def _cleanup_failed_venv(venv_path: Path):
"""Safely removes a venv directory if it exists."""
if venv_path.exists():
shutil.rmtree(venv_path, ignore_errors=True)You could then call |
||
| except subprocess.TimeoutExpired: | ||
| debug_warning(MODULE, f"venv creation timed out for {config.source_rel_path}") | ||
| print_status( | ||
| f"Warning: venv creation timed out for {config.source_rel_path}", | ||
| "warning", | ||
| ) | ||
| # Clean up partial venv so retries aren't blocked | ||
| if venv_path.exists(): | ||
| shutil.rmtree(venv_path, ignore_errors=True) | ||
| return False | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
@@ -800,46 +875,43 @@ def _apply_recreate_strategy( | |
| if install_cmd: | ||
| try: | ||
| debug(MODULE, f"Installing deps from {req_file}") | ||
| pip_result = subprocess.run( | ||
| returncode, _, stderr = _popen_with_cleanup( | ||
| install_cmd, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=120, | ||
| timeout=300, | ||
| label=f"pip install ({req_file})", | ||
| ) | ||
| if pip_result.returncode != 0: | ||
| if returncode != 0: | ||
| debug_warning( | ||
| MODULE, | ||
| f"pip install failed (exit {pip_result.returncode}): " | ||
| f"{pip_result.stderr}", | ||
| f"pip install failed (exit {returncode}): {stderr}", | ||
| ) | ||
| print_status( | ||
| f"Warning: Dependency install failed for {req_file}", | ||
| "warning", | ||
| ) | ||
| # Clean up broken venv so retries aren't blocked | ||
| if venv_path.exists(): | ||
| shutil.rmtree(venv_path, ignore_errors=True) | ||
| return False | ||
| except subprocess.TimeoutExpired: | ||
| debug_warning( | ||
| MODULE, | ||
| f"pip install timed out for {req_file}", | ||
| ) | ||
| print_status( | ||
| f"Warning: Dependency install timed out for {req_file}", | ||
| "warning", | ||
| ) | ||
| # Clean up broken venv so retries aren't blocked | ||
| if venv_path.exists(): | ||
| shutil.rmtree(venv_path, ignore_errors=True) | ||
| return False | ||
| except OSError as e: | ||
| debug_warning(MODULE, f"pip install failed: {e}") | ||
| # Clean up broken venv so retries aren't blocked | ||
| if venv_path.exists(): | ||
| shutil.rmtree(venv_path, ignore_errors=True) | ||
| return False | ||
|
|
||
| # Write completion marker so future runs know this venv is complete | ||
| try: | ||
| marker_path.touch() | ||
| except OSError: | ||
| pass # Non-fatal — venv is still usable without the marker | ||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| debug(MODULE, f"Recreated venv at {config.source_rel_path}") | ||
| return True | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -250,11 +250,12 @@ interface DependencyConfig { | |||||||||
| const DEFAULT_STRATEGY_MAP: Record<string, 'symlink' | 'recreate' | 'copy' | 'skip'> = { | ||||||||||
| // JavaScript / Node.js — symlink is safe and fast | ||||||||||
| node_modules: 'symlink', | ||||||||||
| // Python — venvs MUST be recreated, not symlinked. | ||||||||||
| // CPython bug #106045: pyvenv.cfg discovery does not resolve symlinks, | ||||||||||
| // so a symlinked venv resolves paths relative to the target, not the worktree. | ||||||||||
| venv: 'recreate', | ||||||||||
| '.venv': 'recreate', | ||||||||||
| // Python — symlink for fast worktree creation. | ||||||||||
| // CPython bug #106045 (pyvenv.cfg symlink resolution) does not affect | ||||||||||
| // typical usage (running scripts, imports, pip). If the health check | ||||||||||
| // after symlinking fails, we fall back to recreate automatically. | ||||||||||
| venv: 'symlink', | ||||||||||
| '.venv': 'symlink', | ||||||||||
| // PHP — Composer vendor dir is safe to symlink | ||||||||||
| vendor_php: 'symlink', | ||||||||||
| // Ruby — Bundler vendor/bundle is safe to symlink | ||||||||||
|
|
@@ -364,6 +365,24 @@ async function setupWorktreeDependencies(projectPath: string, worktreePath: stri | |||||||||
| switch (config.strategy) { | ||||||||||
| case 'symlink': | ||||||||||
| performed = applySymlinkStrategy(projectPath, worktreePath, config); | ||||||||||
| // For venvs, verify the symlink is usable — fall back to recreate if not | ||||||||||
| if (performed && (config.depType === 'venv' || config.depType === '.venv')) { | ||||||||||
| const venvPath = path.join(worktreePath, config.sourceRelPath); | ||||||||||
| const pythonBin = isWindows() | ||||||||||
| ? path.join(venvPath, 'Scripts', 'python.exe') | ||||||||||
| : path.join(venvPath, 'bin', 'python'); | ||||||||||
| try { | ||||||||||
| await execFileAsync(pythonBin, ['-c', 'import sys; print(sys.prefix)'], { | ||||||||||
| timeout: 10000, | ||||||||||
| }); | ||||||||||
| debugLog('[TerminalWorktree] Symlinked venv health check passed:', config.sourceRelPath); | ||||||||||
| } catch { | ||||||||||
| debugLog('[TerminalWorktree] Symlinked venv health check failed, falling back to recreate:', config.sourceRelPath); | ||||||||||
|
||||||||||
| } catch { | |
| debugLog('[TerminalWorktree] Symlinked venv health check failed, falling back to recreate:', config.sourceRelPath); | |
| } catch (error) { | |
| debugError('[TerminalWorktree] Symlinked venv health check failed, falling back to recreate:', config.sourceRelPath, error); |
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.
🧹 Nitpick | 🔵 Trivial
Health check correctly decoupled from performed — minor redundancy on line 400.
The health check now runs whenever a venv symlink/path exists regardless of whether a fresh symlink was just created. This addresses the prior review feedback.
One nit: line 400 rebuilds the path path.join(worktreePath, config.sourceRelPath), which is identical to venvPath already declared on line 386.
Use the existing `venvPath` variable
- try { rmSync(path.join(worktreePath, config.sourceRelPath), { recursive: true, force: true }); } catch { /* best-effort */ }
+ try { rmSync(venvPath, { recursive: true, force: true }); } catch { /* best-effort */ }🤖 Prompt for AI Agents
In `@apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts` around
lines 383 - 404, The duplicated path join should use the already-declared
venvPath variable instead of recomputing path.join(worktreePath,
config.sourceRelPath); update the inner rmSync call to rmSync(venvPath, {
recursive: true, force: true }) so the removed target matches the health-check
path; keep the rest of the logic (execFileAsync check, debugLog messages, and
setting performed via applyRecreateStrategy) unchanged.
Uh oh!
There was an error while loading. Please reload this page.