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

Better pep440 #60170

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Better pep440 #60170

wants to merge 5 commits into from

Conversation

T4mmi
Copy link
Contributor

@T4mmi T4mmi commented Jan 16, 2025

Description

This PR follows up the previous 60137, going further into supporting PEP440 and semver syntaxes (see below). It also ships extended unit tests (reflecting the 2 digits used versions in the examples).

It brings "0" padding for missmatching length versions, equality for short/long hand syntaxes (alpha/beta).

Before:

  • 0.1 < 0.1.0
  • 0.1a1 <0.1alpha1
  • 0.1post1 > 0.1dev1

After:

  • 0.1 == 0.1.0
  • 0.1a1 == 0.1alpha1
  • 0.1post1 < 0.1dev1

TODO/FIXME:

It still misses proper handling of missmatching lengths when a tail is provided (pre/post releases):
for example it sorts 1.0a1 < 1.0.0alpha1 when they should be equal ... should we delegate these comparison to a dedicated tool such as packaging.version` ?

@github-actions github-actions bot added this to the 3.42.0 milestone Jan 16, 2025
Copy link

github-actions bot commented Jan 16, 2025

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 74ab312)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 74ab312)

@Guts
Copy link
Contributor

Guts commented Jan 21, 2025

Hello @T4mmi,

It sounds me like a good move! But why not using https://pypi.org/project/packaging/, the official package maintained by PyPa as recommended by the PEP 440 and used by pip:

image

@Guts
Copy link
Contributor

Guts commented Jan 21, 2025

cc @3nids and @Gustry: this PR would align plugins manager with what we chose to enforce in QGIS Plugin CI, right?

It also moves in the same directions of a few QEPs as qgis/QGIS-Enhancement-Proposals#202 by @olivierdalang and qgis/QGIS-Enhancement-Proposals#179 by @s-m-e

@T4mmi
Copy link
Contributor Author

T4mmi commented Jan 21, 2025

Hi, I totally agree with the use of packaging.Version ! (I thought I added this solution in the PR ... forgot it, just mention it in the code as comment)
I did not implement it since packaging is not part of the STL and I have no clue where does QGIS list/maintain its own-shipped python packages.

But if this solution is the one we target I think it should simplify a lot of things for sure :)

On the other hand I'm not sure we should mix the python and other plugins dependencies (QEP suggest it), but there is a need for python dependencies also(might be out of scope)

@3nids
Copy link
Member

3nids commented Jan 21, 2025

+1 to move to use packaging

@T4mmi
Copy link
Contributor Author

T4mmi commented Jan 23, 2025

Then maybe wait before merging, I'll rework this PR first thing next week to use packaging.Version (and test cauze I'm not sure every tests go smooth) since it seems that packaging comes with QGIS (at least since 3.40)

@Guts
Copy link
Contributor

Guts commented Jan 23, 2025

Ok, cool. In the meanwhile, please switch your PR in draft state.

since it seems that packaging comes with QGIS

Make sure it's shipped with every QGIS flavor, not only Windows or Ubuntu. I also recommend to use a strong constraint on the shipped version, like packaging==24.2.

@T4mmi T4mmi marked this pull request as draft January 23, 2025 12:56
@T4mmi
Copy link
Contributor Author

T4mmi commented Jan 23, 2025

Make sure it's shipped with every QGIS flavor

If you have a lead on where to start looking, I'll take it :)

@T4mmi T4mmi marked this pull request as ready for review January 27, 2025 13:28
@T4mmi
Copy link
Contributor Author

T4mmi commented Jan 27, 2025

Ok, I think the PR is ready, I used packaging.Version() for comparison with a fallback on blunt string comparison as previous behavior. I also kept the custom prefixes {ver.|release.|r.|...} which are not PEP compatible.

Only regression I see is for post version using the trunk keyword (personaly never saw any ... it's also not present in the test samples ... but was handled by the previous code)

Also I did NOT check if packaging was shipped with all QGIS flavors ... :s

This one was previously failling
@ptitjano
Copy link
Contributor

Ok, cool. In the meanwhile, please switch your PR in draft state.

since it seems that packaging comes with QGIS

Make sure it's shipped with every QGIS flavor, not only Windows or Ubuntu. I also recommend to use a strong constraint on the shipped version, like packaging==24.2.

packaging is provided by osgeo4W: http://download.osgeo.org/osgeo4w/v2/x86_64/setup.ini
This is the version 24.0 at the moment.

I think this dependency should be added to the requirements.txt file, in the debian/control{.in} files and in the rpm/qgis.spec.template file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants