-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Trace a better error message on installation failure due to invalid .data files in wheels #8656
Trace a better error message on installation failure due to invalid .data files in wheels #8656
Conversation
Previously our wheel installation process allowed wheels which contained non-conforming contents in a contained .data directory. After the refactoring to enable direct-from-wheel installation, pip throws an exception when encountering these wheels, but does not include any helpful information to pinpoint the cause. Now if we encounter such a wheel, we trace an error that includes the name of the requirement we're trying to install, the path to the wheel file, the path we didn't understand, and a hint about what we expect.
3f0dd40
to
517af16
Compare
Are files in the |
I could see it interpreted either way, which means it could probably use a clarifying update. With regards to the pip behavior, our current and past implementations disallowed it in general. Example with pip 20.1:
Then in that directory with a pip 20.1:
When the file happened to be named the same as a scheme key (which is the case in #8654), we silently ignored it. |
cc @dholth for wheel spec inputs. |
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 implementation looks good to me, just a minor comment but I wouldn’t mind if this is merged without it.
Originally we would throw an `AttributeError` if a bad scheme key was used. After refactoring we would throw a `KeyError`, which isn't much better. Now we call out the wheel being processed, scheme key we didn't recognize, and provide a list of the valid scheme keys. This would likely be useful for people developing/testing the wheel.
517af16
to
127c5b0
Compare
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.
LGTM, thank you for addressing this so quickly 👍
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.
LGTM :)
Looks like everyone agrees that this is good to go, so I'll click merge. git will blame Chris if things break anyway. :P |
…-paths Trace a better error message on installation failure due to invalid .data files in wheels
This explicitly handles erroneous files in the
.data
directory of wheels.Before:
After:
While updating this error message, I noticed the very next line was subject to throwing a similarly confusing error. That has also been updated.
Fixes #8654.