-
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
pip install fails when PEP658 metadata is present if the user tries to install using a non-normalized package name #12038
Comments
Hmm, I would have expected pip to always send the wheel url with |
I believe it is. These are the requests I see:
It makes these same requests no matter how I enter the package name, so I believe it is requesting the correct files. I think the problem is that when it downloads and parses the metadata, it compares the name parsed from the metadata file (which is normalized) directly to the name the user entered without normalizing it. |
Perhaps a change like this: --- src/pip/_internal/operations/prepare.py 2023-05-20 03:23:56.269263381 -0500
+++ src/pip/_internal/operations/prepare.py 2023-05-20 03:23:52.665267870 -0500
@@ -410,7 +410,7 @@
# NB: raw_name will fall back to the name from the install requirement if
# the Name: field is not present, but it's noted in the raw_name docstring
# that that should NEVER happen anyway.
- if metadata_dist.raw_name != req.req.name:
+ if metadata_dist.canonical_name != canonicalize_name(req.req.name):
raise MetadataInconsistent(
req, "Name", req.req.name, metadata_dist.raw_name
) Seems to fix it for me but I'm not sure if there are any edge cases. The fact that the comment above the line suggests that |
Ah yes - thanks for the analysis! I'll take a closer look at the code, but IMO your suggested fix looks right, so if there are any additional concerns, I'd want to fix them another way (or maybe just not worry about them unless someone reports them in a real-world situation 🙂) |
I tracked down a comment about name verification from the PR that introduced this code which has some context: #11111 (comment) If I'm understanding correctly, I think they didn't want to use Since the goal is to verify the So perhaps it makes sense to still use --- src/pip/_internal/operations/prepare.py 2023-05-20 03:23:56.269263381 -0500
+++ src/pip/_internal/operations/prepare.py 2023-05-20 03:23:52.665267870 -0500
@@ -410,7 +410,7 @@
# NB: raw_name will fall back to the name from the install requirement if
# the Name: field is not present, but it's noted in the raw_name docstring
# that that should NEVER happen anyway.
- if metadata_dist.raw_name != req.req.name:
+ if canonicalize_name(metadata_dist.raw_name) != canonicalize_name(req.req.name):
raise MetadataInconsistent(
req, "Name", req.req.name, metadata_dist.raw_name
) (Not sure if it's necessary to canonicalize the raw_name or not. In my case it is already canonicalized in the metadata but maybe some wheels have it not canonicalized for whatever reason?) |
Yes, that's precisely the conclusion I'd just come to from looking at the code, so what you found on the tracker makes perfect sense to me. I think it's right to canonicalize explicitly, as the spec doesn't require the name in the metadata file to be normalised, and actually recommends normalising before comparison. Would you be interested in making this fix into a PR? What you posted above plus a short changelog would be sufficient - it would be nice to add a test if you can work out how to trigger the behaviour from a test, but if not, I'd be happy to look into that. Alternatively, I'll make a PR, but I don't want to take the credit if you'd like to do it 🙂 |
One thought - why isn't this reproducible if you use the JSON index from PyPI? That should work the same. If it's not failing in the same way, that suggests the JSON index isn't hitting the PEP 658 fastpath in the first place (because the files will be the same either way). |
Sounds good, I'll try to get a PR up later today. Will also check what's happening with the JSON index, I haven't looked into it all other than noticing that it doesn't seem to be downloading the metadata files in that mode. |
Cool - it would be a shame if the new JSON form of the index is disabling the new separate metadata file 🙁 If fixing that is more complex, though, it can be split into a separate PR if you'd prefer to do that. |
The reason this doesn't happen with the JSON index is because pip is looking for a
I think pypi.org is providing the wrong key name as it should not be prefixed with If I temporarily patch pip to use the (incorrect) key name for the PyPI index, I get a traceback raised:
I think there is a bug on both sides:
If I further patch pip to parse the dict correctly, it reproduces the original error from this issue when using the JSON index:
I will go file separate tickets for the pip and warehouse issues discovered. I wonder if it will be hard for warehouse to fix the key name since I think it would immediately break a lot of existing pip versions if they did due to the parsing bug. |
Here are the 2 bugs:
I will still try to create a PR for this issue now. |
Description
When installing packages from a registry which supports PEP658,
pip install
produces an error if the user tries to install a package using its non-normalized name.For example, testing here with the
fluffy-server
package using an underscore instead of a hyphen:Package name variants like
fluffy_server
,FLUFFY-SERVER
, etc. all fail. Only the normalized namefluffy-server
succeeds.Expected behavior
pip should allow installing using the non-normalized package name as it will be very confusing for users otherwise.
pip version
Tested with 23.1.2 and current
main
Python version
3.11
OS
Linux (debian bullseye)
How to Reproduce
I have created a minimal reproduction PEP503 static file registry here which you can easily test with: https://github.com/chriskuehl/pip-pep658-normalization-bug-repro
python3 -m http.server 7777
pip install -i http://localhost:7777/simple fluffy_server
and observe the error from above.This can also be reproduced using
pypi.org
, however you need to use the PEP503 (HTML/simple/
files) rather than the new JSON format. I could not find a built-in option to ask pip to use the HTML index, but if you comment out these two lines and force pip to usetext/html
, you can see that pip discards the wheel and installs from the sdist instead:Code of Conduct
The text was updated successfully, but these errors were encountered: