Bug: parse_wheel_filename permits non-normalized versions.
#873
Comments
|
Does packaging have any sort of deprecation process for this sort of change? i.e. a public function that throws an error where it didn't used to. Pip is migrating to use I'm still learning how pip migrates to new packaging versions, I guess we could patch packaging to throw a warning instead of an exception for an appropriate deprecation period. |
|
@notatallshaw You may be interested in this branch I am working on to provide a better API here around creating/parsing filenames, including "strict" and "non-strict" parsing: main...di:packaging:filename-api From a cursory glance, it appears that most build backends are producing wheels with normalized filenames, so I wouldn't expect this to be too disruptive, but I could also be convinced to leave |
We can raise a warning for a while if an analysis of PyPI suggests it would be disruptive. |
I don't want to hold up packaging if this is a clear win for most use cases. But a bit of background: Currently pip is using a custom regex to parse wheel filenames: https://github.com/pypa/pip/blob/25.0.1/src/pip/_internal/models/wheel.py#L21 There was a use case pip explicitly supported where These two use cases suggest the two scenarios are users hand rolling their own wheel filenames on private indexes / filesystems and perhaps an edge case in In general when pip is consuming wheel filenames I would rather it be non-strict in cases where the filename can be unambiguously normalized. But I also suppose we could lengthen the depreciation cycle and flag all issues to users (outside the explicitly supported |
Taking a look at all filenames uploaded in the last month: COPY (
SELECT
fe.time AS timestamp,
fe.additional->>'filename' AS filename
FROM
file_events fe
WHERE
fe.time >= NOW() - INTERVAL '1 month'
AND fe.tag = 'file:add'
AND fe.additional->>'filename' LIKE '%.whl'
ORDER BY fe.time
) TO STDOUT WITH (FORMAT CSV, HEADER);Analyzing with: import csv
from packaging.utils import InvalidWheelFilename, parse_wheel_filename
total_filenames = 0
exception_count = 0
with open("out.csv") as file:
reader = csv.DictReader(file) # Assumes CSV has a header
for row in reader:
total_filenames += 1
filename = row["filename"]
try:
parse_wheel_filename(filename)
except InvalidWheelFilename:
exception_count += 1
percentage_exceptions = (exception_count / total_filenames) * 100
print(f"Total filenames processed: {total_filenames}")
print(f"Total exceptions: {exception_count}")
print(f"Percentage of exceptions: {percentage_exceptions:.2f}%")With this branch, we get: The invalid filenames in question are: So it's really only two projects that are producing wheels with non-normalized versions, not 24 projects. I can expand the analysis to a larger timeframe if anyone would like? |
|
Also, seems reasonable to add a |
While helpful information, my experience supporting pip is that it's users who are not uploading to PyPI that are caught by changes to more strictly comply with the spec.
I think this would make it easier, and I definitely like the look of the filename API you linked earlier. All that said, please don't consider it a show stopper, looking at all this code has made me think I need to re-review this and check additional scenarios before migrating to |
If the migration hasn't started yet, I would recommend waiting until we can get a better API for parsing filenames into this library. Note that |
We started the depreciation in pip 24.3 due to be complete in pip 25.1 (the next release), I'll discuss with the other pip maintainers, thanks.
I've not looked at sdists in general yet, that seemed like a much bigger can of worms. The only place I think we're using |
What API are you thinking of? |
The one I mentioned here:
|
|
I'm also OK waiting on that (or something like that) to land instead of merging #874, we can add this check to PyPI in the interim instead. |
FYI that won't work as written as it doesn't handle compressed wheel tags (it's why https://packaging.pypa.io/en/stable/utils.html#packaging.utils.parse_wheel_filename returns a set of tags instead of a single one). |
|
FYI I changed the approach to migrate to this function in pip: pypa/pip#13229
Rereading the spec I do see that it says names and versions of binary distributions should be normalized. Because pip now has a fallback we can extend the deprecation period if this function becomes strict about this if this isn't released before the next pip release. |
Ack, will take that into account, it's still a WIP as I'm hoping to integrate it into PyPI first as a proof-of concept, but I'm currently blocked on other things that haven't landed yet. |
|
I see @di's PR for setuptools to produce normalized wheel file names just released: pypa/setuptools#4766 I'd assumed that build tooling had largely migrated to using normalized names, this broke a bunch of tests in pip's CI, I'm sure in many private repos they will not be strictly following the spec on name normalization. It's up to packaging what to do with this function, but from pip's perspective this makes me nervous about being strict about name normalization. If packaging becomes strict here, with no non-strict option, I think pip should hold off on migrating for several years. |
|
I agree. I'm closing #874 for now in favor of a new API here that supports both strict and non-strict parsing for a gentler migration. |
|
Just a quick update to this, as I had conversations about this change in other settings, and it was pointed out to me the spec does call out tools that consume wheels:
So any update or replacement to |
https://packaging.python.org/en/latest/specifications/binary-distribution-format/ says:
Currently,
parse_wheel_filenamewill raiseInvalidWheelFilenamefor some invalid filenames, but https://packaging.python.org/en/latest/specifications/version-specifiers/#normalization has a long list of version normalizations thatparse_wheel_filenamedoes not enforce:The text was updated successfully, but these errors were encountered: