-
Notifications
You must be signed in to change notification settings - Fork 37
Fix PyEnv Windows path calculation for environment grouping #1210
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
Conversation
On Windows, pyenv-win is at ~/.pyenv/pyenv-win/bin/pyenv.bat, so we need to go up 3 directory levels (not 2) to reach the pyenv root directory (~/.pyenv). This fixes environment grouping (PYENV_VERSIONS vs PYENV_ENVIRONMENTS) which was broken because versionsPath and envsPaths were calculated from the wrong parent directory. Co-authored-by: karthiknadig <3840081+karthiknadig@users.noreply.github.com>
Co-authored-by: karthiknadig <3840081+karthiknadig@users.noreply.github.com>
karthiknadig
left a comment
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 core fix (extracting getPyenvDir with platform-aware logic) is a good idea, but I'm concerned the 3-levels-up calculation on Windows may produce the wrong root directory for environment grouping. Please verify what info.prefix the PET server returns for pyenv-win environments — if prefixes are under ~/.pyenv/pyenv-win/versions/, then versionsPath should also be computed relative to ~/.pyenv/pyenv-win, not ~/.pyenv.
…ests - Use PYENV_ROOT env var as primary source for pyenv root directory - Revert 3-levels-up Windows logic to 2-levels-up (pyenv-win versions are at ~/.pyenv/pyenv-win/versions/, not ~/.pyenv/versions/) - Add test for PYENV_ROOT preference - Fix POSIX test to use absolute paths with path.sep - Fix Windows test expectation to reflect correct 2-levels-up behavior Co-authored-by: karthiknadig <3840081+karthiknadig@users.noreply.github.com>
You were right — pyenv-win prefixes are under |
karthiknadig
left a comment
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.
LGTM! The updated approach is much better — using PYENV_ROOT as the primary source and keeping a consistent 2-levels-up fallback avoids the fragile platform-specific directory counting. Tests cover all three cases well.
nativeToPythonEnvinpyenvUtils.ts- extractgetPyenvDirhelperPYENV_ROOTenv var as primary source, revert 3-levels-up to 2-levels-up (old behavior was correct for pyenv-win since versions live under~/.pyenv/pyenv-win/versions/)path.sepPYENV_ROOTpreferenceOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.