From acf8634e8fc80055347c2d565f9fd1a877ae900e Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Sat, 30 Aug 2014 20:15:32 -0400 Subject: [PATCH 1/3] Fixing issues with install's --target option. 1. Check for the existence of a directory before copying from temporary directory to final target. If it exists, warn the user. 2. If the user specifies the --upgrade option and the directory exists, delete it and continue with installation. 3. Adding tests for above cases. This resolves #1489, #1710 completely and parts of #1833. --- pip/commands/install.py | 17 +++++++++++++++-- tests/functional/test_install.py | 13 +++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/pip/commands/install.py b/pip/commands/install.py index c95594ff846..6ed1bc61280 100644 --- a/pip/commands/install.py +++ b/pip/commands/install.py @@ -51,7 +51,10 @@ def __init__(self, *args, **kw): dest='target_dir', metavar='dir', default=None, - help='Install packages into .') + help='Install packages into . ' + 'By default this will not replace existing files/folders in .' + 'Use --upgrade to replace existing packages in with new versions.' + ) cmd_opts.add_option( '-d', '--download', '--download-dir', '--download-directory', @@ -344,9 +347,19 @@ def run(self, options, args): os.makedirs(options.target_dir) lib_dir = distutils_scheme('', home=temp_target_dir)['purelib'] for item in os.listdir(lib_dir): + target_item_dir = os.path.join(options.target_dir, item) + if os.path.exists(target_item_dir): + if not options.upgrade: + logger.warn('Target directory %s already exists. Specify --upgrade to force replacement.' % target_item_dir) + continue + if not os.path.isdir(target_item_dir): + logger.warn('Target directory %s already exists and is not a directory. Please remove in order for replacement.' % target_item_dir) + continue + shutil.rmtree(target_item_dir) + shutil.move( os.path.join(lib_dir, item), - os.path.join(options.target_dir, item), + target_item_dir ) shutil.rmtree(temp_target_dir) return requirement_set diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index 07f6a6f5b98..5ab99ba7536 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -527,6 +527,19 @@ def test_install_package_with_target(script): str(result) ) + # Test repeated call without --upgrade, no files should have changed + result = script.pip('install', '-t', target_dir, "initools==0.1") + assert not Path('scratch') / 'target' / 'initools' in result.files_updated + + # Test upgrade call, check that new version is installed + result = script.pip('install', '--upgrade', '-t', target_dir, "initools==0.2") + assert Path('scratch') / 'target' / 'initools' in result.files_updated, ( + str(result) + ) + assert Path('scratch') / 'target' / 'INITools-0.2-py%s.egg-info' % pyversion in result.files_created, ( + str(result) + ) + def test_install_package_with_root(script, data): """ From 12ff251d2d25a73bae00e799db1947df7182e4e4 Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Sat, 30 Aug 2014 21:25:03 -0400 Subject: [PATCH 2/3] Fixing pep8 errors --- pip/commands/install.py | 18 ++++++++++++++---- tests/functional/test_install.py | 7 +++++-- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/pip/commands/install.py b/pip/commands/install.py index 6ed1bc61280..9842875041d 100644 --- a/pip/commands/install.py +++ b/pip/commands/install.py @@ -52,8 +52,9 @@ def __init__(self, *args, **kw): metavar='dir', default=None, help='Install packages into . ' - 'By default this will not replace existing files/folders in .' - 'Use --upgrade to replace existing packages in with new versions.' + 'By default this will not replace existing files/folders in ' + '. Use --upgrade to replace existing packages in ' + 'with new versions.' ) cmd_opts.add_option( @@ -350,10 +351,19 @@ def run(self, options, args): target_item_dir = os.path.join(options.target_dir, item) if os.path.exists(target_item_dir): if not options.upgrade: - logger.warn('Target directory %s already exists. Specify --upgrade to force replacement.' % target_item_dir) + logger.warn( + 'Target directory %s already exists. Specify ' + '--upgrade to force replacement.' + % target_item_dir + ) continue if not os.path.isdir(target_item_dir): - logger.warn('Target directory %s already exists and is not a directory. Please remove in order for replacement.' % target_item_dir) + logger.warn( + 'Target directory %s already exists and is ' + 'not a directory. Please remove in order ' + 'for replacement.' + % target_item_dir + ) continue shutil.rmtree(target_item_dir) diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index 5ab99ba7536..7c65dfcd9c5 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -532,11 +532,14 @@ def test_install_package_with_target(script): assert not Path('scratch') / 'target' / 'initools' in result.files_updated # Test upgrade call, check that new version is installed - result = script.pip('install', '--upgrade', '-t', target_dir, "initools==0.2") + result = script.pip('install', '--upgrade', '-t', + target_dir, "initools==0.2") assert Path('scratch') / 'target' / 'initools' in result.files_updated, ( str(result) ) - assert Path('scratch') / 'target' / 'INITools-0.2-py%s.egg-info' % pyversion in result.files_created, ( + egg_folder = ( + Path('scratch') / 'target' / 'INITools-0.2-py%s.egg-info' % pyversion) + assert egg_folder in result.files_created, ( str(result) ) From ecb3049b8d299d9c456102ff513d4e7110eba913 Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Sun, 31 Aug 2014 12:11:27 -0400 Subject: [PATCH 3/3] Covering case of single-module packages. Behavior is now to overwrite files or folders if --upgrade is specified, however, links will not be overwritten. --- pip/commands/install.py | 12 ++++++++---- tests/functional/test_install.py | 11 +++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/pip/commands/install.py b/pip/commands/install.py index 9842875041d..d126e64a2a5 100644 --- a/pip/commands/install.py +++ b/pip/commands/install.py @@ -357,15 +357,19 @@ def run(self, options, args): % target_item_dir ) continue - if not os.path.isdir(target_item_dir): + if os.path.islink(target_item_dir): logger.warn( 'Target directory %s already exists and is ' - 'not a directory. Please remove in order ' - 'for replacement.' + 'a link. Pip will not automatically replace ' + 'links, please remove if replacement is ' + 'desired.' % target_item_dir ) continue - shutil.rmtree(target_item_dir) + if os.path.isdir(target_item_dir): + shutil.rmtree(target_item_dir) + else: + os.remove(target_item_dir) shutil.move( os.path.join(lib_dir, item), diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index 7c65dfcd9c5..8a0a5b19781 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -543,6 +543,17 @@ def test_install_package_with_target(script): str(result) ) + # Test install and upgrade of single-module package + result = script.pip('install', '-t', target_dir, 'six') + assert Path('scratch') / 'target' / 'six.py' in result.files_created, ( + str(result) + ) + + result = script.pip('install', '-t', target_dir, '--upgrade', 'six') + assert Path('scratch') / 'target' / 'six.py' in result.files_updated, ( + str(result) + ) + def test_install_package_with_root(script, data): """