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

Drop LegacySpecifier and LegacyVersion #407

Merged
merged 7 commits into from Jul 8, 2022

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Feb 28, 2021

Follow up to #342.
Closes #530

Let's do this. :)

@jdufresne
Copy link
Contributor

jdufresne commented Feb 28, 2021

WDYT about dropping _BaseVersion in favor of just a Version class (merging the classes really)? Likewise, how about dropping BaseSpecifier?

@jdufresne
Copy link
Contributor

This is more a question, should canonicalize_version now raise an InvalidVersion exception rather than returning the legacy version string?

packaging/utils.py
47               # Legacy versions cannot be normalized

I believe the docs need to be adjusted (I grepped for "legacy"):

docs/specifiers.rst
57       (:class:`Specifier`) or as a legacy, setuptools style specifier
58       (deprecated :class:`LegacySpecifier`). You may combine
94           :class:`Version`, or a deprecated :class:`LegacyVersion` object, is
110          deprecated :class:`LegacySpecifier`) instances in this specifier set.
115          and deprecated :class:`~.LegacyVersion` instances and will then filter
172  .. class:: LegacySpecifier(specifier, prereleases=None)
178      This class abstracts the handling of a single legacy, setuptools style

docs/version.rst
54       will parse it as a deprecated :class:`LegacyVersion`.
141  .. class:: LegacyVersion(version)
160          :class:`LegacyVersion` instances are always ordered lower than :class:`Version` instances.
162          >>> from packaging.version import Version, LegacyVersion
164          >>> v2 = LegacyVersion("1.0")
167          >>> v3 = LegacyVersion("1.3")
172          other versions that are not (:class:`LegacyVersion`). Examples include versions with `Pre-release spelling`_ and
198          <LegacyVersion('0.9.8t')>
207          :class:`LegacyVersion`. This will always be the entire version string.
212          :class:`LegacyVersion` instance. This will always be the entire version
219          :class:`LegacyVersion` instances always compare less than
226          primarily to allow a :class:`LegacyVersion` to be used as a stand in
233          :class:`LegacyVersion` to be used as a stand in for a :class:`Version`.
239          :class:`LegacyVersion` to be used as a stand in for a :class:`Version`.
243          A boolean value indicating whether this :class:`LegacyVersion`
252          :class:`LegacyVersion` to be used as a stand in for a :class:`Version`.
256          A boolean value indicating whether this :class:`LegacyVersion`
265          :class:`LegacyVersion` to be used as a stand in for a :class:`Version`.
269          A boolean value indicating whether this :class:`LegacyVersion`

@pradyunsg
Copy link
Member Author

I believe the docs need to be adjusted (I grepped for "legacy"):

Oooooo! We need to run doctest in our CI. :)

@brettcannon
Copy link
Member

The points made by @jdufresne all make sense to me.

@brettcannon
Copy link
Member

@pradyunsg any reason not to merge this once you have a chance to fix the merge conflicts?

@pradyunsg
Copy link
Member Author

pradyunsg commented Mar 29, 2021

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

@brettcannon
Copy link
Member

We should probably get this merged before we cut a 21.0 release.

@uranusjr
Copy link
Member

uranusjr commented Jul 1, 2021

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)

@pradyunsg
Copy link
Member Author

pradyunsg commented Jul 1, 2021

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?

@brettcannon
Copy link
Member

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

@pradyunsg
Copy link
Member Author

when can we definitely take out these legacy classes?

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.

@di
Copy link
Sponsor Member

di commented Sep 14, 2021

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

@uranusjr
Copy link
Member

Not as far as I’m aware.

prha added a commit to dagster-io/dagster that referenced this pull request Jan 27, 2023
### 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.
gibsondan pushed a commit to dagster-io/dagster that referenced this pull request Jan 27, 2023
### 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.
brusdev added a commit to brusdev/activemq-artemis-broker-image that referenced this pull request May 3, 2023
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
brusdev added a commit to brusdev/activemq-artemis-broker-image that referenced this pull request May 3, 2023
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
brusdev added a commit to brusdev/activemq-artemis-broker-kubernetes-image that referenced this pull request May 3, 2023
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
shailshouryya added a commit to shailshouryya/save-thread-result that referenced this pull request Aug 7, 2023
- 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
moto-timo added a commit to moto-timo/layerindex-web that referenced this pull request Jan 23, 2024
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 <tim.orling@konsulko.com>
heerener added a commit to neuronsimulator/nrn that referenced this pull request Feb 5, 2024
22.0 dropped LegacyVersion: pypa/packaging#407
This makes it so our branches can no longer be parsed as versions
heerener added a commit to neuronsimulator/nrn that referenced this pull request Feb 5, 2024
22.0 dropped LegacyVersion: pypa/packaging#407
This makes it so our branches can no longer be parsed as versions
pramodk pushed a commit to neuronsimulator/nrn that referenced this pull request Feb 5, 2024
* 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
heerener added a commit to neuronsimulator/nrn that referenced this pull request Feb 5, 2024
22.0 dropped LegacyVersion: pypa/packaging#407
This makes it so our branches can no longer be parsed as versions
JCGoran pushed a commit to neuronsimulator/nrn that referenced this pull request Feb 7, 2024
22.0 dropped LegacyVersion: pypa/packaging#407
This makes it so our branches can no longer be parsed as versions
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.

Removal of LegacyVersion and LegacySpecifier
5 participants