justinmayer / virtualfish

Fish shell tool for managing Python virtual environments
MIT License
1.07k stars 101 forks source link

Fix Pyenv version detection #208

Closed cecep2 closed 3 years ago

cecep2 commented 3 years ago

Hopefully this fixes #203.

To test if a Python version is available via Pyenv (e.g. when calling vf new -p 3.8.10 my_venv), VirtualFish called pyenv which python$py_version. However, pyenv which python3.8.10 fails (at least in the most recent version of Pyenv) and pyenv which python 3.8.10 seems to ignore the provided version and always prints the path of whatever Python version is currently active in Pyenv.

This fix instead checks for $PYENV_ROOT/versions/$py_version/bin/python OR $HOME/.pyenv/versions/$py_version/bin/python (if $PYENV_ROOT not set, which might for example be the case when Pyenv was installed with Homebrew). With these changes, specifying versions with just the number works again:

> pyenv versions
  system
  3.8.10
* 3.9.5 (set by /Users/cecep/.pyenv/version)
> mkvirtualenv -p 3.8.10 test_pyenv3810
Creating test_pyenv3810 via ~/.pyenv/versions/3.8.10/bin/python …
test_pyenv3810 (3.8.10) > mkvirtualenv -p 3.9.5 test_pyenv395
Creating test_pyenv395 via ~/.pyenv/versions/3.9.5/bin/python …
test_pyenv395 (3.9.5) >
justinmayer commented 3 years ago

Thank you for the contribution, @cecep2. I think you'll find that pyenv which python3.8 will work as expected, at least according to the corresponding docs. When I originally wrote the version number resolution code, I had intended to support both short (3.8) and long (3.8.10) Python version specifiers, but in retrospect that was probably too ambitious, and the support for both was unevenly applied across the various underlying tools. I think we can safely focus on the full version specifier (3.8.10) for now; some kind soul can add robust cross-tool support for short version numbers, if and when the mood strikes. 😁

cecep2 commented 3 years ago

Thanks for merging!

I think you'll find that pyenv which python3.8 will work as expected, at least according to the corresponding docs.

Interesting! This seems to be buggy. On my system with 3.8.10 and 3.9.5 installed, calling pyenv which python3.9 works, but pyenv which python3.8 gives me an error:

Screenshot 2021-06-28 at 16 49 36

Ideally the pyenv which command would work reliably with both short and long version specifiers, but as long as that's not the case we can stick with the workaround here.

I had intended to support both short (3.8) and long (3.8.10) Python version specifiers

I wonder if we could just check the length of the $py_version variable and if that length corresponds with a short version specifier like 3.8 we attach .0 at the end? Not sure if this would cause complications with asdf or pythonz though.

justinmayer commented 3 years ago

I wonder if we could just check the length of the $py_version variable and if that length corresponds with a short version specifier like 3.8 we attach .0 at the end?

That is indeed a simpler approach to the let's-handle-both-short-and-long-version-specifiers idea. One potential problem with that approach is that it could conflict with user expectations. For example, I think in an ideal world, specifying vf -p 3.8 tmp would install the latest Python 3.8.x version available (say, 3.8.11) rather than assuming 3.8.0 is the target version. But that require considerably more logic to handle correctly, of course.

cecep2 commented 3 years ago

I agree, installing the latest available version would be ideal. But considering user expectations, Pyenv doesn't handle pyenv install 3.8 (it just prints a list of available 3.8 versions), and I assume asdf can't handle this either because it's using Pyenv in the background. I think users experienced enough to have these tools installed already learned to specify Python versions in full and there is no need for VirtualFish to be 'smarter' than Pyenv :-)

justinmayer commented 3 years ago

asdf uses the python-build component of Pyenv but has its own CLI interface. So indeed asdf install python 3.8 is not supported, but asdf otherwise has certain capabilities that Pyenv does not have (and, I imagine, vice-versa). For example, asdf where python 3.9.5 prints ~/.asdf/installs/python/3.9.5 , which I imagine Pyenv does not support.

But I digress… I agree that for the time being, we don't need to be more clever than the underlying tooling. There is a part of me that, when vf -p 3.8 tmp is invoked, wants VirtualFish to run asdf list python to capture the installed version numbers and then select the latest 3.8.x version, but … it's only a small part of me. 😉

Many thanks for all your enhancements, which I bundled up into the release that was just published. ✨