From 042d4844e8fe8b664c7e29c0c66a4fe399c45581 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Mon, 9 Sep 2019 15:22:08 +0100 Subject: [PATCH 01/18] Default to --user install in certain conditions --- src/pip/_internal/commands/install.py | 20 ++++++++++++-- src/pip/_internal/utils/filesystem.py | 40 +++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 66071f6e819..2cb9c055d7f 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -13,6 +13,7 @@ import operator import os import shutil +import site from optparse import SUPPRESS_HELP from pip._vendor import pkg_resources @@ -28,11 +29,11 @@ InstallationError, PreviousBuildDirError, ) -from pip._internal.locations import distutils_scheme +from pip._internal.locations import distutils_scheme, site_packages from pip._internal.operations.check import check_install_conflicts from pip._internal.req import RequirementSet, install_given_reqs from pip._internal.req.req_tracker import RequirementTracker -from pip._internal.utils.filesystem import check_path_owner +from pip._internal.utils.filesystem import check_path_owner, test_writable_dir from pip._internal.utils.misc import ( ensure_dir, get_installed_version, @@ -305,6 +306,17 @@ def run(self, options, args): install_options.append('--user') install_options.append('--prefix=') + elif options.use_user_site is None: + if options.prefix_path or options.target_dir: + options.use_user_site = False + elif site_packages_writable( + root=options.root_path, + isolated=options.isolated_mode + ): + options.use_user_site = False + elif site.ENABLE_USER_SITE: + options.use_user_site = True + target_temp_dir = None # type: Optional[TempDirectory] target_temp_dir_path = None # type: Optional[str] if options.target_dir: @@ -594,6 +606,10 @@ def get_lib_location_guesses(*args, **kwargs): return [scheme['purelib'], scheme['platlib']] +def site_packages_writable(**kwargs): + return all(test_writable_dir(d) for d in get_lib_location_guesses(**kwargs)) + + def create_env_error_message(error, show_traceback, using_user_site): """Format an error message for an EnvironmentError diff --git a/src/pip/_internal/utils/filesystem.py b/src/pip/_internal/utils/filesystem.py index f4a389cd92f..db099e4a64b 100644 --- a/src/pip/_internal/utils/filesystem.py +++ b/src/pip/_internal/utils/filesystem.py @@ -1,5 +1,6 @@ import os import os.path +import random import shutil import stat from contextlib import contextmanager @@ -113,3 +114,42 @@ def replace(src, dest): else: replace = _replace_retry(os.replace) + + +# test_writable_dir and _test_writable_dir_win are copied from Flit, +# with the author's agreement to also place them under pip's license. +def test_writable_dir(path): + """Check if a directory is writable. + + Uses os.access() on POSIX, tries creating files on Windows. + """ + if os.name == 'posix': + return os.access(path, os.W_OK) + + return _test_writable_dir_win(path) + + +def _test_writable_dir_win(path): + # os.access doesn't work on Windows: http://bugs.python.org/issue2528 + # and we can't use tempfile: http://bugs.python.org/issue22107 + basename = 'accesstest_deleteme_fishfingers_custard_' + alphabet = 'abcdefghijklmnopqrstuvwxyz0123456789' + for i in range(10): + name = basename + ''.join(random.choice(alphabet) for _ in range(6)) + file = os.path.join(path, name) + try: + with open(file, mode='xb'): + pass + except FileExistsError: + continue + except PermissionError: + # This could be because there's a directory with the same name. + # But it's highly unlikely there's a directory called that, + # so we'll assume it's because the parent directory is not writable. + return False + else: + os.unlink(file) + return True + + # This should never be reached + raise EnvironmentError('Unexpected condition testing for writable directory') From 14e76d0eaf7f4aa59894403fa92eed4d7fea4074 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Wed, 9 Oct 2019 08:38:58 +0100 Subject: [PATCH 02/18] Remove unused import --- src/pip/_internal/commands/install.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 2cb9c055d7f..b5180f5b514 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -29,7 +29,7 @@ InstallationError, PreviousBuildDirError, ) -from pip._internal.locations import distutils_scheme, site_packages +from pip._internal.locations import distutils_scheme from pip._internal.operations.check import check_install_conflicts from pip._internal.req import RequirementSet, install_given_reqs from pip._internal.req.req_tracker import RequirementTracker From 172274ac1dcb2839ec8a52bdf07a8d88dd6814d9 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Wed, 9 Oct 2019 08:39:07 +0100 Subject: [PATCH 03/18] Add type annotation comments --- src/pip/_internal/utils/filesystem.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/pip/_internal/utils/filesystem.py b/src/pip/_internal/utils/filesystem.py index db099e4a64b..6de6ea65d4d 100644 --- a/src/pip/_internal/utils/filesystem.py +++ b/src/pip/_internal/utils/filesystem.py @@ -119,6 +119,7 @@ def replace(src, dest): # test_writable_dir and _test_writable_dir_win are copied from Flit, # with the author's agreement to also place them under pip's license. def test_writable_dir(path): + # type: (str) -> bool """Check if a directory is writable. Uses os.access() on POSIX, tries creating files on Windows. @@ -130,6 +131,7 @@ def test_writable_dir(path): def _test_writable_dir_win(path): + # type: (str) -> bool # os.access doesn't work on Windows: http://bugs.python.org/issue2528 # and we can't use tempfile: http://bugs.python.org/issue22107 basename = 'accesstest_deleteme_fishfingers_custard_' From f7132d9c6d898c10e0c9146a572818f46be4581e Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Wed, 9 Oct 2019 08:43:42 +0100 Subject: [PATCH 04/18] Fix OSError catching for Python 2 --- src/pip/_internal/utils/filesystem.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/pip/_internal/utils/filesystem.py b/src/pip/_internal/utils/filesystem.py index 6de6ea65d4d..88d214a32ab 100644 --- a/src/pip/_internal/utils/filesystem.py +++ b/src/pip/_internal/utils/filesystem.py @@ -1,3 +1,4 @@ +import errno import os import os.path import random @@ -142,13 +143,15 @@ def _test_writable_dir_win(path): try: with open(file, mode='xb'): pass - except FileExistsError: - continue - except PermissionError: - # This could be because there's a directory with the same name. - # But it's highly unlikely there's a directory called that, - # so we'll assume it's because the parent directory is not writable. - return False + except OSError as e: + if e.errno == errno.EEXIST: + continue + if e.errno == errno.EPERM: + # This could be because there's a directory with the same name. + # But it's highly unlikely there's a directory called that, + # so we'll assume it's because the parent dir is not writable. + return False + raise else: os.unlink(file) return True From a6b13b637b1cd7208f478734817212e409ff8a17 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Wed, 9 Oct 2019 08:43:58 +0100 Subject: [PATCH 05/18] Fix maximum line length --- src/pip/_internal/commands/install.py | 4 +++- src/pip/_internal/utils/filesystem.py | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index b5180f5b514..dfeb9c04ff6 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -607,7 +607,9 @@ def get_lib_location_guesses(*args, **kwargs): def site_packages_writable(**kwargs): - return all(test_writable_dir(d) for d in get_lib_location_guesses(**kwargs)) + return all( + test_writable_dir(d) for d in get_lib_location_guesses(**kwargs) + ) def create_env_error_message(error, show_traceback, using_user_site): diff --git a/src/pip/_internal/utils/filesystem.py b/src/pip/_internal/utils/filesystem.py index 88d214a32ab..90e344a1465 100644 --- a/src/pip/_internal/utils/filesystem.py +++ b/src/pip/_internal/utils/filesystem.py @@ -157,4 +157,6 @@ def _test_writable_dir_win(path): return True # This should never be reached - raise EnvironmentError('Unexpected condition testing for writable directory') + raise EnvironmentError( + 'Unexpected condition testing for writable directory' + ) From f2b1882b6a0438d4fead602ff3c3e08e9e8ea7ae Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Wed, 9 Oct 2019 08:46:33 +0100 Subject: [PATCH 06/18] Add news file --- news/1668.feature | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 news/1668.feature diff --git a/news/1668.feature b/news/1668.feature new file mode 100644 index 00000000000..d200841ea7f --- /dev/null +++ b/news/1668.feature @@ -0,0 +1,2 @@ +Default to doing a user install (as if ``--user`` was passed) when the main +site-packages directory is not writeable and user site-packages are enabled. From a9405b22fcae2fc3f5dc7d2ef79e3bbcdea32363 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Wed, 9 Oct 2019 08:48:33 +0100 Subject: [PATCH 07/18] Ensure necessary install_options are set for default user install --- src/pip/_internal/commands/install.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index dfeb9c04ff6..f7bbceaec37 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -316,6 +316,8 @@ def run(self, options, args): options.use_user_site = False elif site.ENABLE_USER_SITE: options.use_user_site = True + install_options.append('--user') + install_options.append('--prefix=') target_temp_dir = None # type: Optional[TempDirectory] target_temp_dir_path = None # type: Optional[str] From 086ab4c52aa1112f57acfabe166a4095681978df Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Wed, 9 Oct 2019 09:46:12 +0100 Subject: [PATCH 08/18] Fix exclusive-mode open for Python 2 --- src/pip/_internal/utils/filesystem.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pip/_internal/utils/filesystem.py b/src/pip/_internal/utils/filesystem.py index 90e344a1465..f076f738d52 100644 --- a/src/pip/_internal/utils/filesystem.py +++ b/src/pip/_internal/utils/filesystem.py @@ -141,8 +141,7 @@ def _test_writable_dir_win(path): name = basename + ''.join(random.choice(alphabet) for _ in range(6)) file = os.path.join(path, name) try: - with open(file, mode='xb'): - pass + fd = os.open(file, os.O_RDWR | os.O_CREAT | os.O_EXCL) except OSError as e: if e.errno == errno.EEXIST: continue @@ -153,6 +152,7 @@ def _test_writable_dir_win(path): return False raise else: + os.close(fd) os.unlink(file) return True From 357c1322221290ee640454fe28fce7f7dfeae95d Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Wed, 9 Oct 2019 10:13:29 +0100 Subject: [PATCH 09/18] Handle nonexistant directory when checking for write access --- src/pip/_internal/utils/filesystem.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/pip/_internal/utils/filesystem.py b/src/pip/_internal/utils/filesystem.py index f076f738d52..bedd662958a 100644 --- a/src/pip/_internal/utils/filesystem.py +++ b/src/pip/_internal/utils/filesystem.py @@ -125,6 +125,13 @@ def test_writable_dir(path): Uses os.access() on POSIX, tries creating files on Windows. """ + # If the directory doesn't exist, find the closest parent that does. + while not os.path.isdir(path): + parent = os.path.dirname(path) + if parent == path: + break # Should never get here, but infinite loops are bad + path = parent + if os.name == 'posix': return os.access(path, os.W_OK) From 4a4f1ca1cfbfa7d8a6ac4ae55f8aae8501eb2fbc Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Wed, 9 Oct 2019 13:54:46 +0100 Subject: [PATCH 10/18] Relax failing tests due to changing site-packages mtime --- tests/functional/test_install.py | 1 - tests/functional/test_install_upgrade.py | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index 2e980d5402d..d6a2ce0ee9c 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -248,7 +248,6 @@ def test_basic_editable_install(script): in result.stderr ) assert not result.files_created - assert not result.files_updated @pytest.mark.svn diff --git a/tests/functional/test_install_upgrade.py b/tests/functional/test_install_upgrade.py index 36b518b2546..6d2eeb5dc4e 100644 --- a/tests/functional/test_install_upgrade.py +++ b/tests/functional/test_install_upgrade.py @@ -245,7 +245,9 @@ def test_upgrade_to_same_version_from_url(script): 'https://files.pythonhosted.org/packages/source/I/INITools/INITools-' '0.3.tar.gz', ) - assert not result2.files_updated, 'INITools 0.3 reinstalled same version' + assert script.site_packages / 'initools' not in result2.files_updated, ( + 'INITools 0.3 reinstalled same version' + ) result3 = script.pip('uninstall', 'initools', '-y') assert_all_changes(result, result3, [script.venv / 'build', 'cache']) From 5f1468274987348b569aa586eeca4363494d0357 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Wed, 9 Oct 2019 17:34:16 +0100 Subject: [PATCH 11/18] Factor out code for handling the --user option & its default --- src/pip/_internal/commands/install.py | 70 ++++++++++++++++++--------- 1 file changed, 47 insertions(+), 23 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index f7bbceaec37..84feb023bc5 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -292,33 +292,19 @@ def run(self, options, args): options.src_dir = os.path.abspath(options.src_dir) install_options = options.install_options or [] + + options.use_user_site = decide_user_install( + options.use_user_site, + prefix_path=options.prefix_path, + target_dir=options.target_dir, + root_path=options.root_path, + isolated_mode=options.isolated_mode, + ) + if options.use_user_site: - if options.prefix_path: - raise CommandError( - "Can not combine '--user' and '--prefix' as they imply " - "different installation locations" - ) - if virtualenv_no_global(): - raise InstallationError( - "Can not perform a '--user' install. User site-packages " - "are not visible in this virtualenv." - ) install_options.append('--user') install_options.append('--prefix=') - elif options.use_user_site is None: - if options.prefix_path or options.target_dir: - options.use_user_site = False - elif site_packages_writable( - root=options.root_path, - isolated=options.isolated_mode - ): - options.use_user_site = False - elif site.ENABLE_USER_SITE: - options.use_user_site = True - install_options.append('--user') - install_options.append('--prefix=') - target_temp_dir = None # type: Optional[TempDirectory] target_temp_dir_path = None # type: Optional[str] if options.target_dir: @@ -614,6 +600,44 @@ def site_packages_writable(**kwargs): ) +def decide_user_install( + use_user_site, + prefix_path, + target_dir, + root_path, + isolated_mode, +): + """Determine whether to do a user install based on the input options. + + If use_user_site is True/False, that is checked for compatibility with + other options. If None, the default behaviour depends on other options + 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(): + raise InstallationError( + "Can not perform a '--user' install. User site-packages " + "are not visible in this virtualenv." + ) + if use_user_site in (True, False): + return use_user_site + + if prefix_path or target_dir: + return False # user install incompatible with --prefix/--target + + # Default behaviour: prefer non-user installation if that looks possible. + # If we don't have permission for that and user site-packages are visible, + # choose a user install. + return site.ENABLE_USER_SITE and not site_packages_writable( + root=root_path, isolated=isolated_mode + ) + + def create_env_error_message(error, show_traceback, using_user_site): """Format an error message for an EnvironmentError From e94e7877ab09ad11c7eb53ccad50097faa0ebede Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Sat, 19 Oct 2019 11:23:27 +0100 Subject: [PATCH 12/18] Polish & clarify decide_user_install() function --- src/pip/_internal/commands/install.py | 45 +++++++++++++++++---------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 84feb023bc5..f6a5ac53e6a 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -601,19 +601,25 @@ def site_packages_writable(**kwargs): def decide_user_install( - use_user_site, - prefix_path, - target_dir, - root_path, - isolated_mode, + use_user_site, # type: Optional[bool] + prefix_path, # type: Optional[str] + target_dir, # type: Optional[str] + root_path, # type: Optional[str] + isolated_mode, # type: bool ): + # type: (...) -> bool """Determine whether to do a user install based on the input options. - If use_user_site is True/False, that is checked for compatibility with - other options. If None, the default behaviour depends on other options - and the environment. + 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: + if use_user_site is False: + return False + + if use_user_site is True: if prefix_path: raise CommandError( "Can not combine '--user' and '--prefix' as they imply " @@ -624,17 +630,22 @@ def decide_user_install( "Can not perform a '--user' install. User site-packages " "are not visible in this virtualenv." ) - if use_user_site in (True, False): - return use_user_site + 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: - return False # user install incompatible with --prefix/--target + return False + + # If user installs are not enabled, choose a non-user install + if not site.ENABLE_USER_SITE: + return False - # Default behaviour: prefer non-user installation if that looks possible. - # If we don't have permission for that and user site-packages are visible, - # choose a user install. - return site.ENABLE_USER_SITE and not site_packages_writable( - root=root_path, isolated=isolated_mode + # If we don't have permissions for a non-user install, choose a user install + return not site_packages_writable( + root=root_path, isolated=isolated_mode, ) From ecac7a17b51dbfb59a554633a165e4889baac776 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Sat, 19 Oct 2019 11:28:51 +0100 Subject: [PATCH 13/18] Line length --- src/pip/_internal/commands/install.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index f6a5ac53e6a..cb7b972ee17 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -643,7 +643,7 @@ def decide_user_install( if not site.ENABLE_USER_SITE: return False - # If we don't have permissions for a non-user install, choose a user install + # If we don't have permission for a non-user install, choose a user install return not site_packages_writable( root=root_path, isolated=isolated_mode, ) From 79f05916300a0fe41c70b227eadc8fcda4eda170 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Sun, 20 Oct 2019 21:23:49 +0100 Subject: [PATCH 14/18] Don't check write access on the same path twice --- src/pip/_internal/commands/install.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index cb7b972ee17..851a9537974 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -596,7 +596,7 @@ def get_lib_location_guesses(*args, **kwargs): def site_packages_writable(**kwargs): return all( - test_writable_dir(d) for d in get_lib_location_guesses(**kwargs) + test_writable_dir(d) for d in set(get_lib_location_guesses(**kwargs)) ) From 16174f4ad07664d71be72276997f6b0e3a0fd6d4 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Sun, 20 Oct 2019 21:28:56 +0100 Subject: [PATCH 15/18] Add logging for decide_user_install --- src/pip/_internal/commands/install.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 851a9537974..ead9f0fe9c7 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -617,6 +617,7 @@ def decide_user_install( which is provided by the other arguments. """ if use_user_site is False: + logger.debug("Non-user install by explicit request") return False if use_user_site is True: @@ -630,6 +631,7 @@ def decide_user_install( "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 @@ -637,16 +639,22 @@ def decide_user_install( # user install incompatible with --prefix/--target if prefix_path or target_dir: + logger.debug("Non-user install due to --prefix or --target option") 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") return False # If we don't have permission for a non-user install, choose a user install - return not site_packages_writable( - root=root_path, isolated=isolated_mode, - ) + if site_packages_writable(root=root_path, isolated=isolated_mode): + logger.debug("Non-user install because site-packages writeable") + return False + + logger.info("Defaulting to user installation because normal site-packages " + "is not writeable") + return True def create_env_error_message(error, show_traceback, using_user_site): From fbc0588c0135574f6ad61928bd9ca70564bf4ef4 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Sun, 20 Oct 2019 21:48:18 +0100 Subject: [PATCH 16/18] Add unit tests of decide_user_install() --- src/pip/_internal/commands/install.py | 8 +++--- tests/unit/test_command_install.py | 36 ++++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index ead9f0fe9c7..3f19ade8927 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -602,10 +602,10 @@ def site_packages_writable(**kwargs): def decide_user_install( use_user_site, # type: Optional[bool] - prefix_path, # type: Optional[str] - target_dir, # type: Optional[str] - root_path, # type: Optional[str] - isolated_mode, # type: 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. diff --git a/tests/unit/test_command_install.py b/tests/unit/test_command_install.py index 1a3fee5c9ae..34635cd6df9 100644 --- a/tests/unit/test_command_install.py +++ b/tests/unit/test_command_install.py @@ -1,6 +1,6 @@ from mock import Mock, call, patch -from pip._internal.commands.install import build_wheels +from pip._internal.commands.install import build_wheels, decide_user_install class TestWheelCache: @@ -61,3 +61,37 @@ def test_build_wheels__wheel_not_installed(self, is_wheel_installed): ] assert build_failures == ['a'] + + +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 + + @patch('pip._internal.commands.install.site_packages_writable') + def test_user_site_enabled(self, sp_writable): + sp_writable.return_value = False + + with patch('site.ENABLE_USER_SITE', True): + assert decide_user_install(use_user_site=None) is True + + with patch('site.ENABLE_USER_SITE', False): + assert decide_user_install(use_user_site=None) is False + + @patch('site.ENABLE_USER_SITE', True) + @patch('pip._internal.commands.install.site_packages_writable') + def test_site_packages_access(self, sp_writable): + sp_writable.return_value = True + assert decide_user_install(use_user_site=None) is False + + sp_writable.return_value = False + assert decide_user_install(use_user_site=None) is True From c4d92bbb4ea704069de8381e3629348db32c3818 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Mon, 21 Oct 2019 08:52:44 +0100 Subject: [PATCH 17/18] Use pytest parametrize for decide_user_install tests --- tests/unit/test_command_install.py | 35 +++++++++++++++--------------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/tests/unit/test_command_install.py b/tests/unit/test_command_install.py index 34635cd6df9..13d644a8587 100644 --- a/tests/unit/test_command_install.py +++ b/tests/unit/test_command_install.py @@ -1,3 +1,4 @@ +import pytest from mock import Mock, call, patch from pip._internal.commands.install import build_wheels, decide_user_install @@ -77,21 +78,19 @@ def test_prefix_and_target(self, sp_writable): use_user_site=None, target_dir='bar' ) is False - @patch('pip._internal.commands.install.site_packages_writable') - def test_user_site_enabled(self, sp_writable): - sp_writable.return_value = False - - with patch('site.ENABLE_USER_SITE', True): - assert decide_user_install(use_user_site=None) is True - - with patch('site.ENABLE_USER_SITE', False): - assert decide_user_install(use_user_site=None) is False - - @patch('site.ENABLE_USER_SITE', True) - @patch('pip._internal.commands.install.site_packages_writable') - def test_site_packages_access(self, sp_writable): - sp_writable.return_value = True - assert decide_user_install(use_user_site=None) is False - - sp_writable.return_value = False - assert decide_user_install(use_user_site=None) is True + @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 From 42b1ef3537343fa1787480b45daa353241b4705a Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Mon, 21 Oct 2019 13:52:50 +0100 Subject: [PATCH 18/18] Clarify comment --- src/pip/_internal/commands/install.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 3f19ade8927..12d58f6e056 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -647,7 +647,8 @@ def decide_user_install( logger.debug("Non-user install because user site-packages disabled") return False - # If we don't have permission for a non-user install, choose a user install + # 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") return False