-
Notifications
You must be signed in to change notification settings - Fork 249
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
Drop LegacySpecifier
and LegacyVersion
#407
Conversation
WDYT about dropping |
This is more a question, should
I believe the docs need to be adjusted (I grepped for "legacy"):
|
Oooooo! We need to run doctest in our CI. :) |
The points made by @jdufresne all make sense to me. |
@pradyunsg any reason not to merge this once you have a chance to fix the merge conflicts? |
Documentation updates, but nothing other than that If someone else can get to updating this before I can, just say so and pick this up. @brettcannon if that someone ends up being you, you should be able to push to this branch directly too. :) |
We should probably get this merged before we cut a 21.0 release. |
This would have an impact on pip’s type checks, I think. Some parts in pip still rely on those Legacy types, and if this isn’t ready very soon, pip will have trouble updating the type hints to include 21.0 in the next pip release. So either we hurry or this needs to wait a while… (it would be a shame if musllinux needs to wait another three months for this) |
Actually, I think this has been blocking our release for a while and given that we've not released things like musl stuff and more because of it... I'm inclined to suggest that we defer this to after cutting a release or two. Does that sound stupid or reasonable? |
I'm fine with deferring, but then when can we definitely take out these legacy classes? I just want to make sure that if we are postponing for pip that we do make sure pip is able to update appropriately so we don't drag this out (we do have people accidentally using these classes still, so extended delay wouldn't be great). |
I think once there's a pip release with whatever stuff we have merged right now. Basically, I think there's a bunch of tiny details to figure out here, and we can tackle them in a more relaxed manner if there's no "pip release is this month" pressure here. |
because LegacyVersion is deprecated. closes #180.
@pradyunsg There's been several pip releases since your last comment, is pip in a place where it could handle the removal of these classes now? |
Not as far as I’m aware. |
### Summary & Motivation We were using `packaging.parse` to compare mysql server versions, which broke when `0.23.0` was released with dropped support for `LegacyVersion`. This rolls our own custom version compares to find minimum supported versions, which is tolerant of non semver compliant version strings. The regex parses all the numeric values, and does tuple int comparisons. Reported in #11794 Reference links: pypa/packaging#407 ### How I Tested These Changes Added some test cases that override the server version string to exercise the parsing logic.
### Summary & Motivation We were using `packaging.parse` to compare mysql server versions, which broke when `0.23.0` was released with dropped support for `LegacyVersion`. This rolls our own custom version compares to find minimum supported versions, which is tolerant of non semver compliant version strings. The regex parses all the numeric values, and does tuple int comparisons. Reported in #11794 Reference links: pypa/packaging#407 ### How I Tested These Changes Added some test cases that override the server version string to exercise the parsing logic.
The jboss.container.maven.35.bash module cause an Invalid version error with python-packaging to version >= 22, for further details see pypa/packaging#407
The jboss.container.maven.35.bash module cause an Invalid version error with python-packaging to version >= 22, for further details see pypa/packaging#407
The jboss.container.maven.35.bash module cause an Invalid version error with python-packaging to version >= 22, for further details see pypa/packaging#407
- this release does not add any new functionality nor modify existing functionality - **SUMMARY** - see commit 21bd027 for the initial attempt at fixing the upload error - this change fixed the upload error, but changed the functionality of `python_requires` since any `3.0.N` version of python would become incompatible with this change - see commit d3e02a7 for the proper fix to the original upload error while maintaining compatibility for any `3.0.N` version of python - **EXPLANATION (taken from pull request thread)** After doing some digging, this is the likely culprit for what caused this problem: - pypa/packaging#407 - which was the result of pypa/packaging#566 (related: pypa/packaging#530 and pypa/packaging#321) - which in turn looks like the result of the discussion at https://discuss.python.org/t/how-to-pin-a-package-to-a-specific-major-version-or-lower/17077/8 It looks like this is the expected behavior as defined in PEP 440 under the [Inclusive ordered comparison section](https://peps.python.org/pep-0440/#inclusive-ordered-comparison): > An inclusive ordered comparison clause includes a comparison operator and a version identifier, and will match any version where the comparison is correct based on the relative position of the candidate version and the specified version given the consistent ordering defined by the standard [Version scheme](https://peps.python.org/pep-0440/#version-scheme). Following the link to the [Version scheme](https://peps.python.org/pep-0440/#version-scheme) section and looking at the specification under the [Public version identifiers](https://peps.python.org/pep-0440/#public-version-identifiers) section: > The canonical public version identifiers MUST comply with the following scheme: > `[N!]N(.N)*[{a|b|rc}N][.postN][.devN]` > Public version identifiers MUST NOT include leading or trailing whitespace. > > Public version identifiers MUST be unique within a given distribution. > ... The last line included above seems to be the "loose implementation" of the version modifier that the issues and pull requests I linked to at the very top were discussing ("After doing some digging, this is the likely culprit for what caused this problem"). Once that "loose implementation" was fixed, any package that didn't follow the PEP 440 specification for version identifiers broke. In this package, the break occurred because of the `>=3.0.*` comparison, which IS NOT unique within a given distribution, as opposed to `>=3` (what commit d3e02a7 does), which IS unique within a given distribution. To clarify: it looks like the problem we see in this issue is because of implementation fixes in the packaging tools to more closely follow PEP 440, specifically **"Public version identifiers MUST be unique within a given distribution."** Any package that relied on the previous implementation that loosely verified the PEP 440 specification but did not strictly follow PEP 440 broke after the implementation of the packaging tool(s) were fixed to more closely follow PEP 440. More explicitly (from [this comment](https://discuss.python.org/t/how-to-pin-a-package-to-a-specific-major-version-or-lower/17077/8) on the [How to pin a package to a specific major version or lower](https://discuss.python.org/t/how-to-pin-a-package-to-a-specific-major-version-or-lower/17077) discussion): > Christopher already made the response I was going to make: for PEP 440 as written, using wildcards outside of “==” and “!=” comparisons isn’t valid. > > Allowing them for “>=” and “<=” would be reasonable, but it would involve a PEP to fix the spec. (It wasn’t a conscious choice to exclude them, we just didn’t notice at the time that the inclusive ordered comparison operators weren’t formally defined as combining an exclusive ordered comparison with a version match, so the tools have been written to ignore the wildcard instead of paying attention to it) > > Making a coherent definition wouldn’t be too hard: just ignore the wildcard for the exclusive ordered comparison part and keep it for the version matching part. Here are some other posts that aren't directly relevant, but might be interesting tangents for anyone interested in packaging problems: - https://stackoverflow.com/questions/19534896/enforcing-python-version-in-setup-py - https://packaging.python.org/en/latest/guides/distributing-packages-using-setuptools/#python-requires - https://packaging.python.org/en/latest/guides/distributing-packages-using-setuptools/#package-data - https://setuptools.pypa.io/en/latest/userguide/datafiles.html - https://peps.python.org/pep-0345/#requires-python - https://stackoverflow.com/questions/8795617/how-to-pip-install-a-package-with-min-and-max-version-range - https://python3statement.org/practicalities/ - https://discuss.python.org/t/requires-python-upper-limits/12663/20 - https://stackoverflow.com/questions/13924931/setup-py-restrict-the-allowable-version-of-the-python-interpreter/13925176#13925176
Since we are only using parse_version for comparison (typically checking that we are greater than some minimum version for tool or package), one would think we can use packaging.version.parse as if it was parse_version Unfortunately, this requires conforming to PEP-440 version definitions, which does not work for e.g. autotools (2.72d) nor older openssl (1.1.1p). We rely in these (and to be sure other) cases on the LegacyVersion behavior. https://packaging.python.org/en/latest/specifications/version-specifiers/#summary-of-differences-from-pkg-resources-parse-version "This specification purposely restricts the syntax which constitutes a valid version while pkg_resources.parse_version attempts to provide some meaning from any arbitrary string." In order to have the least impact to the overall code, we instead add packaging_legacy to requirements.txt and use packaging_legacy.version.parse as if it was parse_version. https://pypi.org/project/packaging-legacy/ pypa/packaging#407 Since pypi.org itself is depending on packaging_legacy (in fact, a pypi dev developed the package), we can expect it to be supported for quite some time. pypi/warehouse#13500 [YOCTO #15348] Signed-off-by: Tim Orling <[email protected]>
22.0 dropped LegacyVersion: pypa/packaging#407 This makes it so our branches can no longer be parsed as versions
22.0 dropped LegacyVersion: pypa/packaging#407 This makes it so our branches can no longer be parsed as versions
* readthedocs: update config * pin packaging==21.3 - 22.0 dropped LegacyVersion: pypa/packaging#407 - This makes it so our branches can no longer be parsed as versions
22.0 dropped LegacyVersion: pypa/packaging#407 This makes it so our branches can no longer be parsed as versions
22.0 dropped LegacyVersion: pypa/packaging#407 This makes it so our branches can no longer be parsed as versions
Recent 18.2.4 release contained a cherry-pick of 0985e201342 ("ceph-volume: use 'no workqueue' options with dmcrypt") and that patch introduced parsing the output of `cryptsetup --version`, but it had a coupling on either a old (or distro-specific) cryptsetup version output and/or some legacy behavior of the python `packaging` module that is used for the version parsing. As the `cryptsetup` tool on bookworm outputs the following version: > cryptsetup 2.6.1 flags: UDEV BLKID KEYRING KERNEL_CAPI As the extra strings at the end are not accepted anymore by the `packaging` python module in bookworm [0], this test fails ceph-volume when encrypted OSDs are used, which we do by default. [0]: due to pypa/packaging#407 being included in the bookworm version To make this work again cherry-pick two patches that first filter out the numerical part from the raw version output using a regex and only pass that to the version parsing call. Fixes: https://tracker.ceph.com/issues/66393 Signed-off-by: Thomas Lamprecht <[email protected]>
…427) * Set maximum version for packaging, which has removed LegacyVersion pip-audit uses `packaging.version.LegacyVersion` to parse some version numbers, and this is removed in packaging 22.0 (pypa/packaging#407) Closes #426 * test: Remove `pyparsing` as this is no longer a dependency of `packaging` Co-authored-by: Alex Cameron <[email protected]>
…427) * Set maximum version for packaging, which has removed LegacyVersion pip-audit uses `packaging.version.LegacyVersion` to parse some version numbers, and this is removed in packaging 22.0 (pypa/packaging#407) Closes #426 * test: Remove `pyparsing` as this is no longer a dependency of `packaging` Co-authored-by: Alex Cameron <[email protected]>
Follow up to #342.
Closes #530
Let's do this. :)