Skip to content
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

Fix Pyenv version detection #208

Merged
merged 1 commit into from Jun 28, 2021
Merged

Conversation

cecep2
Copy link
Contributor

@cecep2 cecep2 commented May 22, 2021

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) >

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 and 'pyenv which python 3.8.10'
ignores the provided version and always prints whatever Python is
currently active.

This fix instead checks $PYENV_ROOT/versions/$py_version/bin/python OR
$HOME/.pyenv/versions/$py_version/bin/python (if $PYENV_ROOT not set)
@justinmayer
Copy link
Owner

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. 😁

@justinmayer justinmayer merged commit 4662bc9 into justinmayer:master Jun 28, 2021
@cecep2
Copy link
Contributor Author

cecep2 commented Jun 28, 2021

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
Copy link
Owner

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
Copy link
Contributor Author

cecep2 commented Jun 29, 2021

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
Copy link
Owner

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. ✨

@cecep2 cecep2 deleted the fix_pyenv branch June 29, 2021 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to specify minor python version when using pyenv?
2 participants