Fail early when install path is not writable

news/6762.feature

@@ -0,0 +1 @@
+Fail early when install path is not writable.

news/7828.bugfix

@@ -0,0 +1 @@
+Print more concise error when ``--target`` and ``--prefix`` are used together.

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

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.

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
         )
 

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: