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

Show pip version warning at top of command output #9111

Closed
wants to merge 3 commits into from
Closed

Show pip version warning at top of command output #9111

wants to merge 3 commits into from

Conversation

oTree-org
Copy link

No description provided.

@uranusjr
Copy link
Member

uranusjr commented Nov 8, 2020

LGTM (if the linter error is fixed).

Note that this would change the behaviour of pip install -U pip. Previously check was done after installation, but now it wou emit a warning first, and then proceed to upgrade pip to the latest version. There isn’t really a reliable way to check for that (pip can be upgraded/downgraded by another package as a dependency), so this is probably the best we can do.

@pradyunsg
Copy link
Member

TBH, we can probably skip the warning if pip is mentioned as a top-level requirement. That'll cover the pip install -U pip use case, and that's basically the only one that matters TBH.

@pfmoore pfmoore closed this Aug 5, 2021
@pfmoore pfmoore reopened this Aug 5, 2021
Comment on lines +216 to +217
self.handle_pip_version_check(options)

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove whitespace to fix linting:

Suggested change
self.handle_pip_version_check(options)
self.handle_pip_version_check(options)

@@ -213,6 +213,8 @@ def _main(self, args):
"This will become an error in pip 21.0."
)

self.handle_pip_version_check(options)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... I'm wary of doing it like this -- if this consistently fails or raises an error, then the user would basically not be able to use pip in any way.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. If (for example) a user has no internet, they should still be able to use pip offline. A network error in the check shouldn't kill pip.

We should probably:

  1. Catch and suppress any errors from this check.
  2. Add tests to ensure that we did so successfully.

Maybe the contract for handle_pip_version_check should simply say that it will never generate an exception? I've been playing with Rust recently, so my brain translates this as "the function should just not return a Result" but it's harder to enforce such constraints in Python 🙁

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I'm also not sure that this is the correct thing to do TBH. The message being at the end of the output is a good thing when you have an install run that spewed out a lot of data.

Copy link
Member

Choose a reason for hiding this comment

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

TBH, I don't have a strong opinion about this either way. I suspect that whatever we do with the message, there will always be a certain proportion of people who hate it, so I don't think it's worth putting a lot of effort into improving it. Ultimately, it does the job of making people aware of the need to keep pip updated, and that's the key thing for me.

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Sep 21, 2021
@pradyunsg
Copy link
Member

Going to go ahead and close this, since (1) it has merge conflicts that haven't been resolved in a while and (2) there doesn't seem to be much interest in this idea.

That said, thanks @oTree-org for filing this PR! ^>^

@pradyunsg pradyunsg closed this Sep 30, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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