Allow --root to be used with other install dest options

Effectively user-site also need write permission checking.

news/7828.bugfix

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

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
 
@@ -571,15 +572,17 @@ 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])
         raise CommandError("Different installation locations are implied by "
                            "{} 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))
@@ -589,7 +592,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")
@@ -637,9 +641,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
 
 

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,59 +75,58 @@ 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')
 
 
 @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
 
 
 @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,17 +170,25 @@ 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)
 
 
 @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)