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

Correctly normalize package names. Fixes #2956 and #3002 #3008

Closed
wants to merge 2 commits into from
Closed

Correctly normalize package names. Fixes #2956 and #3002 #3008

wants to merge 2 commits into from

Conversation

orf
Copy link

@orf orf commented Oct 11, 2018

Fixes #2956 and #3002

Copy link
Member

@techalchemy techalchemy left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for identifying this (we will need a patch for it, but this is the last bit of the vendoring process that is somewhat involved, so I can take care of it and/or automate it if we do merge it)

Overall I'm not too sure if we do want to go this route, patching pip directly to fix this. I am reasonably confident this is only working incorrectly for the index, so this is probably the relevant patch. I will want to solicit some input before making a call on this one

@@ -766,7 +766,7 @@ def egg_info_matches(
return full_match[full_match.index('-'):]
name = match.group(0).lower()
# To match the "safe" name that pkg_resources creates:
name = name.replace('_', '-')
name = name.replace('_', '-').replace('.', '-')
Copy link
Member

Choose a reason for hiding this comment

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

^_^ I am actually tempted to do this, but I have a preference for not patching pip whenever possible.

I'm likely going to hold off on accepting this for the next release to see about getting feedback from the pip maintainers when I have a bit more clarity on all of the places we are currently working around this problem, which I think is pretty numerous by now...

Copy link
Author

Choose a reason for hiding this comment

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

It's up to you if you accept it, and I understand if you do not, but this is currently breaking our (am I am sure others) workflow entirely. I can't see a workaround apart from downgrading

Copy link
Member

Choose a reason for hiding this comment

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

@orf, apart from the issues mentioned and the fixes for those specifically (which are in master), can you describe what is continuing to cause you problems? I.e. with the current master branch, how do I see a problem?

Copy link
Author

Choose a reason for hiding this comment

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

I will get back to you here tomorrow morning, after I try installing the specific pipfile I have with the current master

Copy link
Member

Choose a reason for hiding this comment

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

ah, I see the issue in question. This should be resolved by #3007

Copy link
Author

Choose a reason for hiding this comment

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

Awesome! Thanks for your continued work on pipenv, and I'm sorry for the volume of issues!

I will confirm tomorrow that this is fixing it, or though I cannot see any code that would after my brief lookm

@techalchemy
Copy link
Member

/cc @pradyunsg, @cjerdonek (don't want to bother paul since I know he is taking time off)

tl;dr package name normalization isn't fully pep 503 compliant when being handed off to the index lookup in pip, we have a bunch of workarounds right now but this PR is proposing a patch to our vendored pip version.

Any thoughts about whether this is intentional or an oversight? We can push this upstream if desired

@cjerdonek
Copy link
Member

@techalchemy I don't know much about this aspect of pip. Note that @uranusjr just opened this PR which touches this very code: pypa/pip#5875
Does that PR address the issue being raised here?

@techalchemy
Copy link
Member

techalchemy commented Oct 11, 2018

We don't even communicate that well when we are co-maintainers of the same projects

I don't think that one will fix this issue, but it is another case where this code is in disagreement to be sure

Just read the full PR, I think he does capture this, he probably already saw / encountered it and just upstreamed it

@cjerdonek
Copy link
Member

Yeah, I figured that might have been happening so wanted to be sure you were at least alerted here!

@uranusjr
Copy link
Member

To make sure, is pypa/pip#5875 required to fix this? I was assuming it is worked around by 59dd901#diff-2c68dad4d149b80846780635fda149f2

@techalchemy
Copy link
Member

@uranusjr nope, as far as I understand we don't need that change for this to work

@techalchemy
Copy link
Member

closing for now since we will catch this upstream in the next release of pip, thanks for the PR and thanks for the input all

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.

4 participants