Skip to content
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

Merged
merged 5 commits into from
Sep 27, 2021

Conversation

Omegaice
Copy link
Contributor

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

@pradyunsg
Copy link
Member

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.

@Omegaice
Copy link
Contributor Author

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 test_provides_correct_contents to filter directories so that assert got_files == files didn't fail and I also had to edit dist_info_filenames to also filter out directories to make it's test pass correctly.

@@ -139,6 +139,7 @@ def dist_info_filenames(self):
return [
name[len(base) + 1 :]
for name in self._zipfile.namelist()
if name[-1] != "/"
Copy link
Member

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:] != "/")

Copy link
Contributor Author

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.

@Omegaice
Copy link
Contributor Author

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

@pradyunsg
Copy link
Member

Let me know if you feel it is relevant

It's adding additional public API methods on the WheelFile class, which I strongly dislike. Even if those were private though, I don't think there's much value to removing the duplication of a signle conditional that shows up only twice + once in the tests.

@Omegaice
Copy link
Contributor Author

Omegaice commented Sep 27, 2021

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?

@pradyunsg pradyunsg merged commit 8a8e7e3 into pypa:main Sep 27, 2021
@pradyunsg
Copy link
Member

Nope, we're good here!

Thanks a ton for contributing this fix, being patient about this and working collaboratively to iterate on this PR! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zipinfo Directories
5 participants