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

Add pip.main as func raising clean error #5254

Closed
wants to merge 1 commit into from

Conversation

sirosen
Copy link
Contributor

@sirosen sirosen commented Apr 16, 2018

This is follow-up/an alternative to #5149

This has upsides and downsides. It improves the errors and guidance for those who invoke pip.main, but it changes the time of failure for some modules attempting to import and use pip.main. Instead of failing at import time, they'll fail when they invoke pip.main().
On the plus side, it means that modules which import pip.main but only use it in some of their functions/classes may remain otherwise functional, so the scope of breakage is reduced.

Modules using pip.main() are going to be broken one way or another, and although ImportErrors will be superior for a number of users, it forbids us from sending good and clear messaging about what to do to remediate.

Advertising pip<10 as "maybe working" is probably a bit controversial, but it's practical/useful info. Whether or not pip takes a hardline stance on this is a matter for discussion/debate.

This has upsides and downsides. It improves the errors and guidance for
those who invoke `pip.main`, but it changes the time of failure for some
modules attempting to import and use `pip.main`. Instead of failing at
import time, they'll fail when they invoke `pip.main()`.
On the plus side, it means that modules which import pip.main but only
use it in some of their functions/classes may remain otherwise
functional, so the scope of breakage is reduced.

Modules using `pip.main()` are going to be broken one way or another,
and although ImportErrors will be superior for a number of users, it
forbids us from sending good and clear messaging about what to do to
remediate.

Advertising pip<10 as "maybe working" is probably a bit controversial,
but it's practical/useful info. Whether or not pip takes a hardline
stance on this is a matter for discussion/debate.
@pfmoore
Copy link
Member

pfmoore commented Apr 16, 2018

I remain -1 on this. There's no evidence that it'll result in any change in user behaviour - the people encountering this error are typically doing things like sudo pip install -U pip which breaks their system. Giving an alternative message that explains how to call pip without using pip.main is no help to them, as it's the distro that need to make that change, and there's no reason to think they aren't doing that, in conjunction with updating their system packages to pip 10.

@sirosen
Copy link
Contributor Author

sirosen commented Apr 16, 2018

The ImportError they'll see without this doesn't help those users either.

This helps package maintainers who have been calling pip.main and need to fix their usage.
Instead of a blunt ImportError, they'll get information on how to fix their packages.

@pfmoore
Copy link
Member

pfmoore commented Apr 16, 2018

Well, they could always read the release notes, and the docs, and the various release emails and announcements, and look at the issue tracker for discussions of similar issues...

@sirosen
Copy link
Contributor Author

sirosen commented Apr 16, 2018

Yes, the information is already out there. Why does that make it bad to present it in a more cogent and direct manner?

@pfmoore
Copy link
Member

pfmoore commented Apr 16, 2018

I didn't say it was bad, just that I'm against doing it. I don't think there's any real benefit, and it's something we'd have to maintain and potentially debate (I already object to the "you may find pip.main works on pip<10" comment, people will read that as "pin to pip<10"). But that's mostly "I'm not going to do anything on this" not "I'm going to block this".

@sirosen
Copy link
Contributor Author

sirosen commented Apr 17, 2018

Something is better than nothing if it can be done without making things worse.
If you're not on the listhosts, there's no warning that packages you're using are about to break, or that your careless use of sudo pip install -U is about to land you in hot water. (Not that there necessarily needs to be. You can't save everyone.)

I already object to the "you may find pip.main works on pip<10" comment, people will read that as "pin to pip<10"

If you're on a distro that doesn't support pip10 yet, and you sudo pip install -U package-with-pip-dependency, that's exactly the information which you need and the correct thing to do until your distro updates.

shrugs I don't have a super-strong commitment to this. I just think having a clear message will save less clue-ful users a lot of headache.

@absognety
Copy link

absognety commented Apr 17, 2018

pip.main() for pip 10 doesn't seem to work seems like the function main in the latest version has been dropped. However we can use subprocess.check_call([]) with similar arguments ex: subprocess.check_call(['install','package_name']) to install a package and also to upgrade a package

@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 eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Apr 20, 2018
@pradyunsg pradyunsg added the S: needs triage Issues/PRs that need to be triaged label May 11, 2018
@pradyunsg
Copy link
Member

pradyunsg commented Jun 6, 2018

I agree with @pfmoore here. I'm also not in favor of this.

Anyways, the ship has sailed on this. I don't see users being helped by this change because nearly all of the reported issues are due to users messing up their OS package manager-managed pip installations.

@pradyunsg
Copy link
Member

The underlying issue here IMO was pip's upgrade being done incorrectly, partly due to pip printing a bad message for users to do the upgrade. We're working on that in #5346.

@pradyunsg
Copy link
Member

Closing since I don't see anything actionable on this PR anymore.

Feel free to comment here if I'm wrong on that. :)

@lock
Copy link

lock bot commented Jun 2, 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 Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
@pradyunsg pradyunsg removed the S: needs triage Issues/PRs that need to be triaged label Feb 21, 2020
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