replaced distutils LooseVersion with packaging.version.parse
Fixes #4072, and #3996 partially.
This is my first PR for easybuild-framework, help is appreciated :)
Adding an extra requirement is quite a lot to ask. I implemented a simple copy of LooseVersion in #3794 and is purposely simple.
And I mean from #4072
comparing Java-1.8_265-b01 to Java-1.8.0_292, i.e. 1.8_265 to 1.8.0_292
What does it really mean to compare 1.8_265 to 1.8.0_292? Without someone checking those shouldn't be (easily) comparable so an error in this case is good IMO.
The pros/cons for the two ways of resolving the version comparison problem (feel free in adding/subtracting pros/cons from both lists, the following is just my opinion)
packaging.version.parse:
Pros:
- Explicitly adheres to Python's extensive and well defined versioning scheme (PEP-0440)
- Available for all Python versions (including 2.7)
- Is the advised way of replacing
disutils.version.LooseVersion(PEP-0632)-
packagingis already included insetuptools -
distutils(which is used in e.g.setup.py) is deprecated and needs to be replaced bysetuptoolsanyway by Python 3.12 - PEP-0632 is also linked in the thread from which @Flamefire takes the fix for LooseVersion's str/int comparison issue #3996.
-
Cons:
- Adds a requirement
- Makes a (possibly wrong) decision in ambiguous cases such as comparing Java-1.8_265-b01 to Java-1.8.0_292, i.e. 1.8_265 to 1.8.0_292
easybuild-framework.tools.LooseVersion
Pros:
- No additional requirement
- "Simple" implementation
Cons:
- Doesn't adhere to any standard(ized) versioning scheme
- Re-invents the wheel
- Doesn't handle complex cases (automatically)
I understand why the ambiguous case of comparing 1.8_265 to 1.8.0_292 is unfortunate, but breaking tools such as tweak.py in favor of manual decision making is (in my opinion) not a solution.
We need to also consider the easyblocks and the many many places they use LooseVersion (and not only the ones we have, but also whatever people are using outside of the main repo). SI suspect noone is volunteering to changing all of those to parse and checking that nothing broke, so a switch to parse is something i think we have to introduce gradually there, and we regardless need to provide the easybuild-framework.tools.LooseVersion during that transition.
My 2 cents on this: while it seems like we may not be able to avoid a dependency on setuptools from Python 3.12 onwards (to be checked, I still hope we can avoid that since we know it's going to be a PITA if not), my preference goes to "vendoring" LooseVersion as is done in #3794.
Not only does it avoid introducing a dependency on a Python package outside the standard library, but it also means that nothing really changes, so there won't be surprises with version checking suddenly being done slightly differently.
We have a broad set of test cases in the easyconfigs repository that we can test with, there are other interesting cases like OpenFOAM, LAMMPS, etc. which we should also check.
That said, I think whatever road we take, this is EasyBuild 5.0 material, since the potential impact is not small (and Python 3.12 is not available yet).
That said, I think whatever road we take, this is EasyBuild 5.0 material, since the potential impact is not small (and Python 3.12 is not available yet).
I don't see how #3794 is a breaking change. It is a more or less 1:1 copy of the existing LooseVersion with a corner case fixed which didn't work before simplifying code that wants to use it. I would suggest to merge that and transition to it soon so that we can test it extensively before the next release.
That said, I think whatever road we take, this is EasyBuild 5.0 material, since the potential impact is not small (and Python 3.12 is not available yet).
I don't see how #3794 is a breaking change. It is a more or less 1:1 copy of the existing LooseVersion with a corner case fixed which didn't work before simplifying code that wants to use it. I would suggest to merge that and transition to it soon so that we can test it extensively before the next release.
I didn't call #3794 a breaking change, but it definitely is an intrusive one. In theory, it's the same code (except, it's not really, since there's a bugfix on top).
We've discussed #3794 briefly during the EasyBuild conf call (again) today, and the general consensus is that we should just go ahead with it (without waiting for EasyBuild v5.0).
Closing this since #3794 has been merged