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

Search wheels for .dist-info directories #137

Merged
merged 5 commits into from
Dec 13, 2022

Conversation

stefanor
Copy link
Contributor

Some wheels don't use normalized names for their .dist-info directories, so search the wheel for them.

Fixes: #134

@ArmaanT
Copy link

ArmaanT commented Sep 14, 2022

Is there anything I can do to help get this merged? I've been running into this problem when using bazel to install wheels with capitalization issues.

@stefanor stefanor changed the title Search weels for .dist-info directories Search wheels for .dist-info directories Oct 16, 2022
@stefanor
Copy link
Contributor Author

stefanor commented Dec 5, 2022

Ping?

pradyunsg
pradyunsg previously approved these changes Dec 5, 2022
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with one non-blocking suggestion.

Lemme know if you'd prefer that I merge without that change. :)

@@ -93,6 +93,14 @@ def parse_metadata_file(contents: str) -> Message:
return feed_parser.close()


def normalize_distribution_name(name: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... this might be easier to parse for folks if this has the same name as the packaging function.

Suggested change
def normalize_distribution_name(name: str) -> str:
def canonicalize_name(name: str) -> str:

Comment on lines 143 to 144
norm_di_dname = normalize_distribution_name(di_dname)
norm_file_dname = normalize_distribution_name(self.distribution)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
norm_di_dname = normalize_distribution_name(di_dname)
norm_file_dname = normalize_distribution_name(self.distribution)
norm_di_dname = canonicalize_name(di_dname)
norm_file_dname = canonicalize_name(self.distribution)

@pradyunsg
Copy link
Member

TestWheelFile.test_finds_dist_info is failing on 3.7.

@pradyunsg pradyunsg dismissed their stale review December 7, 2022 01:43

This PR needs fixes for the failure identified in CI.

Some wheels don't use canonicalized names for their .dist-info
directories, so search the wheel for them.

Fixes: pypa#134
At least, once they are both normalized.
@stefanor
Copy link
Contributor Author

Applied your name suggestion, and rebased on top of master.

TestWheelFile.test_finds_dist_info is failing on 3.7.

Added a workaround for that in a separate commit, that's easy to revert later.

@stefanor stefanor requested a review from pradyunsg December 12, 2022 20:59
@pradyunsg pradyunsg changed the title Search wheels for .dist-info directories Search wheels for .dist-info directories Dec 13, 2022
@pradyunsg pradyunsg merged commit 525c09f into pypa:main Dec 13, 2022
@pradyunsg
Copy link
Member

Thanks @stefanor!

PS: I've filed #157 for a few follow up changes that I'd like to make. :)

@stefanor stefanor deleted the search-distinfo branch December 13, 2022 18:28
pradyunsg pushed a commit that referenced this pull request Mar 2, 2023
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.

Can't install wheels with non-normalized names
3 participants