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/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/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 8c2c32fd43f..0050aa72d57 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -5,8 +5,9 @@ import operator import os import shutil -import site +from distutils.util import change_root 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 @@ -230,9 +231,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: @@ -243,8 +241,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, @@ -256,13 +259,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 @@ -605,84 +601,139 @@ 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( - 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] 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. + If use_user_site is None, the behavior is decided upon other + arguments and the environment. + + Compatibility of installation location options and the environment + is also asserted in this function. """ - # 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 + # Check for incompatible options + 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) + ) - if use_user_site: - if prefix_path: - raise CommandError( - "Can not combine '--user' and '--prefix' as they imply " - "different installation locations" + 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) ) - if virtualenv_no_global(): + if not test_writable_dir(target_dir): raise InstallationError( - "Can not perform a '--user' install. User site-packages " - "are not visible in this virtualenv." + "Cannot write to target directory: {}".format(target_dir) ) - 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") + 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 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) + ) + 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): - logger.debug("Non-user install because site-packages writeable") + # 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 might be passed as an int. + use_user_site = bool(use_user_site) + 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 cannot be used because " + "site.ENABLE_USER_SITE is None, which indicates " + "it was disabled for security reasons or by an administrator." + ) + elif not ENABLE_USER_SITE: + logger.debug( + "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: + 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" + ) + use_user_site = True - logger.info("Defaulting to user installation because normal site-packages " - "is not writeable") - return 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 is 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.") + return use_user_site def reject_location_related_install_options(requirements, options): 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/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 ) diff --git a/tests/unit/test_command_install.py b/tests/unit/test_command_install.py index 7b6b38de0fa..624804d2c6c 100644 --- a/tests/unit/test_command_install.py +++ b/tests/unit/test_command_install.py @@ -1,50 +1,16 @@ import errno 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 from pip._internal.req.req_install import InstallRequirement -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 - - def test_rejection_for_pip_install_options(): install_options = ["--prefix=/hello"] with pytest.raises(CommandError) as e: diff --git a/tests/unit/test_decide_user_install.py b/tests/unit/test_decide_user_install.py new file mode 100644 index 00000000000..81aa27c0d51 --- /dev/null +++ b/tests/unit/test_decide_user_install.py @@ -0,0 +1,194 @@ +"""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' +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' + + +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(monkeypatch): + """test_writable_dir mocked to always return False.""" + monkeypatch.setattr(WRITABLE, false) + + +@fixture +def writable(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'), + filter(lambda args: sum(map(bool, args)) > 1, + 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) + + +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') + + +@mark.parametrize(('exist', 'is_dir'), + ((false, false), (false, true), (true, true))) +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): + """Test nonwritable path check with target specified.""" + with raises(InstallationError): + decide_user_install(target_dir='bar') + + +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): + """Test nonwritable path check with prefix specified.""" + with raises(InstallationError): + decide_user_install(prefix_path='foo') + + +def test_prefix_writable(writable): + """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, + 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, + 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_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(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): + """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, 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): + """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)