-
Notifications
You must be signed in to change notification settings - Fork 52
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
Fix for wheels compressed with zip instead of python wheels package #72
Conversation
for more information, see https://pre-commit.ci
Ah fun. Coverage is able to realise now that we never hit this branch in the tests. Would you be willing to add a test for this @Omegaice? If not, I’ll be happy to pick this up instead. |
I just pushed a commit which should have a valid test. It passes locally but I didn't run the full gamut of tests as I don't have all the python versions installed and pre-commit had some issues with something to do with GPG. I had to edit the test in |
src/installer/sources.py
Outdated
@@ -139,6 +139,7 @@ def dist_info_filenames(self): | |||
return [ | |||
name[len(base) + 1 :] | |||
for name in self._zipfile.namelist() | |||
if name[-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.
I wonder if we need to account for an empty string here. (easiest solution would be name[-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.
I don't think this should be necessary because it looks at entries inside of the ZIP file but I will add it for extra safety.
…ase of an empty zip entry filename
I also pushed to another branch some exploration with restructuring to reduce the number of times the directory check is handled. Let me know if you feel it is relevant and I can merge it into this branch. Omegaice@9f4a186 |
It's adding additional public API methods on the |
That was the reason I ended up pushing it to a different branch as I felt it was a bit overbearing for the thing it was trying to improve. Is there anything else that would benefit this PR? |
Nope, we're good here! Thanks a ton for contributing this fix, being patient about this and working collaboratively to iterate on this PR! :) |
When using zip to compress a wheel package it adds entries for the directories themselves which breaks this code because the code expects every entry in the zip file to have an entry in the RECORD file.
Fixes #71