Display message on Windows when modifying pip #4490
Conversation
pip/basecommand.py
Outdated
| # python -m pip ... | ||
| # See https://github.com/pypa/pip/issues/1299 for more discussion | ||
| should_show_use_python_msg = ( | ||
| os.name == 'nt' and |
There was a problem hiding this comment.
use sys.platform == 'win32' instead. Much cleaner and catches all Windows targets as well.
ea0ba0e to
9021c12
Compare
|
/request-review @dstufft @xavfernandez @pfmoore |
|
I'm gonna rebase to trigger news-file/pr again. |
If the user run a pip modifying command without using the python -m, pip now displays an error requesting the user to use python -m method of invoking pip.
059bb71 to
053fa2f
Compare
| raise CommandError( | ||
| 'To modify pip, please run the following command:\n{}' | ||
| .format(" ".join(new_command)) | ||
| ) |
There was a problem hiding this comment.
Would this be better as a warning rather than an error? I can't immediately think of an existing scenario where the test should_show_use_python_msg would give a false positive, but I'd rather not block users from proceeding if they know what they are doing is OK.
(Hypothetical example, if there's a 3rd party tool called upgrade-pip.exe that uses the pip.main entry point to upgrade pip, it would be caught by this check even though it's perfectly OK. That's arguably OK as we don't support using pip's API directly, but it's still a bit unfriendly...)
There was a problem hiding this comment.
Would this be better as a warning rather than an error?
The whole point is to not proceed with a command that might modify the pip package. Otherwise, it still shows an unfriendly traceback which would bury this warning.
That's arguably OK as we don't support using pip's API directly, but it's still a bit unfriendly...
I do think that one shouldn't go around breaking existing workflows in general. In this case though, I'd argue that the gains from breaking such tools (that already not supported) in exchange for a much better UX on running pip install -U pip on Windows is worth it.
There was a problem hiding this comment.
OK, on thinking about it, I'm OK with this approach. If it causes issues for people, we can always improve the check. Or add a --do-it-anyway flag if the check significantly messes up people's workflows in practice.
There was a problem hiding this comment.
If it causes issues for people, we can always improve the check. Or add a --do-it-anyway flag if the check significantly messes up people's workflows in practice.
Yep! ^.^
| # python -m pip ... | ||
| # See https://github.com/pypa/pip/issues/1299 for more discussion | ||
| should_show_use_python_msg = ( | ||
| sys.platform == 'win32' and |
There was a problem hiding this comment.
This checks fails at least on Jython and IronPython. I've used os.sep != '/' which is hackish but works.
There was a problem hiding this comment.
Thanks for spotting that. I don't have much experience with these.
Would os.name == 'nt' work on those platforms?
There was a problem hiding this comment.
Unfortunately os.name won't work with Jython where it's java regardless the OS. They do have os._name with the same semantics as os.name on CPython, so something like getattr(os, '_name', os.name) could work.
There was a problem hiding this comment.
@pekkaklarck It seems Jython has sys.platform attribute - http://www.jython.org/docs/library/sys.html, IronPython says "IronPython is a implementation of Python 2.7, any Python documentation is useful when using IronPython" which leads me to believe the same would work there too.
So, now, I'm confused about whether this works or not.
It's likely that one of the core developers has a better idea of what the scene with these other implementations is than I do. I'll play it safe and wait for their comment. :)
There was a problem hiding this comment.
Both Jython and IronPython have sys.platform attribute, but its value is different to CPython. On Jython it's something like 'java1.8.0_131' and on IronPython 'cli'. These values don't change depending on the OS.
I agree this is a topic where comments from core developers would be good. Preventing changes to pip breaking it on alternative Python implementations would probably also require some collaboration with the developers of these implementations.
There was a problem hiding this comment.
There's pip.compat.WINDOWS which works (according to the comment next to the definition) for cpython and ironpython. I don't know why it doesn't work for Jython (I suspect it means that Jython support might be broken in other places...) Also, I don't know if it would show Cygwin as a Windows system (and I don't even know if it should - Cygwin is a weird mixture of Unix and Windows).
But using pip.compat.WINDOWS for the check is probably the correct thing to do, and separate issues should be raised if that symbol isn't giving the right result on Jython or any other systems...
There was a problem hiding this comment.
Jython trunk, which will be the 2.7.1 release:
>>> import os, platform
>>> platform.python_implementation()
'Jython'
On OS X:
>>> platform.java_ver()
('1.8.0_112', 'Oracle Corporation', ('Java HotSpot(TM) 64-Bit Server VM', '25.112-b16', 'Oracle Corporation'), ('Mac OS X', '10.12.4', 'x86_64'))
>>> os.name
'java ( ==posix for targets )'
>>> os.name == 'java'
True
>>> os._name
'posix'
On Windows:
>>> platform.python_implementation( )
'Jython'
>>> os.name
'java ( ==nt for targets )'
>>> os._name
'nt'
On Ubuntu 16.04:
>>> os.name
'java ( ==posix for targets )'
>>> os._name
'posix'
>>> platform.python_implementation()
'Jython'
>>> platform.java_ver()
('1.7.0_80', 'Oracle Corporation', ('Java HotSpot(TM) 64-Bit Server VM', '24.80-b11', 'Oracle Corporation'), ('Linux', '4.4.0-66-generic', 'amd64'))
We can stop here, and just suggest some combination of the data above to determine the correct functionality is the right way to do this.
But you are a Python enthusiast, so it's worth pointing out that os.name has grown some functionality with Jython 2.7.1, as described in http://bugs.jython.org/issue2557 - this is the targets functionality, and it's why os.name == 'java' is True above, despite the __repr__ being 'java ( ==posix for targets )'. This is implemented by providing a specific target string implementation in Jython. (Fortunately adding new builtin types in Jython is rather cheap.)
>>> type(os.name)
<type 'shadowstr'>
You might think this would allow for immediate comparison of os.name == 'posix', but the Python ecosystem is just too rich in its complexification for something so simple like that to work; in this case I'm trying this out on OSX:
>>> os.name == 'posix'
False
Instead, it's necessary to register such interest, via os.name.addtarget('org\.python.*'), which is global; or via a scheme that enables for this registration to be done from a distance. See the above Jython bug for more details.
|
Won't this be something that should have a changelog entry? Edit: Nvm. I missed the unlabeling on by mobile screen. |
Workaround. Resolves #1299.