-
Notifications
You must be signed in to change notification settings - Fork 203
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
vendor distutils.version.LooseVersion as easybuild.tools.LooseVersion (since distutils is deprecated in Python 3.10) #3794
Conversation
1373a1d
to
fd5ced6
Compare
@boegel Updated this. As this is required for #3963 I'd like to get this in. IMO having this class is very good, especially as we now have full control over it. This means:
|
from itertools import izip_longest as zip_longest | ||
|
||
|
||
class LooseVersion(object): |
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.
Maybe we should rename this to something else than LooseVersion
?
It's clearly based on that, but it's been fiddled with quite a bit, and behaves a bit different for some specific cases, so not naming it LooseVersion
makes sense to me to avoid confusion with the original?
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.
I find the name good and would like to keep it:
- It really is a "loose version" as it doesn't care much about the format or specifics like pre-releases etc.
- It is a drop-in replacement, i.e. only change is the
import
statement while everything else can be kept which reduces work, see this PR - Only change is the comparison but that is basically a backported bugfix not a real behavior change, so I wouldn't say it behaves different, just consistent across different python versions (i.e. with and without the bugfix)
If you really want a different name, please suggest one you find better fitting. But note that the diff will be much larger as every use will have to be modified.
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.
Yeah, good points, thanks for clarifying! 👍
Port/Copy of distutils.version.LooseVersion as distutils will be deprecated and eventually removed from Python. Slightly simplified and with tests
When a number is in a place where the other version has a letter, the comparison would fail in Python 3 but succeed in Python 2. Add patch from https://bugs.python.org/issue14894
This now works directly. Also fix the sort_looseversions function for the distutils version of LooseVersion
fd5ced6
to
99fc332
Compare
Another (short-term) option here could be to keep using Longer-term (EasyBuild v5.0?) we should probably fully take matters into our own hands, to avoid a required dependency outside of the Python standard library going forward... |
Why wait and use different versions on different Pythons creating possible hard-to-recreate bugs? So why not just use this everywhere now? Is there any downside? |
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.
This is a great step towards fixing #3963, and although I was a bit reluctant to take this approach at first, I have to admit it the most sensible solution.
Thanks a lot for your efforts on this @Flamefire!
Port/Copy of
distutils.version.LooseVersion
asdistutils
will be deprecated and eventually removed from Python 3.10+Slightly simplified and with tests
Also includes the fix from https://bugs.python.org/issue14894 to compare any LooseVersion instance in Python 3
BTW: There is a major pitfall we might actually run into at some points:
With the included fix we could actually fix that such that missing components are considered equal. So the above would return equal which IMO is more sane for our usages
This is how e.g. CMake does its
if(ver VERSION_GREATER ver2)