Update non PEP 440 wheel filename deprecation notice #12939
Conversation
notatallshaw
force-pushed
the
update-non-PEP-440-wheel-filename-deprecation-notice
branch
from
686293f to
fbe7924
Compare
|
CI is green, ready to review/merge. I would say this one is pretty important to land for 24.3 as this message was a lot nosier than expected and this removes false positives. |
src/pip/_internal/models/wheel.py
Outdated
| _version = _version.replace("_", "-") | ||
|
|
||
| try: | ||
| parse_wheel_filename(filename) | ||
| except (PackagingInvalidWheelName, InvalidVersion): |
There was a problem hiding this comment.
By just reading the code here, it's not obvious that all wheel filename parsing errors here are guaranteed to relate to an invalid version part.
Would it make sense to catch the two exception separately with a slightly different deprecation message?
Or is there a guarantee here that all exceptions are due to the version part? In that case a little comment explaining that might be useful.
There was a problem hiding this comment.
It's not guaranteed but it would have to occur under the following series of events:
- The wheel filename passes the existing regex
- There is a
_that is valid in what the regex parses as "version" but is not actually part of the version - There is a different error in the filename that packaging catches
This would therefore have to be a currently unknown error that wheel filenames can have that packaging does not support but pip's regex was passing on anyway.
The reason I am catching both exceptions is because I did the same in #12327 and I was borrowing that code, but in that PR the code was scanning a directory that could find arbitrary names and I saw both errors in my testing.
Happy to split catching the two different exceptions up and give an appropriate message for PackagingInvalidWheelName .
| f"The wheel filename {filename!r} is not correctly normalised. " | ||
| "Future versions of pip will fail to recognise this wheel. " | ||
| f"and report the error: {e.args[0]}." |
There was a problem hiding this comment.
This reads poorly:
DEPRECATION: The wheel filename 'six-1.16.0_build1-py3-none-any.whl' is not correctly normalised. Future versions of pip will fail to recognise this wheel. and report the error: Invalid wheel filename (invalid version): six-1.16.0_build1-py3-none-any. pip 25.1 will enforce this behaviour change. A possible replacement is rename the wheel to use a correctly normalised name (this may require updating the version in the project metadata). Discussion can be found at https://github.com/pypa/pip/issues/12938
ERROR: Invalid requirement: 'six==1.16.0-build1': Expected end or semicolon (after version specifier)
six==1.16.0-build1
~~~~~~~~~~^
That period seems superfluous? "pip will fail to recognise this wheel. and report the error"
There was a problem hiding this comment.
😞 , yes, I should have done another read over of the error before pushing the commit, I didn't expect it to actually ever trigger though.
Does your example actually cause this error to trigger? i.e. it passes the regex but not the parse_wheel_filename?
I can make yet another follow up to improve the wording.
There was a problem hiding this comment.
I made it up by renaming the random six wheel I have lying around. It should not be considered a real world example :P
It did work in raising this error though.
There was a problem hiding this comment.
I'll make a PR tomorrow aimed to be as small as possible to clean up the wording. Thanks for pointing out.
As requested in #12918 (comment) this creates an issue and points to the deprecation notice to that issue (#12938).
Further, upon testing pip main for a little bit I found several packages that use
_to separate the version and build number, which is valid, but the deprecation was pretty noisey as the current logic can't distinguish this, so I updated the deprecation to not fire unlessparse_wheel_filenameidentified it as invalid.These examples no longer fire the deprecation notice with this PR.