fix(scripts): restore missing scripts/env/select_venv.py (#41)#50
Open
Uptrend-john wants to merge 1 commit into
Open
fix(scripts): restore missing scripts/env/select_venv.py (#41)#50Uptrend-john wants to merge 1 commit into
Uptrend-john wants to merge 1 commit into
Conversation
setup-windows.bat and setup-unix.sh both invoke scripts/env/select_venv.py to resolve the per-platform venv path, but the file was missing from the repo. Setup failed in step 2/4 with "No such file or directory" and could not create the venv. The script's intended contract (inferred from how the setup scripts call it): - --print resolves the platform-appropriate venv path (.venv.win / .venv.mac / .venv.nix at repo root) and prints it if it exists. - --ensure creates the venv at that path if missing. - --ensure --print does both. Also adds a !scripts/env/ negation to .gitignore so the source directory is not masked by the broader ENV/ venv-ignore rule on case-insensitive filesystems (Windows, default macOS).
📝 WalkthroughWalkthroughThis pull request adds a cross-platform virtual environment selection script ( ChangesVirtual Environment Platform Management
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/env/select_venv.py`:
- Around line 20-33: venv_dir_name() currently maps non-win/non-mac to
".venv.nix" and misses WSL plus legacy ".venv" fallback; update venv_dir_name()
so it first detects WSL (e.g., sys.platform == "linux" combined with presence of
environment variable "WSL_DISTRO_NAME" or "/proc/version" containing
"microsoft"/"Microsoft") and return ".venv.wsl" for WSL, then check for an
existing legacy repo_root()/".venv" and return ".venv" if that path exists,
otherwise fall back to ".venv.nix"; keep repo_root() and venv_path() usage
intact so venv_path() uses the updated venv_dir_name().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f819abd4-5737-41ef-abf5-77eafa7a1fe3
📒 Files selected for processing (2)
.gitignorescripts/env/select_venv.py
Comment on lines
+20
to
+33
| def venv_dir_name() -> str: | ||
| if sys.platform.startswith("win"): | ||
| return ".venv.win" | ||
| if sys.platform == "darwin": | ||
| return ".venv.mac" | ||
| return ".venv.nix" | ||
|
|
||
|
|
||
| def repo_root() -> Path: | ||
| return Path(__file__).resolve().parent.parent.parent | ||
|
|
||
|
|
||
| def venv_path() -> Path: | ||
| return repo_root() / venv_dir_name() |
There was a problem hiding this comment.
Add WSL + legacy .venv fallback to match documented selector behavior.
venv_dir_name() currently routes all non-Windows/non-macOS platforms to .venv.nix. The documented contract in docs/development/VENV.md expects WSL detection and a legacy .venv fallback, so this can select the wrong env for WSL users and ignore existing legacy environments.
Suggested patch
+import platform
import argparse
import os
import sys
import venv
from pathlib import Path
@@
-def venv_dir_name() -> str:
+def venv_dir_name() -> str:
+ # Prefer legacy env when present
+ legacy = repo_root() / ".venv"
+ if legacy.exists():
+ return ".venv"
+
+ # WSL detection
+ if "microsoft" in platform.uname().release.lower():
+ return ".venv.wsl"
+
if sys.platform.startswith("win"):
return ".venv.win"
if sys.platform == "darwin":
return ".venv.mac"
return ".venv.nix"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/env/select_venv.py` around lines 20 - 33, venv_dir_name() currently
maps non-win/non-mac to ".venv.nix" and misses WSL plus legacy ".venv" fallback;
update venv_dir_name() so it first detects WSL (e.g., sys.platform == "linux"
combined with presence of environment variable "WSL_DISTRO_NAME" or
"/proc/version" containing "microsoft"/"Microsoft") and return ".venv.wsl" for
WSL, then check for an existing legacy repo_root()/".venv" and return ".venv" if
that path exists, otherwise fall back to ".venv.nix"; keep repo_root() and
venv_path() usage intact so venv_path() uses the updated venv_dir_name().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
scripts/env/select_venv.pyreferenced bysetup-windows.batandsetup-unix.sh. Without it, fresh-clone setup fails in step 2/4 withNo such file or directoryand cannot create the venv. Fixes macOS setup: missing scripts/env/select_venv.py and toast notification crash on Python 3.13 #41.!scripts/env/negation to.gitignoreso the new source directory is not masked by the existingENV/venv-ignore pattern on case-insensitive filesystems (Windows, default macOS).Behavior
Contract inferred from how
setup-windows.batcalls the script:--print— resolve the platform-appropriate venv path (.venv.win/.venv.mac/.venv.nixat repo root) and print it if it exists.--ensure— create the venv at that path if missing.--ensure --print— do both.Implementation uses the stdlib
venvmodule (with_pip=True), no third-party deps. Symlinks are enabled on non-Windows platforms.Test plan
py -3.12 scripts\env\select_venv.py --ensure --printcreates.venv.winand prints its absolute path.--printonly (after creation) prints the path without recreating.setup-windows.bat's desktop-app install path (pip install -e "apps/desktop[dev]") completes successfully into that venv.setup-unix.sh(path logic is symmetric but I haven't run it).🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes