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.

Sign up for GitHub

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

Display message on Windows when modifying pip #4490

Merged
merged 2 commits into from
May 19, 2017
Merged

Display message on Windows when modifying pip #4490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e41bad7
Display a message on Windows if pip is being modified.
pradyunsg May 17, 2017
053fa2f
:newspaper:
pradyunsg May 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/4490.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Display a message to run the right command for modifying pip on Windows
17 changes: 17 additions & 0 deletions pip/basecommand.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,23 @@ def populate_requirement_set(requirement_set, args, options, finder,
'You must give at least one requirement to %(name)s '
'(see "pip help %(name)s")' % opts)

# On Windows, any operation modifying pip should be run as:
# python -m pip ...
# See https://github.com/pypa/pip/issues/1299 for more discussion
should_show_use_python_msg = (
sys.platform == 'win32' and
Copy link
Contributor

Choose a reason for hiding this comment

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

This checks fails at least on Jython and IronPython. I've used os.sep != '/' which is hackish but works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for spotting that. I don't have much experience with these.

Would os.name == 'nt' work on those platforms?

Copy link
Contributor

@pekkaklarck pekkaklarck May 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Member Author

Choose a reason for hiding this comment

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

On it!

Copy link

Choose a reason for hiding this comment

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

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.

requirement_set.has_requirement('pip') and
"pip" in os.path.basename(sys.argv[0])
)
if should_show_use_python_msg:
new_command = [
sys.executable, "-m", "pip"
] + sys.argv[1:]
raise CommandError(
'To modify pip, please run the following command:\n{}'
.format(" ".join(new_command))
)
Copy link
Member

Choose a reason for hiding this comment

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

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...)

Copy link
Member Author

@pradyunsg pradyunsg May 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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! ^.^


def _build_package_finder(self, options, session,
platform=None, python_versions=None,
abi=None, implementation=None):
Expand Down