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 #5889 which occurs when comes_from.link is None #6082

Closed

Conversation

ircwaves
Copy link

@ircwaves ircwaves commented Dec 13, 2018

fixes issue #5889, which blocks a private-package use case of installing a package C, that requires package B via a direct URL requirement, which requires A via direct URL requirement (a la pep508).

@benoit-pierre said:

  • allow calling it with comes_from=None (easier for testing)
  • handle the case where comes_from.link is None (which can happen
    if a package using a PEP 508 URL requirement is already installed)

On benoit-pierre's PR #5893, @cjerdonek suggested that a slimmer PR might be better, so here it is. This may be a total faux pas, but I've cherry-picked the commit he made, as it fixes issue #5889.

I see that on the PR #5893 @cjerdonek asked for a unit test, but I'm not familiar enough with pip internals to set up that unit test. The test that benoit-pierre wrote should ensure that there isn't a regression that blocks this use-case again.

@ircwaves
Copy link
Author

OK. Now I see that the test has issues.

@benoit-pierre
Copy link
Member

Yes, you can't use file:///path/to/package because of a packaging bug (pypa/packaging#115), and the work around using file://localhost/path/to/package does not work on Linux because of a pip bug (see #5892)... Either wait for a new packaging version to be released and vendored, or work on getting #5892 merged, or drop the functional test and add a unit test.

@xavfernandez
Copy link
Member

xavfernandez commented Feb 11, 2019

FWIW, pypa/packaging#115, was merged and packaging 19 is now vendored in pip.

@ircwaves ircwaves force-pushed the 5889-check-for-link-before-access branch from 9f94750 to 590fdba Compare February 11, 2019 21:08
* allow calling it with `comes_from=None` (easier for testing)
* handle the case where `comes_from.link is None` (which can happen
  if a package using a PEP 508 URL requirement is already installed)
@ircwaves ircwaves force-pushed the 5889-check-for-link-before-access branch from 590fdba to b422cae Compare February 11, 2019 21:22
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Mar 20, 2019
@cjerdonek
Copy link
Member

@xavfernandez Can this be closed because of PR #6336 just merged?

@xavfernandez
Copy link
Member

Indeed

@xavfernandez
Copy link
Member

Thanks @ircwaves for the initial PR though

@ircwaves
Copy link
Author

ircwaves commented Mar 21, 2019

@xavfernandez -- I'll pass those thanks on back to @benoit-pierre, @cjerdonek, and @aloosley. I just wish my bandwidth had allowed me to get more involved and push the fix through. Thanks to all for their efforts, of any measure.

@lock
Copy link

lock bot commented May 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation needs rebase or merge PR has conflicts with current master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants