-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Deprecate wheel filenames that are not compliant with PEP 440 #12918
Deprecate wheel filenames that are not compliant with PEP 440 #12918
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question, otherwise looks good to me, thanks!
src/pip/_internal/models/wheel.py
Outdated
f"Wheel filename {filename!r} contains invalid character '_' " | ||
f"in version part {_version!r}." | ||
), | ||
replacement="use '-' instead of '_'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when doing this, the part after _
goes into the build number part instead of the version? So that may not do precisely what's intended? Not sure what a better replacement would be, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct. I don’t think there’s a real replacement per se; the only alternative that makes sense is to not use such a version at all. (Using .post
is a probable solution; build number is another if that works for the use case.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description is wrong, but the remedy is correct:
>>> parse_wheel_filename('simple-0.1_1-py2-none-any.whl')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "python3.11/site-packages/pip/_vendor/packaging/utils.py", line 102, in parse_wheel_filename
version = Version(parts[1])
^^^^^^^^^^^^^^^^^
File "lib/python3.11/site-packages/pip/_vendor/packaging/version.py", line 266, in __init__
raise InvalidVersion(f"Invalid version: '{version}'")
pip._vendor.packaging.version.InvalidVersion: Invalid version: '0.1_1'
>>> parse_wheel_filename('simple-0.1-1-py2-none-any.whl')
('simple', <Version('0.1')>, (1, ''), frozenset({<py2-none-any @ 140419507927680>}))
I will update the description to be _
is deprecated as a seperator between version and the build tag, use -
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current special case, anything after the _
is tacked onto the version. So Wheel("simple-0.1_1-py2-none-any.whl")
would be a version of 0.1-1
. This normalizes to 0.1.post1
, so in this case, it would be more appropriate to say that _
is deprecated as a separator between a release segment and an implicit post release segment. However, taking a look an example from the linked issue in the code, translate_toolkit-1.9.0_pinterest3-py27-none-any.whl
could be using _
as a separator between the version and build tag (although, it is ambiguous as pinterest3
is not a valid build tag).
On the other hand, I scraped the index pages for the top 8000 PyPI packages and could only find 4 ancient wheels that have this quirk in their filename. It may be simply best to suggest using either a post release or build tag, or neither and call it a day.
j2cli-0.3.0_1-py2-none-any.whl (2014-08-14T16:03:35.935783Z)
j2cli-0.3.1_0-py2-none-any.whl (2014-10-08T13:25:50.237961Z)
password_strength-0.0.1_1-py2-none-any.whl (2014-08-14T15:45:44.306377Z)
password_strength-0.0.2_0-py2-none-any.whl (2014-08-14T16:38:50.239827Z)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, technically _
is still allowed in the wheel version as pre or post release separators and packaging will happily parse them:
>>> from packaging.utils import parse_wheel_filename
>>> parse_wheel_filename("six-1.16.0_post1-py3-none-any.whl")
('six', <Version('1.16.0.post1')>, (), frozenset({<py3-none-any @ 129024502605504>}))
So this deprecation warning is technically overly broad, although in fairness, using _
does violate the spirit of the wheel specification as the version should (not a MUST though) be normalized (eliminating the underscore).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the difference between parse_wheel_filename
and Wheel.wheel_file_re
is more complex than I thought. There are two choices as I see it:
- Attempt to morely clearly explain the issue in the description, saying the user might need to update the wheel file
- Attempt to parse first with
parse_wheel_filename
and then fallback toWheel.wheel_file_re
, this will require moving some existing code around and reworking what types they produce, etc.
I'm strongly inclined to go with "1." as it appears that using _
here is very uncommon. But if "2." is prefered I will implement that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer (1). If I understand the problem correctly, we'd be warning the user if pip encounters a wheel with _
in the version part of the filename, saying something like "Wheel filename {filename!r} uses an invalid filename format, as the version part {_version!r} is not correctly normalised, and contains an underscore character. Future versions of pip may fail to recognise this wheel." And the resolution could be "rename the wheel to use a correctly normalised version part (this may require updating the version in the project metadata)".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks all, updated!
Although, it will be funny if no one ever reads the message outside this PR 😉
40d3b80
to
8427ab4
Compare
news/12918.feature.rst
Outdated
@@ -0,0 +1 @@ | |||
Deprecate '_' in version part of a wheel filename, use '-' instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure what this should say (probably a simplified version of the deprecation message), but this should be renamed to 12918.removal.rst
which yes can be used for deprecations as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
8427ab4
to
2c792b6
Compare
Thanks @notatallshaw |
"rename the wheel to use a correctly normalised version part " | ||
"(this may require updating the version in the project metadata)" | ||
), | ||
gone_in="25.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@notatallshaw Could you file an issue for 25.1, to track this removal?
Fixes #12914
It should be noted that because of #12327, which already uses
parse_wheel_filename
instead ofWheel.wheel_file_re
a wheel that uses a_
in the version part of the string will not be found using--find-links=<path-to-dir>
. This was an unintended side effect, but no one has raised an issue about it.Once the depreciation cycle is complete #12917 can land.
I thought about merging this PR and #12917 by first parsing with
parse_wheel_filename
and then falling back toWheel.wheel_file_re
with a depreciation error, but it introduced a lot of additional complexity.I did some reading on the motivation to allow
_
in the wheel version part, and it seemed to be supporting tools which have long since been fixed or are themselves no longer maintained.