From 3b70f493c41552ee628c7322640600ad469b7502 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Gia=20Phong?= Date: Fri, 6 Mar 2020 22:54:15 +0700 Subject: [PATCH 1/8] Fail early when install path is not writable At the same time install location compatibility is checked. --- news/6762.bugfix | 2 + src/pip/_internal/commands/install.py | 106 +++++++++++++------------- 2 files changed, 57 insertions(+), 51 deletions(-) create mode 100644 news/6762.bugfix diff --git a/news/6762.bugfix b/news/6762.bugfix new file mode 100644 index 00000000000..a980d99bc97 --- /dev/null +++ b/news/6762.bugfix @@ -0,0 +1,2 @@ +Fail early when install path is not writable. At the same time, +install location compatibility is checked. diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 8c2c32fd43f..978b824e8d1 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -230,9 +230,6 @@ def add_options(self): @with_cleanup def run(self, options, args): # type: (Values, List[str]) -> int - if options.use_user_site and options.target_dir is not None: - raise CommandError("Can not combine '--user' and '--target'") - cmdoptions.check_install_build_global(options) upgrade_strategy = "to-satisfy-only" if options.upgrade: @@ -256,13 +253,6 @@ def run(self, options, args): if options.target_dir: options.ignore_installed = True options.target_dir = os.path.abspath(options.target_dir) - if (os.path.exists(options.target_dir) and not - os.path.isdir(options.target_dir)): - raise CommandError( - "Target path exists but is not a directory, will not " - "continue." - ) - # Create a target directory for using with the target option target_temp_dir = TempDirectory(kind="target") target_temp_dir_path = target_temp_dir.path @@ -633,56 +623,70 @@ def decide_user_install( isolated_mode=False, # type: bool ): # type: (...) -> bool - """Determine whether to do a user install based on the input options. + """Determine whether to do a user install. - If use_user_site is False, no additional checks are done. - If use_user_site is True, it is checked for compatibility with other - options. - If use_user_site is None, the default behaviour depends on the environment, - which is provided by the other arguments. - """ - # In some cases (config from tox), use_user_site can be set to an integer - # rather than a bool, which 'use_user_site is False' wouldn't catch. - if (use_user_site is not None) and (not use_user_site): - logger.debug("Non-user install by explicit request") - return False + If use_user_site is None, the behavior is decided upon other + arguments and the environment. - if use_user_site: - if prefix_path: - raise CommandError( - "Can not combine '--user' and '--prefix' as they imply " - "different installation locations" - ) - if virtualenv_no_global(): + Compatibility of installation location options and the environment + is also asserted in this function. + """ + # Check for incompatible options + locations = {"--target": target_dir, "--user": use_user_site, + "--root": root_path, "--prefix": prefix_path} + destinations = [k for k, v in locations.items() if v] + if len(destinations) > 1: + last, rest = destinations[-1], ", ".join(destinations[:-1]) + raise CommandError("Different installation locations are implied by " + "{} and {}.".format(rest, last)) + + if target_dir: + if os.path.exists(target_dir) and not os.path.isdir(target_dir): + raise InstallationError("Target path exists but is not " + "a directory: {}".format(target_dir)) + if not test_writable_dir(target_dir): raise InstallationError( - "Can not perform a '--user' install. User site-packages " - "are not visible in this virtualenv." - ) - logger.debug("User install by explicit request") - return True - - # If we are here, user installs have not been explicitly requested/avoided - assert use_user_site is None - - # user install incompatible with --prefix/--target - if prefix_path or target_dir: - logger.debug("Non-user install due to --prefix or --target option") + "Cannot write to target directory: {}".format(target_dir)) + logger.debug("Non-user install due to specified target directory") return False - - # If user installs are not enabled, choose a non-user install - if not site.ENABLE_USER_SITE: - logger.debug("Non-user install because user site-packages disabled") + if prefix_path: + if not test_writable_dir(prefix_path): + raise InstallationError( + "Cannot write to prefix path: {}".format(prefix_path)) + logger.debug("Non-user install due to specified prefix path") return False - # If we have permission for a non-user install, do that, - # otherwise do a user install. - if site_packages_writable(root=root_path, isolated=isolated_mode): + if use_user_site is not None: + logger.debug("{} install by explicit request".format( + "User" if use_user_site else "Non-user")) + use_user_site = bool(use_user_site) + elif site.ENABLE_USER_SITE is False: # This never seems to be reached. + logger.debug("Non-user install due to disabled user site-packages by " + "user request (with 'python -s' or PYTHONNOUSERSITE)") + use_user_site = False + elif root_path: + logger.debug( + "Non-user install since alternate root directory is specified") + use_user_site = False + elif site_packages_writable(root=root_path, isolated=isolated_mode): logger.debug("Non-user install because site-packages writeable") return False + else: + logger.info("Defaulting to user installation because " + "normal site-packages is not writeable") + use_user_site = True - logger.info("Defaulting to user installation because normal site-packages " - "is not writeable") - return True + if use_user_site: + if site.ENABLE_USER_SITE is None: + raise InstallationError("User site-packages are disabled for " + "security reasons or by an administrator.") + if virtualenv_no_global(): + raise InstallationError("User site-packages are not visible " + "in this virtualenv.") + else: + if not site_packages_writable(root=root_path, isolated=isolated_mode): + raise InstallationError("Cannot write to global site-packages.") + return use_user_site def reject_location_related_install_options(requirements, options): From f6dce7e8626852a473a906bce4489be7aaa4f58b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Gia=20Phong?= Date: Sat, 7 Mar 2020 15:53:15 +0700 Subject: [PATCH 2/8] Add unit tests for user install decision Functional tests for conflict install locations are removed in favor of the unittest which handle more cases. --- src/pip/_internal/commands/install.py | 28 ++++--- tests/functional/test_install.py | 31 ------- tests/unit/test_command_install.py | 111 +++++++++++++++++++------- 3 files changed, 98 insertions(+), 72 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 978b824e8d1..b734e5b819c 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -616,7 +616,7 @@ def site_packages_writable(root, isolated): def decide_user_install( - use_user_site, # type: Optional[bool] + use_user_site=None, # type: Optional[bool] prefix_path=None, # type: Optional[str] target_dir=None, # type: Optional[str] root_path=None, # type: Optional[str] @@ -657,12 +657,19 @@ def decide_user_install( return False if use_user_site is not None: + use_user_site = bool(use_user_site) logger.debug("{} install by explicit request".format( "User" if use_user_site else "Non-user")) - use_user_site = bool(use_user_site) - elif site.ENABLE_USER_SITE is False: # This never seems to be reached. - logger.debug("Non-user install due to disabled user site-packages by " - "user request (with 'python -s' or PYTHONNOUSERSITE)") + if use_user_site is True and site.ENABLE_USER_SITE is None: + raise InstallationError("User site-packages are disabled for " + "security reasons or by an administrator.") + elif site.ENABLE_USER_SITE is None: + logger.debug("Non-user install since user site-packages are disabled " + "for security reasons or by an administrator.") + use_user_site = False + elif site.ENABLE_USER_SITE is False: + logger.debug("Non-user install since user site-packages are disabled " + "by user request (with 'python -s' or PYTHONNOUSERSITE)") use_user_site = False elif root_path: logger.debug( @@ -676,14 +683,13 @@ def decide_user_install( "normal site-packages is not writeable") use_user_site = True - if use_user_site: - if site.ENABLE_USER_SITE is None: - raise InstallationError("User site-packages are disabled for " - "security reasons or by an administrator.") + if use_user_site is True: if virtualenv_no_global(): - raise InstallationError("User site-packages are not visible " - "in this virtualenv.") + raise InstallationError( + "Can not perform a '--user' install. " + "User site-packages are not visible in this virtualenv.") else: + assert use_user_site is False if not site_packages_writable(root=root_path, isolated=isolated_mode): raise InstallationError("Cannot write to global site-packages.") return use_user_site diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index 2185251d290..90934a6e018 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -887,22 +887,6 @@ def test_install_package_with_target(script, with_wheel): result.did_update(singlemodule_py) -@pytest.mark.parametrize("target_option", ['--target', '-t']) -def test_install_package_to_usersite_with_target_must_fail(script, - target_option): - """ - Test that installing package to usersite with target - must raise error - """ - target_dir = script.scratch_path / 'target' - result = script.pip_install_local( - '--user', target_option, target_dir, "simple==1.0", expect_error=True - ) - assert "Can not combine '--user' and '--target'" in result.stderr, ( - str(result) - ) - - def test_install_nonlocal_compatible_wheel(script, data): target_dir = script.scratch_path / 'target' @@ -1075,21 +1059,6 @@ def test_install_editable_with_prefix(script): result.did_create(install_path) -def test_install_package_conflict_prefix_and_user(script, data): - """ - Test installing a package using pip install --prefix --user errors out - """ - prefix_path = script.scratch_path / 'prefix' - result = script.pip( - 'install', '-f', data.find_links, '--no-index', '--user', - '--prefix', prefix_path, 'simple==1.0', - expect_error=True, quiet=True, - ) - assert ( - "Can not combine '--user' and '--prefix'" in result.stderr - ) - - def test_install_package_that_emits_unicode(script, data): """ Install a package with a setup.py that emits UTF-8 output and then fails. diff --git a/tests/unit/test_command_install.py b/tests/unit/test_command_install.py index 7b6b38de0fa..5e844e97e70 100644 --- a/tests/unit/test_command_install.py +++ b/tests/unit/test_command_install.py @@ -1,4 +1,5 @@ import errno +from itertools import product import pytest from mock import patch @@ -9,40 +10,90 @@ decide_user_install, reject_location_related_install_options, ) -from pip._internal.exceptions import CommandError +from pip._internal.exceptions import CommandError, InstallationError from pip._internal.req.req_install import InstallRequirement +ENABLE_USER_SITE = 'site.ENABLE_USER_SITE' +WRITABLE = 'pip._internal.commands.install.test_writable_dir' +ISDIR = 'pip._internal.commands.install.os.path.isdir' +EXISTS = 'pip._internal.commands.install.os.path.exists' + +def false(*args, **kwargs): + """Return False.""" + return False + + +def true(*args, **kwargs): + """Return True.""" + return True + + +# virtualenv_no_global is tested by functional test +@patch('pip._internal.commands.install.virtualenv_no_global', false) class TestDecideUserInstall: - @patch('site.ENABLE_USER_SITE', True) - @patch('pip._internal.commands.install.site_packages_writable') - def test_prefix_and_target(self, sp_writable): - sp_writable.return_value = False - - assert decide_user_install( - use_user_site=None, prefix_path='foo' - ) is False - - assert decide_user_install( - use_user_site=None, target_dir='bar' - ) is False - - @pytest.mark.parametrize( - "enable_user_site,site_packages_writable,result", [ - (True, True, False), - (True, False, True), - (False, True, False), - (False, False, False), - ]) - def test_most_cases( - self, enable_user_site, site_packages_writable, result, monkeypatch, - ): - monkeypatch.setattr('site.ENABLE_USER_SITE', enable_user_site) - monkeypatch.setattr( - 'pip._internal.commands.install.site_packages_writable', - lambda **kw: site_packages_writable - ) - assert decide_user_install(use_user_site=None) is result + @pytest.mark.parametrize('use_user_site,prefix_path,target_dir,root_path', + filter(lambda args: sum(map(bool, args)) > 1, + product((False, True), (None, 'foo'), + (None, 'bar'), (None, 'baz')))) + def test_conflicts(self, use_user_site, + prefix_path, target_dir, root_path): + with pytest.raises(CommandError): + decide_user_install( + use_user_site=use_user_site, prefix_path=prefix_path, + target_dir=target_dir, root_path=root_path) + + def test_target_dir(self): + with patch(WRITABLE, true): + with patch(EXISTS, true), patch(ISDIR, false): + with pytest.raises(InstallationError): + decide_user_install(target_dir='bar') + for exists, isdir in (false, false), (false, true), (true, true): + with patch(EXISTS, exists), patch(ISDIR, isdir): + assert decide_user_install(target_dir='bar') is False + + def test_target_writable(self): + with patch(EXISTS, false): + with patch(WRITABLE, false), pytest.raises(InstallationError): + decide_user_install(target_dir='bar') + with patch(WRITABLE, true): + assert decide_user_install(target_dir='bar') is False + + def test_prefix_writable(self): + with patch(WRITABLE, false), pytest.raises(InstallationError): + decide_user_install(prefix_path='foo') + with patch(WRITABLE, true): + assert decide_user_install(prefix_path='foo') is False + + def test_global_site_writable(self): + with patch(ENABLE_USER_SITE, True): + with patch(WRITABLE, false): + with pytest.raises(InstallationError): + decide_user_install(use_user_site=False) + with pytest.raises(InstallationError): + decide_user_install(root_path='baz') + assert decide_user_install() is True + with patch(WRITABLE, true): + assert decide_user_install(use_user_site=True) is True + assert decide_user_install(root_path='baz') is False + assert decide_user_install() is False + + def test_enable_user_site(self): + with patch(WRITABLE, true): + with patch(ENABLE_USER_SITE, None): + assert decide_user_install() is False + assert decide_user_install(use_user_site=False) is False + with pytest.raises(InstallationError): + decide_user_install(use_user_site=True) + with patch(ENABLE_USER_SITE, False): + assert decide_user_install() is False + assert decide_user_install(use_user_site=False) is False + assert decide_user_install(use_user_site=True) is True + with patch(ENABLE_USER_SITE, True): + assert decide_user_install(use_user_site=False) is False + assert decide_user_install(use_user_site=True) is True + with patch(WRITABLE, false), patch(ENABLE_USER_SITE, True): + assert decide_user_install() is True def test_rejection_for_pip_install_options(): From 57d13050a54e042f5a99ff0818b08c57d61cad6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Gia=20Phong?= Date: Sun, 8 Mar 2020 11:36:05 +0700 Subject: [PATCH 3/8] Drop redundant explicit bool checks and clarify site.ENABLE_USER_SITE Co-authored-by: Sviatoslav Sydorenko --- src/pip/_internal/commands/install.py | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index b734e5b819c..b9dcb418bbc 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -5,8 +5,8 @@ import operator import os import shutil -import site from optparse import SUPPRESS_HELP +from site import ENABLE_USER_SITE from pip._vendor import pkg_resources from pip._vendor.packaging.utils import canonicalize_name @@ -656,18 +656,27 @@ def decide_user_install( logger.debug("Non-user install due to specified prefix path") return False + # ENABLE_USER_SITE shows user site-packages directory status: + # * True means that it is enabled and was added to sys.path. + # * False means that it was disabled by user request + # (with -s or PYTHONNOUSERSITE). + # * None means it was disabled for security reasons + # (mismatch between user or group id and effective id) + # or by an administrator. if use_user_site is not None: use_user_site = bool(use_user_site) - logger.debug("{} install by explicit request".format( - "User" if use_user_site else "Non-user")) - if use_user_site is True and site.ENABLE_USER_SITE is None: + if use_user_site: + logger.debug("User install by explicit request") + else: + logger.debug("Non-user install by explicit request") + if use_user_site and ENABLE_USER_SITE is None: raise InstallationError("User site-packages are disabled for " "security reasons or by an administrator.") - elif site.ENABLE_USER_SITE is None: + elif ENABLE_USER_SITE is None: logger.debug("Non-user install since user site-packages are disabled " "for security reasons or by an administrator.") use_user_site = False - elif site.ENABLE_USER_SITE is False: + elif ENABLE_USER_SITE is False: logger.debug("Non-user install since user site-packages are disabled " "by user request (with 'python -s' or PYTHONNOUSERSITE)") use_user_site = False @@ -683,13 +692,13 @@ def decide_user_install( "normal site-packages is not writeable") use_user_site = True - if use_user_site is True: + assert isinstance(use_user_site, bool) + if use_user_site: if virtualenv_no_global(): raise InstallationError( "Can not perform a '--user' install. " "User site-packages are not visible in this virtualenv.") else: - assert use_user_site is False if not site_packages_writable(root=root_path, isolated=isolated_mode): raise InstallationError("Cannot write to global site-packages.") return use_user_site From 33eb54557e843ba50edfa291d53131f0ce98cdc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Gia=20Phong?= Date: Sun, 8 Mar 2020 15:15:45 +0700 Subject: [PATCH 4/8] Move decide_user_install to a separate unit test The use of the function is also emphasized. --- news/6762.bugfix | 2 - news/6762.feature | 1 + news/7829.feature | 1 + src/pip/_internal/commands/install.py | 7 +- tests/unit/test_command_install.py | 87 +----------- tests/unit/test_decide_user_install.py | 185 +++++++++++++++++++++++++ 6 files changed, 194 insertions(+), 89 deletions(-) delete mode 100644 news/6762.bugfix create mode 100644 news/6762.feature create mode 100644 news/7829.feature create mode 100644 tests/unit/test_decide_user_install.py diff --git a/news/6762.bugfix b/news/6762.bugfix deleted file mode 100644 index a980d99bc97..00000000000 --- a/news/6762.bugfix +++ /dev/null @@ -1,2 +0,0 @@ -Fail early when install path is not writable. At the same time, -install location compatibility is checked. diff --git a/news/6762.feature b/news/6762.feature new file mode 100644 index 00000000000..ec18c8fb92d --- /dev/null +++ b/news/6762.feature @@ -0,0 +1 @@ +Fail early when install path is not writable. diff --git a/news/7829.feature b/news/7829.feature new file mode 100644 index 00000000000..8bc9e19ed7c --- /dev/null +++ b/news/7829.feature @@ -0,0 +1 @@ +Do not allow ``--target``, ``--user``, ``--root`` and ``--prefix`` together. diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index b9dcb418bbc..090ae25d527 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -240,8 +240,13 @@ def run(self, options, args): install_options = options.install_options or [] logger.debug("Using %s", get_pip_version()) + + # decide_user_install not only return the whether to use user + # site-packages, but also validate compatibility of the input + # options with each other and with the environment. Therefore + # the following statement should not be moved downward. options.use_user_site = decide_user_install( - options.use_user_site, + use_user_site=options.use_user_site, prefix_path=options.prefix_path, target_dir=options.target_dir, root_path=options.root_path, diff --git a/tests/unit/test_command_install.py b/tests/unit/test_command_install.py index 5e844e97e70..624804d2c6c 100644 --- a/tests/unit/test_command_install.py +++ b/tests/unit/test_command_install.py @@ -1,100 +1,15 @@ import errno -from itertools import product import pytest -from mock import patch from pip._vendor.packaging.requirements import Requirement from pip._internal.commands.install import ( create_env_error_message, - decide_user_install, reject_location_related_install_options, ) -from pip._internal.exceptions import CommandError, InstallationError +from pip._internal.exceptions import CommandError from pip._internal.req.req_install import InstallRequirement -ENABLE_USER_SITE = 'site.ENABLE_USER_SITE' -WRITABLE = 'pip._internal.commands.install.test_writable_dir' -ISDIR = 'pip._internal.commands.install.os.path.isdir' -EXISTS = 'pip._internal.commands.install.os.path.exists' - - -def false(*args, **kwargs): - """Return False.""" - return False - - -def true(*args, **kwargs): - """Return True.""" - return True - - -# virtualenv_no_global is tested by functional test -@patch('pip._internal.commands.install.virtualenv_no_global', false) -class TestDecideUserInstall: - @pytest.mark.parametrize('use_user_site,prefix_path,target_dir,root_path', - filter(lambda args: sum(map(bool, args)) > 1, - product((False, True), (None, 'foo'), - (None, 'bar'), (None, 'baz')))) - def test_conflicts(self, use_user_site, - prefix_path, target_dir, root_path): - with pytest.raises(CommandError): - decide_user_install( - use_user_site=use_user_site, prefix_path=prefix_path, - target_dir=target_dir, root_path=root_path) - - def test_target_dir(self): - with patch(WRITABLE, true): - with patch(EXISTS, true), patch(ISDIR, false): - with pytest.raises(InstallationError): - decide_user_install(target_dir='bar') - for exists, isdir in (false, false), (false, true), (true, true): - with patch(EXISTS, exists), patch(ISDIR, isdir): - assert decide_user_install(target_dir='bar') is False - - def test_target_writable(self): - with patch(EXISTS, false): - with patch(WRITABLE, false), pytest.raises(InstallationError): - decide_user_install(target_dir='bar') - with patch(WRITABLE, true): - assert decide_user_install(target_dir='bar') is False - - def test_prefix_writable(self): - with patch(WRITABLE, false), pytest.raises(InstallationError): - decide_user_install(prefix_path='foo') - with patch(WRITABLE, true): - assert decide_user_install(prefix_path='foo') is False - - def test_global_site_writable(self): - with patch(ENABLE_USER_SITE, True): - with patch(WRITABLE, false): - with pytest.raises(InstallationError): - decide_user_install(use_user_site=False) - with pytest.raises(InstallationError): - decide_user_install(root_path='baz') - assert decide_user_install() is True - with patch(WRITABLE, true): - assert decide_user_install(use_user_site=True) is True - assert decide_user_install(root_path='baz') is False - assert decide_user_install() is False - - def test_enable_user_site(self): - with patch(WRITABLE, true): - with patch(ENABLE_USER_SITE, None): - assert decide_user_install() is False - assert decide_user_install(use_user_site=False) is False - with pytest.raises(InstallationError): - decide_user_install(use_user_site=True) - with patch(ENABLE_USER_SITE, False): - assert decide_user_install() is False - assert decide_user_install(use_user_site=False) is False - assert decide_user_install(use_user_site=True) is True - with patch(ENABLE_USER_SITE, True): - assert decide_user_install(use_user_site=False) is False - assert decide_user_install(use_user_site=True) is True - with patch(WRITABLE, false), patch(ENABLE_USER_SITE, True): - assert decide_user_install() is True - def test_rejection_for_pip_install_options(): install_options = ["--prefix=/hello"] diff --git a/tests/unit/test_decide_user_install.py b/tests/unit/test_decide_user_install.py new file mode 100644 index 00000000000..d65bae549f0 --- /dev/null +++ b/tests/unit/test_decide_user_install.py @@ -0,0 +1,185 @@ +"""Test user site-packages installation decision +and other install destination option conflicts. +""" + +from itertools import product + +from pytest import fixture, mark, param, raises + +from pip._internal.commands.install import decide_user_install +from pip._internal.exceptions import CommandError, InstallationError + +ENABLE_USER_SITE = 'pip._internal.commands.install.ENABLE_USER_SITE' +ISDIR = 'pip._internal.commands.install.os.path.isdir' +EXISTS = 'pip._internal.commands.install.os.path.exists' +WRITABLE = 'pip._internal.commands.install.test_writable_dir' +VIRTUALENV_NO_GLOBAL = 'pip._internal.commands.install.virtualenv_no_global' + + +def false(*args, **kwargs): + """Return False.""" + return False + + +def true(*args, **kwargs): + """Return True.""" + return True + + +@fixture +def user_site_enabled(monkeypatch): + """site.ENABLE_USER_SITE mocked to be True.""" + monkeypatch.setattr(ENABLE_USER_SITE, True) + + +@fixture +def nonexists(monkeypatch): + """os.path.exists mocked to always return False.""" + monkeypatch.setattr(EXISTS, false) + + +@fixture +def exists(monkeypatch): + """os.path.exists mocked to always return True.""" + monkeypatch.setattr(EXISTS, true) + + +@fixture +def isnotdir(monkeypatch): + """os.path.isdir mocked to always return False.""" + monkeypatch.setattr(ISDIR, false) + + +@fixture +def nonwritable_global(monkeypatch): + """test_writable_dir mocked to always return False.""" + monkeypatch.setattr(WRITABLE, false) + + +@fixture +def writable_global(monkeypatch): + """test_writable_dir mocked to always return True.""" + monkeypatch.setattr(WRITABLE, true) + + +@fixture +def virtualenv_global(monkeypatch): + """virtualenv_no_global mocked to always return False.""" + monkeypatch.setattr(VIRTUALENV_NO_GLOBAL, false) + + +@fixture +def virtualenv_no_global(monkeypatch): + """virtualenv_no_global mocked to always return False.""" + monkeypatch.setattr(VIRTUALENV_NO_GLOBAL, true) + + +@mark.parametrize(('use_user_site', 'prefix_path', 'target_dir', 'root_path'), + filter(lambda args: sum(map(bool, args)) > 1, + product((False, True), (None, 'foo'), + (None, 'bar'), (None, 'baz')))) +def test_conflicts(use_user_site, prefix_path, target_dir, root_path): + """Test conflicts of target, user, root and prefix options.""" + with raises(CommandError): + decide_user_install( + use_user_site=use_user_site, prefix_path=prefix_path, + target_dir=target_dir, root_path=root_path) + + +def test_target_exists_error(writable_global, exists, isnotdir): + """Test existing target which is not a directory.""" + with raises(InstallationError): + decide_user_install(target_dir='bar') + + +@mark.parametrize(('exist', 'is_dir'), + ((false, false), (false, true), (true, true))) +def test_target_exists(exist, is_dir, writable_global, monkeypatch): + """Test target paths for non-error exist-isdir combinations.""" + monkeypatch.setattr(EXISTS, exist) + monkeypatch.setattr(ISDIR, is_dir) + assert decide_user_install(target_dir='bar') is False + + +def test_target_nonwritable(nonexists, nonwritable_global): + """Test nonwritable path check with target specified.""" + with raises(InstallationError): + decide_user_install(target_dir='bar') + + +def test_target_writable(nonexists, writable_global): + """Test writable path check with target specified.""" + assert decide_user_install(target_dir='bar') is False + + +def test_prefix_nonwritable(nonwritable_global): + """Test nonwritable path check with prefix specified.""" + with raises(InstallationError): + decide_user_install(prefix_path='foo') + + +def test_prefix_writable(writable_global): + """Test writable path check with prefix specified.""" + assert decide_user_install(prefix_path='foo') is False + + +@mark.parametrize('kwargs', ( + param({'use_user_site': False}, id='not using user-site specified'), + param({'root_path': 'baz'}, id='root path specified'))) +def test_global_site_nonwritable(kwargs, nonwritable_global, + virtualenv_global): + """Test error handling when global site-packages is not writable.""" + with raises(InstallationError): + decide_user_install(**kwargs) + + +@mark.parametrize('kwargs', ( + param({'use_user_site': True}, id='using user-site specified'), + param({'root_path': 'baz'}, id='root path specified'))) +def test_global_site_writable(kwargs, writable_global, + virtualenv_global, user_site_enabled): + """Test if user site-packages decision is the same as specified + when global site-packages is writable. + """ + expected_decision = kwargs.get('use_user_site', False) + assert decide_user_install(**kwargs) is expected_decision + + +@mark.parametrize(('writable', 'result'), ((false, True), (true, False))) +def test_global_site_auto(writable, result, virtualenv_global, + user_site_enabled, monkeypatch): + """Test error handling and user site-packages decision + with writable and non-writable global site-packages, + when no argument is provided. + """ + monkeypatch.setattr(WRITABLE, writable) + assert decide_user_install() is result + + +def test_enable_user_site_error(virtualenv_global, monkeypatch): + """Test error raised when site.ENABLE_USER_SITE is None + but use_user_site is requested. + """ + monkeypatch.setattr(ENABLE_USER_SITE, None) + with raises(InstallationError): + decide_user_install(use_user_site=True) + + +@mark.parametrize(('use_user_site', 'enable_user_site'), + filter(lambda args: set(args) != {None, True}, + product((None, False, True), (None, False, True)))) +def test_enable_user_site(use_user_site, enable_user_site, + virtualenv_global, writable_global, monkeypatch): + """Test with different values of site.ENABLE_USER_SITE.""" + monkeypatch.setattr(ENABLE_USER_SITE, enable_user_site) + assert decide_user_install(use_user_site) is bool(use_user_site) + + +@mark.parametrize('use_user_site', (None, True)) +def test_virtualenv_no_global(use_user_site, virtualenv_no_global, + user_site_enabled, nonwritable_global): + """Test for final assertion of virtualenv_no_global + when user site-packages is decided to be used. + """ + with raises(InstallationError): + decide_user_install(use_user_site) From a7cd38b628cf2cd4047110212f01107cbdf24441 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Gia=20Phong?= Date: Fri, 8 May 2020 23:12:28 +0700 Subject: [PATCH 5/8] Allow --root to be used with other install dest options Effectively user-site also need write permission checking. --- news/7828.bugfix | 1 + news/7829.feature | 1 - src/pip/_internal/commands/install.py | 18 ++++++--- tests/unit/test_decide_user_install.py | 55 +++++++++++++++----------- 4 files changed, 45 insertions(+), 30 deletions(-) create mode 100644 news/7828.bugfix delete mode 100644 news/7829.feature diff --git a/news/7828.bugfix b/news/7828.bugfix new file mode 100644 index 00000000000..0241caf87ef --- /dev/null +++ b/news/7828.bugfix @@ -0,0 +1 @@ +Print more concise error when ``--target`` and ``--prefix`` are used together. diff --git a/news/7829.feature b/news/7829.feature deleted file mode 100644 index 8bc9e19ed7c..00000000000 --- a/news/7829.feature +++ /dev/null @@ -1 +0,0 @@ -Do not allow ``--target``, ``--user``, ``--root`` and ``--prefix`` together. diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 090ae25d527..7c2854158d0 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -5,6 +5,7 @@ import operator import os import shutil +from distutils.util import change_root from optparse import SUPPRESS_HELP from site import ENABLE_USER_SITE @@ -637,8 +638,8 @@ def decide_user_install( is also asserted in this function. """ # Check for incompatible options - locations = {"--target": target_dir, "--user": use_user_site, - "--root": root_path, "--prefix": prefix_path} + locations = {"--target": target_dir is not None, "--user": use_user_site, + "--prefix": prefix_path is not None} destinations = [k for k, v in locations.items() if v] if len(destinations) > 1: last, rest = destinations[-1], ", ".join(destinations[:-1]) @@ -646,6 +647,8 @@ def decide_user_install( "{} and {}.".format(rest, last)) if target_dir: + if root_path is not None: + target_dir = change_root(root_path, target_dir) if os.path.exists(target_dir) and not os.path.isdir(target_dir): raise InstallationError("Target path exists but is not " "a directory: {}".format(target_dir)) @@ -655,7 +658,8 @@ def decide_user_install( logger.debug("Non-user install due to specified target directory") return False if prefix_path: - if not test_writable_dir(prefix_path): + if not site_packages_writable(root=root_path, isolated=isolated_mode, + prefix=prefix_path): raise InstallationError( "Cannot write to prefix path: {}".format(prefix_path)) logger.debug("Non-user install due to specified prefix path") @@ -703,9 +707,11 @@ def decide_user_install( raise InstallationError( "Can not perform a '--user' install. " "User site-packages are not visible in this virtualenv.") - else: - if not site_packages_writable(root=root_path, isolated=isolated_mode): - raise InstallationError("Cannot write to global site-packages.") + if not site_packages_writable(user=use_user_site, root=root_path, + isolated=isolated_mode): + raise InstallationError("Cannot write to user site-packages.") + elif not site_packages_writable(root=root_path, isolated=isolated_mode): + raise InstallationError("Cannot write to global site-packages.") return use_user_site diff --git a/tests/unit/test_decide_user_install.py b/tests/unit/test_decide_user_install.py index d65bae549f0..81aa27c0d51 100644 --- a/tests/unit/test_decide_user_install.py +++ b/tests/unit/test_decide_user_install.py @@ -12,6 +12,7 @@ ENABLE_USER_SITE = 'pip._internal.commands.install.ENABLE_USER_SITE' ISDIR = 'pip._internal.commands.install.os.path.isdir' EXISTS = 'pip._internal.commands.install.os.path.exists' +SITE_WRITABLE = 'pip._internal.commands.install.site_packages_writable' WRITABLE = 'pip._internal.commands.install.test_writable_dir' VIRTUALENV_NO_GLOBAL = 'pip._internal.commands.install.virtualenv_no_global' @@ -51,13 +52,13 @@ def isnotdir(monkeypatch): @fixture -def nonwritable_global(monkeypatch): +def nonwritable(monkeypatch): """test_writable_dir mocked to always return False.""" monkeypatch.setattr(WRITABLE, false) @fixture -def writable_global(monkeypatch): +def writable(monkeypatch): """test_writable_dir mocked to always return True.""" monkeypatch.setattr(WRITABLE, true) @@ -74,19 +75,18 @@ def virtualenv_no_global(monkeypatch): monkeypatch.setattr(VIRTUALENV_NO_GLOBAL, true) -@mark.parametrize(('use_user_site', 'prefix_path', 'target_dir', 'root_path'), +@mark.parametrize(('use_user_site', 'prefix_path', 'target_dir'), filter(lambda args: sum(map(bool, args)) > 1, - product((False, True), (None, 'foo'), - (None, 'bar'), (None, 'baz')))) -def test_conflicts(use_user_site, prefix_path, target_dir, root_path): + product((False, True), (None, 'foo'), (None, 'bar')))) +def test_conflicts(use_user_site, prefix_path, target_dir): """Test conflicts of target, user, root and prefix options.""" with raises(CommandError): - decide_user_install( - use_user_site=use_user_site, prefix_path=prefix_path, - target_dir=target_dir, root_path=root_path) + decide_user_install(use_user_site=use_user_site, + prefix_path=prefix_path, + target_dir=target_dir) -def test_target_exists_error(writable_global, exists, isnotdir): +def test_target_exists_error(writable, exists, isnotdir): """Test existing target which is not a directory.""" with raises(InstallationError): decide_user_install(target_dir='bar') @@ -94,31 +94,31 @@ def test_target_exists_error(writable_global, exists, isnotdir): @mark.parametrize(('exist', 'is_dir'), ((false, false), (false, true), (true, true))) -def test_target_exists(exist, is_dir, writable_global, monkeypatch): +def test_target_exists(exist, is_dir, writable, monkeypatch): """Test target paths for non-error exist-isdir combinations.""" monkeypatch.setattr(EXISTS, exist) monkeypatch.setattr(ISDIR, is_dir) assert decide_user_install(target_dir='bar') is False -def test_target_nonwritable(nonexists, nonwritable_global): +def test_target_nonwritable(nonexists, nonwritable): """Test nonwritable path check with target specified.""" with raises(InstallationError): decide_user_install(target_dir='bar') -def test_target_writable(nonexists, writable_global): +def test_target_writable(nonexists, writable): """Test writable path check with target specified.""" assert decide_user_install(target_dir='bar') is False -def test_prefix_nonwritable(nonwritable_global): +def test_prefix_nonwritable(nonwritable): """Test nonwritable path check with prefix specified.""" with raises(InstallationError): decide_user_install(prefix_path='foo') -def test_prefix_writable(writable_global): +def test_prefix_writable(writable): """Test writable path check with prefix specified.""" assert decide_user_install(prefix_path='foo') is False @@ -126,7 +126,7 @@ def test_prefix_writable(writable_global): @mark.parametrize('kwargs', ( param({'use_user_site': False}, id='not using user-site specified'), param({'root_path': 'baz'}, id='root path specified'))) -def test_global_site_nonwritable(kwargs, nonwritable_global, +def test_global_site_nonwritable(kwargs, nonwritable, virtualenv_global): """Test error handling when global site-packages is not writable.""" with raises(InstallationError): @@ -136,7 +136,7 @@ def test_global_site_nonwritable(kwargs, nonwritable_global, @mark.parametrize('kwargs', ( param({'use_user_site': True}, id='using user-site specified'), param({'root_path': 'baz'}, id='root path specified'))) -def test_global_site_writable(kwargs, writable_global, +def test_global_site_writable(kwargs, writable, virtualenv_global, user_site_enabled): """Test if user site-packages decision is the same as specified when global site-packages is writable. @@ -145,15 +145,16 @@ def test_global_site_writable(kwargs, writable_global, assert decide_user_install(**kwargs) is expected_decision -@mark.parametrize(('writable', 'result'), ((false, True), (true, False))) -def test_global_site_auto(writable, result, virtualenv_global, +@mark.parametrize('writable_global', (False, True)) +def test_global_site_auto(writable_global, virtualenv_global, user_site_enabled, monkeypatch): """Test error handling and user site-packages decision with writable and non-writable global site-packages, when no argument is provided. """ - monkeypatch.setattr(WRITABLE, writable) - assert decide_user_install() is result + monkeypatch.setattr(SITE_WRITABLE, + lambda **kwargs: kwargs.get('user') or writable_global) + assert decide_user_install() is not writable_global def test_enable_user_site_error(virtualenv_global, monkeypatch): @@ -169,7 +170,7 @@ def test_enable_user_site_error(virtualenv_global, monkeypatch): filter(lambda args: set(args) != {None, True}, product((None, False, True), (None, False, True)))) def test_enable_user_site(use_user_site, enable_user_site, - virtualenv_global, writable_global, monkeypatch): + virtualenv_global, writable, monkeypatch): """Test with different values of site.ENABLE_USER_SITE.""" monkeypatch.setattr(ENABLE_USER_SITE, enable_user_site) assert decide_user_install(use_user_site) is bool(use_user_site) @@ -177,9 +178,17 @@ def test_enable_user_site(use_user_site, enable_user_site, @mark.parametrize('use_user_site', (None, True)) def test_virtualenv_no_global(use_user_site, virtualenv_no_global, - user_site_enabled, nonwritable_global): + user_site_enabled, nonwritable): """Test for final assertion of virtualenv_no_global when user site-packages is decided to be used. """ with raises(InstallationError): decide_user_install(use_user_site) + + +def test_user_site_nonwritable(nonwritable): + """Test when user-site is not writable, + which usually only happens when root path is specified. + """ + with raises(InstallationError): + decide_user_install(True) From e5ddcc7c59c96b678c3f03443d1b4c70eadc7bc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Gia=20Phong?= Date: Wed, 8 Jul 2020 22:04:33 +0700 Subject: [PATCH 6/8] Fix commands.install.site_packages_writable signature --- src/pip/_internal/commands/install.py | 28 +++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 7c2854158d0..c2e35be7b65 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -601,24 +601,28 @@ def _warn_about_conflicts(self, conflict_details, new_resolver): def get_lib_location_guesses( - user=False, # type: bool - home=None, # type: Optional[str] - root=None, # type: Optional[str] - isolated=False, # type: bool - prefix=None # type: Optional[str] + user=False, # type: bool + home=None, # type: Optional[str] + root=None, # type: Optional[str] + isolated=False, # type: bool + prefix=None, # type: Optional[str] ): - # type:(...) -> List[str] + # type: (...) -> List[str] scheme = distutils_scheme('', user=user, home=home, root=root, isolated=isolated, prefix=prefix) return [scheme['purelib'], scheme['platlib']] -def site_packages_writable(root, isolated): - # type: (Optional[str], bool) -> bool - return all( - test_writable_dir(d) for d in set( - get_lib_location_guesses(root=root, isolated=isolated)) - ) +def site_packages_writable( + user=False, # type: bool + root=None, # type: Optional[str] + isolated=False, # type: bool + prefix=None, # type: Optional[str] +): + # type: (...) -> bool + return all(map(test_writable_dir, set(get_lib_location_guesses( + user=user, root=root, isolated=isolated, prefix=prefix, + )))) def decide_user_install( From 90a1357455268bb97474791678b460409ac3dd94 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Thu, 30 Jul 2020 01:30:08 +0530 Subject: [PATCH 7/8] Blacken decide_user_install and clarify log output --- src/pip/_internal/commands/install.py | 76 +++++++++++++++++---------- 1 file changed, 48 insertions(+), 28 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index c2e35be7b65..dfde7af107f 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -642,30 +642,41 @@ def decide_user_install( is also asserted in this function. """ # Check for incompatible options - locations = {"--target": target_dir is not None, "--user": use_user_site, - "--prefix": prefix_path is not None} + locations = { + "--target": target_dir is not None, + "--user": use_user_site, + "--prefix": prefix_path is not None, + } destinations = [k for k, v in locations.items() if v] if len(destinations) > 1: last, rest = destinations[-1], ", ".join(destinations[:-1]) - raise CommandError("Different installation locations are implied by " - "{} and {}.".format(rest, last)) + raise CommandError( + "Different installation locations are implied " + "by {} and {}.".format(rest, last) + ) - if target_dir: + if target_dir is not None: if root_path is not None: target_dir = change_root(root_path, target_dir) if os.path.exists(target_dir) and not os.path.isdir(target_dir): - raise InstallationError("Target path exists but is not " - "a directory: {}".format(target_dir)) + raise InstallationError( + "Target path exists but is not " + "a directory: {}".format(target_dir) + ) if not test_writable_dir(target_dir): raise InstallationError( - "Cannot write to target directory: {}".format(target_dir)) + "Cannot write to target directory: {}".format(target_dir) + ) logger.debug("Non-user install due to specified target directory") return False - if prefix_path: - if not site_packages_writable(root=root_path, isolated=isolated_mode, - prefix=prefix_path): + + if prefix_path is not None: + if not site_packages_writable( + root=root_path, isolated=isolated_mode, prefix=prefix_path, + ): raise InstallationError( - "Cannot write to prefix path: {}".format(prefix_path)) + "Cannot write to prefix path: {}".format(prefix_path) + ) logger.debug("Non-user install due to specified prefix path") return False @@ -683,26 +694,33 @@ def decide_user_install( else: logger.debug("Non-user install by explicit request") if use_user_site and ENABLE_USER_SITE is None: - raise InstallationError("User site-packages are disabled for " - "security reasons or by an administrator.") + raise InstallationError( + "User site-packages are disabled " + "for security reasons or by an administrator." + ) elif ENABLE_USER_SITE is None: - logger.debug("Non-user install since user site-packages are disabled " - "for security reasons or by an administrator.") + logger.debug( + "User site-packages are disabled " + "for security reasons or by an administrator." + ) use_user_site = False elif ENABLE_USER_SITE is False: - logger.debug("Non-user install since user site-packages are disabled " - "by user request (with 'python -s' or PYTHONNOUSERSITE)") - use_user_site = False - elif root_path: logger.debug( - "Non-user install since alternate root directory is specified") + "User site-packages are disabled by user request " + "(with 'python -s' or PYTHONNOUSERSITE)" + ) use_user_site = False - elif site_packages_writable(root=root_path, isolated=isolated_mode): - logger.debug("Non-user install because site-packages writeable") + elif root_path is not None: + logger.debug("Non-user install in favor of alternative root directory") + use_user_site = False + elif site_packages_writable(isolated=isolated_mode): + logger.debug("Installing to global site-packages as it is writeable") return False else: - logger.info("Defaulting to user installation because " - "normal site-packages is not writeable") + logger.info( + "Defaulting to user installation because " + "normal site-packages is not writeable" + ) use_user_site = True assert isinstance(use_user_site, bool) @@ -710,9 +728,11 @@ def decide_user_install( if virtualenv_no_global(): raise InstallationError( "Can not perform a '--user' install. " - "User site-packages are not visible in this virtualenv.") - if not site_packages_writable(user=use_user_site, root=root_path, - isolated=isolated_mode): + "User site-packages are not visible in this virtualenv." + ) + if not site_packages_writable( + user=use_user_site, root=root_path, isolated=isolated_mode, + ): raise InstallationError("Cannot write to user site-packages.") elif not site_packages_writable(root=root_path, isolated=isolated_mode): raise InstallationError("Cannot write to global site-packages.") From d7472a88950bb687d6db1a792d823e70b21ec4e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Gia=20Phong?= Date: Thu, 30 Jul 2020 17:34:09 +0700 Subject: [PATCH 8/8] Improve log/error messages esp. ones involve ENABLE_USER_SITE This is to be squashed with the commit above --- src/pip/_internal/commands/install.py | 21 +++++++++------------ tests/functional/test_install_user.py | 2 +- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index dfde7af107f..0050aa72d57 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -688,6 +688,7 @@ def decide_user_install( # (mismatch between user or group id and effective id) # or by an administrator. if use_user_site is not None: + # use_user_site might be passed as an int. use_user_site = bool(use_user_site) if use_user_site: logger.debug("User install by explicit request") @@ -695,19 +696,15 @@ def decide_user_install( logger.debug("Non-user install by explicit request") if use_user_site and ENABLE_USER_SITE is None: raise InstallationError( - "User site-packages are disabled " - "for security reasons or by an administrator." + "User site-packages cannot be used because " + "site.ENABLE_USER_SITE is None, which indicates " + "it was disabled for security reasons or by an administrator." ) - elif ENABLE_USER_SITE is None: + elif not ENABLE_USER_SITE: logger.debug( - "User site-packages are disabled " - "for security reasons or by an administrator." - ) - use_user_site = False - elif ENABLE_USER_SITE is False: - logger.debug( - "User site-packages are disabled by user request " - "(with 'python -s' or PYTHONNOUSERSITE)" + "User site-packages is disabled " + "because site.ENABLE_USER_SITE is %s", + ENABLE_USER_SITE, ) use_user_site = False elif root_path is not None: @@ -728,7 +725,7 @@ def decide_user_install( if virtualenv_no_global(): raise InstallationError( "Can not perform a '--user' install. " - "User site-packages are not visible in this virtualenv." + "User site-packages is not visible in this virtualenv." ) if not site_packages_writable( user=use_user_site, root=root_path, isolated=isolated_mode, diff --git a/tests/functional/test_install_user.py b/tests/functional/test_install_user.py index 24169470a70..97d1c7f9a16 100644 --- a/tests/functional/test_install_user.py +++ b/tests/functional/test_install_user.py @@ -98,7 +98,7 @@ def test_install_user_venv_nositepkgs_fails(self, virtualenv, expect_error=True, ) assert ( - "Can not perform a '--user' install. User site-packages are not " + "Can not perform a '--user' install. User site-packages is not " "visible in this virtualenv." in result.stderr )