Make `--require-virtualenv` output more info #2401

msabramo · 2015-02-05T17:33:52Z

Make --require-virtualenv output more info

if not in a virtualenv and --user wasn't specified.

[marca@marca-mac2 pip]$ pip install fdfdfdfdfdfdffdd  --require-virtualenv
******************************************************
Using pip outside a virtualenv and without --user
is not recommended!

Doing stuff with the system python can break your system.

Did you mean to use a virtualenv or use --user?
******************************************************
[marca@marca-mac2 pip]$ pip install fdfdfdfdfdfdffdd  --require-virtualenv --user
Collecting fdfdfdfdfdfdffdd
  Could not find any downloads that satisfy the requirement fdfdfdfdfdfdffdd
  No distributions at all found for fdfdfdfdfdfdffdd

msabramo · 2015-02-05T17:38:39Z

Note that if folks have some kind of process that relies on being able to pip install to the system Python (I don't know; maybe folks who are using Docker and don't think they need virtualenv though this point is debatable), then they could completely disable this by putting the following in their ~/.pip/pip.conf:

force_use_system_python = true

aconrad · 2015-02-05T17:39:23Z

pip/basecommand.py

@@ -104,6 +114,9 @@ def parse_args(self, args):
        # factored out for testability
        return self.parser.parse_args(args)

+    def running_outside_virtualenv(self):


I think is_in_virtualenv would be a nicer name

Alex, is_in_virtualenv would require negative logic in the if statement below, which is not ideal.

I don't think that's too bad. If we had it as .in_virtualenv() then it would look like:

if not self.in_virtualenv(): ...

I actually don't like function that's have negative logic because if I want to reverse the logic, then I have to read a double-negative logic and my brain starts to melt. I'd rather read positive logic and then just add a not to negate it.

msabramo · 2015-02-05T17:52:55Z

Hmmm, there appears to already be some code to do something like this, but I can't figure out how to use it:

$ ag 'require_venv|running_under_virtualenv' pip
pip/basecommand.py
15:from pip.locations import running_under_virtualenv
268:        if options.require_venv:
270:            if not running_under_virtualenv():

pip/baseparser.py
14:    legacy_config_file, config_basename, running_under_virtualenv,
179:        if running_under_virtualenv():

pip/cmdoptions.py
67:    dest='require_venv',

pip/locations.py
78:def running_under_virtualenv():
99:    if running_under_virtualenv() and os.path.isfile(no_global_file):
145:if running_under_virtualenv():
244:    if running_under_virtualenv():

pip/req/req_install.py
29:    bin_py, running_under_virtualenv, PIP_DELETE_MARKER_FILENAME, bin_user,
785:            if running_under_virtualenv():
941:                elif (running_under_virtualenv()

pip/utils/__init__.py
19:    site_packages, user_site, running_under_virtualenv, virtualenv_no_global,
333:    if not running_under_virtualenv():
445:    if running_under_virtualenv():

pip/utils/outdated.py
15:from pip.locations import USER_CACHE_DIR, running_under_virtualenv
93:    if running_under_virtualenv():

msabramo · 2015-02-05T17:55:30Z

Ah. Here it is:

[marca@marca-mac2 pip]$ pip install fdfdfdfdfdfdffdd  --require-virtualenv
Could not find an activated virtualenv (required).

That said since I've never heard of any user seeing this message and I didn't know about it myself until just now, I would say that perhaps we need something a bit stronger...

aconrad · 2015-02-05T18:00:24Z

I like the general idea. The message could be tweaked to be less verbose somehow.

Instead of exiting the sudo pip install command after this big warning message, you could rather prompt the user with Installing outside a virtualenv, are you sure? [Y/n] -- if called from a script (not a TTY) then pip would go ahead and install the package without showing the prompt. That would keep sudo pip install backwards compatible, not introduce the new --force-use-system-python nor having to let users mess with their pip.conf file, which I assume most people don't have.

msabramo · 2015-02-05T18:05:39Z

@aconrad: I'll probably redo this to be a smaller change to the already existing --require-virtualenv option. See my comment above. What do you think of that?

I do feel like perhaps that needs to be on my default and maybe needs to display a bit more info, because folks who aren't steeped in Python packaging may not be able to make sense of Could not find an activated virtualenv (required)..

xavfernandez · 2015-02-05T18:13:16Z

I also like the idea, but maybe the warning should not appear when performing:

pip install --user <some_package>

which should (hopefully) be the default soon(?). Cf #1668

aconrad · 2015-02-05T18:15:48Z

@msabramo yeah, it would be better if you could leverage the implementation --require-virtualenv. Although, I'd assume that --require-virtualenv aborts the pip install command altogether.

I think another option that could be enabled by default is --warn-no-virtualenv which would produce what I explained above: prompt the user to ensure this is what he/she meant or just skip the question if command is called from a script to preserve backwards compatibility.

aconrad · 2015-02-05T18:21:15Z

pip/basecommand.py

@@ -38,6 +39,7 @@ class Command(object):
    name = None
    usage = None
    hidden = False
+    warn_outside_virtualenv = False


warn_outside_virtualenv could be renamed to warn_no_virtualenv (shorter) and work well with the --warn-no-virtualenv option I suggested in my comment #2401 (comment).

msabramo · 2015-02-05T18:22:26Z

I was thinking we could simply invert --require-virtualenv so that it requires a virtualenv by default and it would fail and the user would specify --no-require-virtualenv on the command-line it the equivalent conf file key or env variable to turn that off.

aconrad · 2015-02-05T18:29:43Z

I'm just worried that existing scripts would break as your suggestion would be backwards incompatible -- it would abort the install by default, correct?

But hey, we could make it a major version bump too (that being said, it didn't prevent pip 6 from being a nightmare with regards to PEP440).

aconrad · 2015-02-05T18:33:52Z

With your suggestion:

$ sudo pip install foobar
Not in a virtualenv, aborting.

$ sudo pip install foobar --force-no-virtualenv
[... installing ...]

aconrad · 2015-02-05T18:35:32Z

I must say that I like it, I just don't know if it's acceptable for legacy scripts.

dstufft · 2015-02-05T18:39:03Z

I'm pretty down on the idea of mandating virtual environments by default. It feels like the wrong solution to the problem of users installing things into their system. I think switching the default to --user is a far better solution.

pfmoore · 2015-02-05T18:51:23Z

I agree with @dstufft, mandating virtual environments is a bad move. For a start, it sort of breaks the whole point of ensurepip (which is that Python ships with pip - what use is that if you can't then use pip?) In particular, on Windows people are expected to use pip to install packages in their main Python (it's not a "system" Python, there's no such thing in Windows terms).

I thought there was another PR around that addressed this issue by actually checking if you were using sudo (checking euid vs uid?) That seems much less prone to disallowing valid use cases, IMO.

msabramo · 2015-02-05T19:32:10Z

Interesting. I actually started with the idea of checking for sudo rather a virtualenv.

msabramo · 2015-02-05T19:35:57Z

I didn't know that --user was going to become the default. I pretty much never use that option; not for any particular reason; it just doesn't seem to be well-known. When does that become the default? In pip 7?

msabramo · 2015-02-05T20:24:58Z

I just simplified this greatly to take advantage of the existing --require-virtualenv option.

Doesn't output the error if --user is specified. This isn't a virtualenv per se, but I think that installing to a user directory also solves the problem that this was trying to solve so it's reasonable to not output an error for this case. This will make it possible for users to put require_virtualenv = true in their ~/.pip/pip.confand still use pip install --user and to not have things break when --user becomes the default behavior.
Explains the problem a bit more. Could be especially helpful if the system administrator has turned on the option but users of the system are not yet familiar with virtualenvs or --user.

Note that --require-virtualenv is still not the default behavior so this shouldn't break existing behavior in scripts and such as @aconrad brought up.

msabramo · 2015-02-05T20:31:46Z

I think this also addresses @pfmoore's comment about Windows, because it's not the default behavior. Presumably, one wouldn't enable this option on a Windows system.

aconrad · 2015-02-05T21:00:25Z

Sounds like --user would be nice if it was the default indeed. That would simply install packages under ~/.local/lib/pythonX.Y/site-packages such that users without a virtualenv and no root access could still install packages in their HOME.

@msabramo so right now with the state of the PR, if I do sudo pip install foo then it would just not be possible at all?

So if --user becomes the default behavior, would it make this PR moot?

msabramo · 2015-02-05T21:03:24Z

@aconrad: Now that I've updated it, sudo pip install foo should work as before. Nothing changes unless you specify --require-virtualenv or use the corresponding conf file or environment variable setting.

aconrad · 2015-02-05T21:19:10Z

@msabramo oh I see, this code is under if options.require_venv:. A whole different PR (I see you updated the PR's title and description as well).

So now that I look at it with a fresh eye, this feels like --user and --require-virtualenv would be mutually exclusive IMO:

$ pip install foobar --require-virtualenv --user

The point of --user is that it doesn't require a virtualenv to install a package, thus making the semantics of --require-virtualenv somewhat conflicting.

msabramo · 2015-02-05T21:26:12Z

Travis fail looks like a network blip. Restart please?

msabramo · 2015-02-05T21:32:35Z

@aconrad Yeah I wouldn't expect a user to use both options on the command-line. The more likely use case is that a user would specify require_virtualenv in the pip conf file to prevent against future accidental pip installing to the system python dir. Particularly valuable on a shared system where a lot of people have sudo (hello monkeytest). This will allow the end users to still do pip install --user which wouldn't work before.

Funny how this PR started out as being something that made pip more restrictive and now it's less restrictive (though that just means the discussion was valuable to steer it in a different direction).

msabramo · 2015-02-05T21:35:12Z

Oh and require_virtualenv is specified with SUPPRESS_HELP, which means it's undocumented when you do --help, which I think is a sign that it was meant to be used mainly in conf files; not on the command-line.

if not in a virtualenv **and** --user wasn't specified. Previously, this would print an error even if --user was specified. I believe the spirit of `--require-virtualenv` is that it was meant to be used as `require_virtualenv = True` in the pip config file and what it really seeks to do is to prevent the user from messing with packages in the system python envvironment. Thus, I'd argue that --user shouldn't cause an error, because it doesn't mess with the system python. With this change, output looks like this: $ python -m pip install --require-virtualenv foo ****************************************************** Using pip outside a virtualenv and without --user is not recommended! Doing stuff with the system python's packages can break your system. Did you mean to use a virtualenv or use --user? ******************************************************

msabramo · 2015-03-05T16:19:24Z

Updated to make the diff a bit easier to understand at a glance.

qwcode · 2015-03-07T23:03:01Z

pip/basecommand.py

-                    'Could not find an activated virtualenv (required).'
-                )
-                sys.exit(VIRTUALENV_NOT_FOUND)
+                if not getattr(options, 'use_user_site'):


this is odd to me. If I've set the option to require a virtualenv, it shouldn't allow a user install? that's redefining the option.

qwcode · 2015-03-07T23:08:05Z

given my comments, and comments from @dstufft and @pfmoore , closing.

#2418 or something similar seems like the path forward to help keep people out of the system python.

msabramo · 2015-03-07T23:16:57Z

Agreed. Actually was surprised to find it wasn't closed earlier, given the other concerns.

aconrad reviewed Feb 5, 2015
View reviewed changes

msabramo force-pushed the warn_when_outside_virtualenv branch from 9ae09c9 to 5bdb2e0 Compare February 5, 2015 20:18

msabramo changed the title ~~Make [un]install warn when outside virtualenv~~ Make --require-virtualenv output more info Feb 5, 2015

msabramo force-pushed the warn_when_outside_virtualenv branch from 5bdb2e0 to 83e9228 Compare February 5, 2015 21:59

msabramo mentioned this pull request Feb 5, 2015

Display warning when 'SUDO_COMMAND' in os.environ #2403

Closed

msabramo force-pushed the warn_when_outside_virtualenv branch from 83e9228 to 2b1c10c Compare February 6, 2015 14:43

dstufft mentioned this pull request Feb 10, 2015

Default to --user #1668

Closed

msabramo force-pushed the warn_when_outside_virtualenv branch from 2b1c10c to ed0b20a Compare March 5, 2015 16:17

msabramo force-pushed the warn_when_outside_virtualenv branch from ed0b20a to 82a2f35 Compare March 5, 2015 16:18

qwcode reviewed Mar 7, 2015
View reviewed changes

qwcode closed this Mar 7, 2015

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 4, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 4, 2019

Make --require-virtualenv output more info #2401

Make --require-virtualenv output more info #2401

Uh oh!

Conversation

msabramo commented Feb 5, 2015

Uh oh!

msabramo commented Feb 5, 2015

Uh oh!

aconrad Feb 5, 2015

Choose a reason for hiding this comment

Uh oh!

sudarkoff Feb 5, 2015

Choose a reason for hiding this comment

Uh oh!

aconrad Feb 5, 2015

Choose a reason for hiding this comment

Uh oh!

msabramo commented Feb 5, 2015

Uh oh!

msabramo commented Feb 5, 2015

Uh oh!

aconrad commented Feb 5, 2015

Uh oh!

msabramo commented Feb 5, 2015

Uh oh!

xavfernandez commented Feb 5, 2015

Uh oh!

aconrad commented Feb 5, 2015

Uh oh!

aconrad Feb 5, 2015

Choose a reason for hiding this comment

Uh oh!

msabramo commented Feb 5, 2015

Uh oh!

aconrad commented Feb 5, 2015

Uh oh!

aconrad commented Feb 5, 2015

Uh oh!

aconrad commented Feb 5, 2015

Uh oh!

dstufft commented Feb 5, 2015

Uh oh!

pfmoore commented Feb 5, 2015

Uh oh!

msabramo commented Feb 5, 2015

Uh oh!

msabramo commented Feb 5, 2015

Uh oh!

msabramo commented Feb 5, 2015

Uh oh!

msabramo commented Feb 5, 2015

Uh oh!

aconrad commented Feb 5, 2015

Uh oh!

msabramo commented Feb 5, 2015

Uh oh!

aconrad commented Feb 5, 2015

Uh oh!

msabramo commented Feb 5, 2015

Uh oh!

msabramo commented Feb 5, 2015

Uh oh!

msabramo commented Feb 5, 2015

Uh oh!

msabramo commented Mar 5, 2015

Uh oh!

qwcode Mar 7, 2015

Choose a reason for hiding this comment

Uh oh!

qwcode commented Mar 7, 2015

Uh oh!

msabramo commented Mar 7, 2015

Uh oh!

Uh oh!

Make `--require-virtualenv` output more info #2401

Make `--require-virtualenv` output more info #2401