asdf-community / asdf-python

Python plugin for the asdf version manager
https://github.com/asdf-vm/asdf
MIT License
643 stars 55 forks source link

Bug: `asdf latest python <version>` and `asdf install python <shorthand-version>` broken with latest commits. #140

Closed emcd closed 1 year ago

emcd commented 1 year ago

Probably due to #139, if I had to guess. Need to confirm.

Symptoms

latest now shows:

$ asdf latest python 3.7
3.7

Expected 3.7.13 instead.

install now shows:

$ asdf install python 3.7
python-build  /home/me/.asdf/installs/python/
Usage: python-build [-kpv] <definition> <prefix>
       python-build --definitions
       python-build --version

  -k/--keep        Do not remove source tree after installation
  -p/--patch       Apply a patch from stdin before building
  -v/--verbose     Verbose mode: print compilation status to stdout
  -4/--ipv4        Resolve names to IPv4 addresses only
  -6/--ipv6        Resolve names to IPv6 addresses only
  --definitions    List all built-in definitions
  --version        Show version of python-build
  -g/--debug       Build a debug version

Expected python 3.7.13 is already installed or an installation activity instead. Looks like version is being parsed to an empty string inside the asdf-python code.

asdf install python 3.7.13 works as expected.

emcd commented 1 year ago

Confirmed that #139 broke latest:

~/.asdf/plugins/python$ git revert 3c2bb7b8a18e0aea01bd98e58d41f0e39485076d
Auto-merging bin/install
[master 54676d4] Revert "added major/minor shorthands"
 2 files changed, 12 insertions(+), 60 deletions(-)
 rewrite bin/list-all (73%)
$ asdf latest python 3.7
3.7.13
emcd commented 1 year ago

Looks like the problem in #139 is an assumption about using \d for the digit character class:

$ ~/.asdf/plugins/python/pyenv/plugins/python-build/bin/python-build --definitions | grep -E '^\d+\.\d+\.\d+$' | wc -l
0
$ ~/.asdf/plugins/python/pyenv/plugins/python-build/bin/python-build --definitions | grep -E '^[[:digit:]]+\.[[:digit:]]+\.[[:digit:]]+$' | wc -l
154
$ grep --version
grep (GNU grep) 3.4
jdx commented 1 year ago

I'll put a patch in ASAP

jdx commented 1 year ago

@emcd would you mind testing #141? It seems to work for me and I'm going to test it on Linux to use GNU grep but it would be nice to have an extra set of eyes

emcd commented 1 year ago

Thanks @jdxcode - sorry for the delay - was busy with work. Here's what I see after pulling master from your repo and using it:

$ asdf latest python 3.7
3.7.13

which is good. However:

$ asdf install python 3.7
python-build 3.7 /home/me/.asdf/installs/python/3.7
python-build: definition not found: 3.7

which does not seem to be intended behavior. I can dive more on this about 5 or 6 hours from now. Could be something on my end since I added your repo as a new remote and then cut over to using your master rather than danhper's master - caused a merge commit on my end. Will do something cleaner to double-check when I have time, if you haven't sorted it out before then.

jdx commented 1 year ago

@emcd perhaps I should make this clearer, this change removes the 3.7 shorthands in favor of 3.7-latest which is more compatible with asdf. Try asdf install python 3.7-latest

emcd commented 1 year ago

@jdxcode : Oh, I didn't read the commit. :) Was in a hurry - am doing this while eating lunch. How would that be different than the existing latest:3.7 behavior?

Also, this triggered a fresh install of 3.7.13, which I already had installed:

$ asdf install python 3.7-latest
python-build 3.7.13 /home/me/.asdf/installs/python/3.7.13
Downloading Python-3.7.13.tar.xz...
jdx commented 1 year ago

-latest suffix versions are explicitly excluded by asdf for the purpose of asdf latest: https://github.com/asdf-vm/asdf/blob/5334d1db3d390c46ed49101528f74483eb6b2987/lib/functions/versions.bash#L153

asdf install latest:3.7 works but in a .tool-versions file you can't have python latest:3.7. This method will work for .tool-versions.

emcd commented 1 year ago

-latest suffix versions are explicitly excluded by asdf for the purpose of asdf latest: https://github.com/asdf-vm/asdf/blob/5334d1db3d390c46ed49101528f74483eb6b2987/lib/functions/versions.bash#L153

Ah. TIL.

asdf install latest:3.7 works but in a .tool-versions file you can't have python latest:3.7. This method will work for .tool-versions.

Ah, true. I think I came to that realization once upon a time when I tried latest:<version> in .tool-versions.

So, I guess things work aside from asdf install python <shorthand-version>-latest triggering unexpected behavior if you already have the latest version installed.

jdx commented 1 year ago

Yeah it does seem there are some minor edge cases. If you install with asdf install python 3.7-latest and run it again after a new 3.7 comes out it will refuse because it already has "3.7-latest" installed. If you manually install the latest 3.7 and install 3.7-latest on top of it, it will reinstall 3.7.x but that's kind of okay since it will also add the 3.7-latest symlink (which is what you want). It's just adding some unnecessary work. I could put a conditional in there to skip reinstalling, but I'd like to avoid the branching and complexity.

I'm more concerned about not being able to install 3.7-latest when a new version comes out, but FWIW asdf-nodejs is susceptible to this issue as well.

I still think this should be merged because without it asdf latest is broken. I'll think about these edge cases. One solution might just be to make latest:3.7 work in asdf for all languages.

slewsys commented 1 year ago

@emcd perhaps I should make this clearer, this change removes the 3.7 shorthands in favor of 3.7-latest which is more compatible with asdf. Try asdf install python 3.7-latest

Not sure what you mean by "more compatible". Try, for instance, asdf latest nodejs 16 --> 16.16.0

jdx commented 1 year ago

That is explained in the description of #141

slewsys commented 1 year ago

My understanding is that after PR #141, asdf latest python 3.10 would not return (currently) 3.10.6. My point is that there is precedence for that working in many other asdf plugins (nodejs, golang, rust, perl, ruby...).

jdx commented 1 year ago

you've got it backwards: #141 fixes asdf latest which was broken in #139

❯ git co 3c2bb7b && asdf latest python 3.10
HEAD is now at 3c2bb7b added major/minor shorthands
3.10

❯ git co 2333d78 && asdf latest python 3.10
HEAD is now at 2333d78 map add -latest to shorthands
3.10.6