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

vendor distutils.version.LooseVersion as easybuild.tools.LooseVersion (since distutils is deprecated in Python 3.10) #3794

Merged
merged 7 commits into from
Dec 21, 2022

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Aug 3, 2021

Port/Copy of distutils.version.LooseVersion as distutils 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:

        # Careful here: 1.0 > 1 !!!
        self.assertGreater(LooseVersion('1.0'), LooseVersion('1'))
        self.assertLess(LooseVersion('1'), LooseVersion('1.0'))

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)

@boegel boegel added this to the next release (4.4.2?) milestone Aug 4, 2021
@boegel boegel modified the milestones: 4.4.2, release after 4.4.2 Sep 1, 2021
@boegel boegel changed the title Add easybuild.tools.LooseVersion Add easybuild.tools.LooseVersion (since distutils is deprecated in Python 3.10) Oct 13, 2021
@boegel boegel modified the milestones: 4.5.0, 4.x Oct 13, 2021
@Flamefire Flamefire force-pushed the looseversion branch 2 times, most recently from 1373a1d to fd5ced6 Compare July 26, 2022 14:15
@Flamefire
Copy link
Contributor Author

@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:

  • Fix the bug when sorting
  • Drop-in replacement
  • Easy use in comparisons e.g. version = LooseVersion(self.version) and later if version > '1.4' and version < '1.6' reads very nice.

from itertools import izip_longest as zip_longest


class LooseVersion(object):
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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! 👍

@boegel boegel changed the title Add easybuild.tools.LooseVersion (since distutils is deprecated in Python 3.10) add easybuild.tools.LooseVersion (since distutils is deprecated in Python 3.10) Aug 3, 2022
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
@boegel
Copy link
Member

boegel commented Oct 14, 2022

Another (short-term) option here could be to keep using LooseVersion for Python versions where it is available, and fall back to the looseversion Python package that is available via PyPI for Python > = v3.12 .

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

@Flamefire
Copy link
Contributor Author

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?

@boegel boegel modified the milestones: 4.x, next release (4.6.3?) Oct 28, 2022
Copy link
Member

@boegel boegel left a 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!

@boegel boegel merged commit aaa6b38 into easybuilders:develop Dec 21, 2022
@Flamefire Flamefire deleted the looseversion branch December 22, 2022 15:26
@boegel boegel changed the title add easybuild.tools.LooseVersion (since distutils is deprecated in Python 3.10) vendor distutils.version.LooseVersion as easybuild.tools.LooseVersion Jan 8, 2023
@boegel boegel changed the title vendor distutils.version.LooseVersion as easybuild.tools.LooseVersion vendor distutils.version.LooseVersion as easybuild.tools.LooseVersion (since distutils is deprecated in Python 3.10) Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants