asdf-community / asdf-python

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

fix `asdf latest python` #141

Closed jdx closed 2 years ago

jdx commented 2 years ago

Fixes #140

This contains bug fixes related to shorthand PR #139.

This makes the partial version lookup compatible with GNU grep and adds a "-latest" suffix to shorthands. asdf list-all python broke asdf latest python without that suffix but asdf knows to ignore versions with -latest.

Note this means that asdf install python 3.7 won't work anymore (but that change was only in for less than 24 hours). We'll need to use asdf install install python 3.7-latest.

jdx commented 2 years ago

Tested the following on mac and linux and seems to work:

asdf install python 3.9-latest
asdf local python 3.9-latest
python -V
asdf latest python 3.9
asdf latest python
jdx commented 2 years ago

Note that this won't break people's installs that used the 3.9 style shorthands but they won't be able to use that shorthand for new installs after this is merged.

jdx commented 2 years ago

@danhper sorry to bother you but it would be really good if you could review this PR. Right now asdf latest python isn't working but this will fix it.

slewsys commented 2 years ago

This PR is not quite right:

asdf latest python 3.9 --> 3.9.13

but

asdf latest python 3.1 --> 3.10.6

was expecting 3.1.5. I wonder if the decimal point is getting quoted in your regular expression? For instance, in PR #142, I use ${version//./\\.}. And although,

asdf install python 3.1-latest --> downloads 3.1.5

the -latest suffix should not be necessary.

Finally, please try to use POSIX syntax where possible. asdf-vm is used by systems for which sort and grep are not GNU. Again, please see PR #142.

jdx commented 2 years ago

asdf latest python 3.1 is in fact broken but it doesn't appear to ever have worked in this plugin. It's not something either of my PRs caused. I assume since 3.1 was deprecated like 6 years ago it's just not something people have tried to use much if at all.

the -latest suffix should not be necessary.

It is critical. That's the primary fix in here as I explain in the description. Without appending the -latest suffix then asdf latest python 3.10 won't work.

Finally, please try to use POSIX syntax where possible. asdf-vm is used by systems for which sort and grep are not GNU. Again, please see PR https://github.com/danhper/asdf-python/pull/142.

That's fair, I'm not familiar with writing POSIX compatible scripts or how to make sure I'm not using tools that aren't generally available. If you have any tips on methods or tools I could use to check my code for that I'd be happy to use them.

slewsys commented 2 years ago

It is critical. That's the primary fix in here as I explain in the description. Without appending the -latest suffix then asdf latest python 3.10 won't work.

Please try PR #142. gh pr checkout 142 and verify that both asdf latest python 3.10 works as well as asdf install python 3.10. What am I missing? Though I see that 3.1 is indeed still broken here: asdf latest python 3.1 --> 3.10.6

I'm not familiar with writing POSIX compatible scripts or how to make sure I'm not using tools that aren't generally available. If you have any tips on methods or tools I could use to check my code for that I'd be happy to use them.

It might be worth bookmarking SUSv4. But generally, I'd recommend going GNU if you can get away with it :)

jdx commented 2 years ago

Removed non-POSIX compatible usage of sort -V. I believe this is entirely POSIX compatible now.

slewsys commented 2 years ago

Looks good. It might be nice to support trailing decimal point, but not essential.

jdx commented 2 years ago

It might be nice to support trailing decimal point, but not essential.

I'd be happy to but it seems to work?

❯ asdf latest python 3.9.
3.9.13

The only place it doesn't work would be here, but I'm not sure this is a realistic use-case:

❯ asdf install python 3.10.-latest

Is there a use-case I'm not considering?

In any case, I appreciate your help with this @slewsys making this POSIX compatible. Also sorry about the bug in my initial PR. I'm kicking myself that I hadn't tested it more thoroughly.

slewsys commented 2 years ago

Testing shell code is hard :( I've closed PR #142. Thank you for your work!

emcd commented 2 years ago

Finally, please try to use POSIX syntax where possible. asdf-vm is used by systems for which sort and grep are not GNU. Again, please see PR #142.

Technically speaking, [[:digit:]] is POSIX syntax. See the POSIX standard for regular expressions, particularly item 6 of section 9.3.5.:

All character classes specified in the current locale shall be recognized. A character class expression is expressed as a character class name enclosed within bracket- ( "[:" and ":]" ) delimiters. The following character class expressions shall be supported in all locales: [:alnum:] [:cntrl:] [:lower:] [:space:] [:alpha:] [:digit:] [:print:] [:upper:] [:blank:] [:graph:] [:punct:] [:xdigit:]

That said, if we are assuming that only ASCII 0-9 can form digits of version numbers (which I think is a reasonable assumption), then I have no objection to using [0-9]. Any regex syntax I can think of supports that range notation, so it is probably more portable than the POSIX [[:digit:]].

slewsys commented 2 years ago

Since character classes are POSIX, I believe [[:digit:]] generally matches only the ASCII range [0-9]. The Python docs, in particular, point to the Unicode standard, which indicate that this is the case for Python as well. My objection was more to the use of \d in PR #139 which unfortunately isn't supported by *BSD, for instance. My personal preference for [0-9] over [[:digit:]] is that it's shorter :)

emcd commented 2 years ago

Since character classes are POSIX, I believe [[:digit:]] generally matches only the ASCII range [0-9]. The Python docs, in particular, point to the Unicode standard, which indicate that this is the case for Python as well. My objection was more to the use of \d in PR #139 which unfortunately isn't supported by *BSD, for instance. My personal preference for [0-9] over [[:digit:]] is that it's shorter :)

👍 I agree that [0-9] is shorter and more clearly expresses the correct intent here; I do think it is the correct way to go in this context. I thought you were responding to the use of [[:digit:]] to replace \d, which is a more apples-to-apples replacement, since that change had already been made by the time you joined the conversation.

Technically, Python \d matches Unicode category Nd, which includes much more than [0-9], unless you supply the ASCII flag or provide the (?a) pattern inline when building the regex. The point about POSIX [[:digit:]] being locale-aware means that it could also match things other than [0-9]. In the case of Python version numbers, I think we just want [0-9] for digits though. I'm not aware of anyone using Bengali or Thai digits in Python version numbers, for example. :)

danhper commented 2 years ago

Hi, and thanks for the fix. I feel like this adds a bit too many inconsistencies, so I'd suggest we try to get latest:3.7 or so to work with .tool-versions in the main asdf repo rather than introducing another way of doing the same thing here. I reverted the previous PR to avoid leaving asdf latest broken.