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

Match normalized requirement names #3153

Merged

Conversation

ldaniluk
Copy link
Contributor

@ldaniluk ldaniluk commented Mar 6, 2022

Summary of changes

Requirements might be specified using normalized names as described in PEP 503 but working set would not match them to installed packages that are identified by their canonical name. This is the reason of this issue in liccheck.

Pull Request Checklist

@ldaniluk ldaniluk force-pushed the ldaniluk/match-normalized-requirement-names branch from 8b9fd2d to fb39eff Compare March 6, 2022 22:35
@ldaniluk ldaniluk changed the title Ldaniluk/match normalized requirement names Match normalized requirement names Mar 7, 2022
Due to PEP 503 package requirements might be specified using normalized
name which won't be resolved by WorkingSet.

Signed-off-by: Łukasz Daniluk <lukasz.dan@gmail.com>
Signed-off-by: Łukasz Daniluk <lukasz.dan@gmail.com>
@ldaniluk ldaniluk force-pushed the ldaniluk/match-normalized-requirement-names branch from b7e694f to 3669910 Compare March 24, 2022 11:01
@ldaniluk
Copy link
Contributor Author

Rebased changes

@abravalheri
Copy link
Contributor

Thank you very much @ldaniluk for the contribution.

I am postponing my review a little bit because I am currently focusing in preparing the next release, but at first glance it looks great!

This extra time will also allow other maintainers that have more experience in pkg_resources to have a look on the code. I hope you understand.

@Jorricks
Copy link

Nice work @ldaniluk!
I was looking at introducing the same change but I went a different route.
I instead changed the safe_name function call to also change the . to -.
So I was wondering, what is the added benefit of keeping the canonical names in the by_key dict and keeping a separate index for normalised names next to it? Why not update by_key to use normalised names to begin with?

@ldaniluk
Copy link
Contributor Author

@abravalheri - no rush, take your time, I welcome all the comments

@Jorricks - I did not want to introduce a potentially breaking change since by_key seems to already be a part of interface of this class. I might be mistaken on the topic though - it was my first "real" journey into setuptools.
Initially I considered not adding a translation dictionary and going through all registered distributions in case WorkingSet.find failed to match a canonical name version of a particular distribution.
I have decided against it since current approach with find using a by_key dictionary indicated to me that its focus is on speed and iterating over all by_key entries to convert names to find a match (since normalizing name is one-way function and I cannot just make a canonical name out of normalized one) seemed to be contrarian.

@Jorricks
Copy link

Jorricks commented Mar 28, 2022

Yes this sounds like a better way to implement it as this makes it completely backwards compatible.

@abravalheri thanks a lot for taking a look at the PR. I understand this might not be at the top of your list, but please be aware, this is quite an annoying issue for many people using the popular pip-tools package.
They also opened the related issue: #3157

@abravalheri
Copy link
Contributor

abravalheri commented Mar 28, 2022

Hi @Jorricks, sorry for the time it is taking to merge this.

Unfortunately the timing is a bit complicated because some features we were working on since the end of last year were released last week.

In order to make sure we can track down and fix any bugs, I think the best approach here is to give people some extra time to report.

If we mix everything together right now it might make things more difficult for everyone.

@Jorricks
Copy link

Ahh allright, that makes sense. Thanks for the explanation 👍

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.

None yet

3 participants