From 23e120b4cfb9ea4eed67f2ad604c791401374088 Mon Sep 17 00:00:00 2001 From: "Pradyun S. Gedam" Date: Fri, 16 Sep 2016 14:20:50 +0530 Subject: [PATCH 1/7] Add an upgrade-strategy option Associate an upgrade-strategy with `pip install --upgrade`. The upgrade- strategy determines how upgrading of the dependencies. Provides 2 upgrade-strategies "eager" and "non-eager". "eager" upgrades dependencies regardless of whether they currently satisfy the new parent version-specification. "non-eager" upgrades dependencies if and only if they do not satisfy the new parent version-specification. --- pip/commands/install.py | 25 ++++- pip/req/req_set.py | 45 ++++++-- tests/data/packages/README.txt | 5 + tests/data/packages/require_simple-1.0.tar.gz | Bin 0 -> 760 bytes tests/functional/test_install_upgrade.py | 106 ++++++++++++++++++ 5 files changed, 169 insertions(+), 12 deletions(-) create mode 100644 tests/data/packages/require_simple-1.0.tar.gz diff --git a/pip/commands/install.py b/pip/commands/install.py index af577c673d1..f25535ae5a0 100644 --- a/pip/commands/install.py +++ b/pip/commands/install.py @@ -88,8 +88,20 @@ def __init__(self, *args, **kw): dest='upgrade', action='store_true', help='Upgrade all specified packages to the newest available ' - 'version. This process is recursive regardless of whether ' - 'a dependency is already satisfied.' + 'version. The handling of dependencies depends on the ' + 'upgrade-strategy used.' + ) + + cmd_opts.add_option( + '--upgrade-strategy', + dest='upgrade_strategy', + default='non-eager', + help='Determines how dependency upgrading should be handled. ' + '"eager" - dependencies are upgraded regardless of ' + 'whether the currently installed version satisfies the ' + 'requirements of the upgraded package(s). ' + '"non-eager" - are upgraded only when they do not satisfy ' + 'the requirements of the upgraded package(s).' ) cmd_opts.add_option( @@ -224,6 +236,14 @@ def run(self, options, args): if options.build_dir: options.build_dir = os.path.abspath(options.build_dir) + allowed_strategies = ["eager", "non-eager"] + if options.upgrade_strategy not in allowed_strategies: + raise CommandError( + "pip does not know upgrade strategy provided: '%s'" % ( + options.upgrade_strategy, + ) + ) + options.src_dir = os.path.abspath(options.src_dir) install_options = options.install_options or [] if options.use_user_site: @@ -278,6 +298,7 @@ def run(self, options, args): src_dir=options.src_dir, download_dir=options.download_dir, upgrade=options.upgrade, + upgrade_strategy=options.upgrade_strategy, as_egg=options.as_egg, ignore_installed=options.ignore_installed, ignore_dependencies=options.ignore_dependencies, diff --git a/pip/req/req_set.py b/pip/req/req_set.py index 40b0d3e1307..e149a802838 100644 --- a/pip/req/req_set.py +++ b/pip/req/req_set.py @@ -140,10 +140,10 @@ def prep_for_dist(self): class RequirementSet(object): def __init__(self, build_dir, src_dir, download_dir, upgrade=False, - ignore_installed=False, as_egg=False, target_dir=None, - ignore_dependencies=False, force_reinstall=False, - use_user_site=False, session=None, pycompile=True, - isolated=False, wheel_download_dir=None, + upgrade_strategy=None, ignore_installed=False, as_egg=False, + target_dir=None, ignore_dependencies=False, + force_reinstall=False, use_user_site=False, session=None, + pycompile=True, isolated=False, wheel_download_dir=None, wheel_cache=None, require_hashes=False): """Create a RequirementSet. @@ -170,6 +170,7 @@ def __init__(self, build_dir, src_dir, download_dir, upgrade=False, # the wheelhouse output by 'pip wheel'. self.download_dir = download_dir self.upgrade = upgrade + self.upgrade_strategy = upgrade_strategy self.ignore_installed = ignore_installed self.force_reinstall = force_reinstall self.requirements = Requirements() @@ -241,6 +242,8 @@ def add_requirement(self, install_req, parent_req_name=None): install_req.use_user_site = self.use_user_site install_req.target_dir = self.target_dir install_req.pycompile = self.pycompile + install_req.is_direct = (parent_req_name is None) + if not name: # url or path requirement w/o an egg fragment self.unnamed_requirements.append(install_req) @@ -375,6 +378,13 @@ def prepare_files(self, finder): if hash_errors: raise hash_errors + def _is_upgrade_allowed(self, req): + return self.upgrade and ( + self.upgrade_strategy == "eager" or ( + self.upgrade_strategy == "non-eager" and req.is_direct + ) + ) + def _check_skip_installed(self, req_to_install, finder): """Check if req_to_install should be skipped. @@ -396,17 +406,20 @@ def _check_skip_installed(self, req_to_install, finder): # Check whether to upgrade/reinstall this req or not. req_to_install.check_if_exists() if req_to_install.satisfied_by: - skip_reason = 'satisfied (use --upgrade to upgrade)' - if self.upgrade: - best_installed = False + upgrade_allowed = self._is_upgrade_allowed(req_to_install) + + # Is the best version is installed. + best_installed = False + + if upgrade_allowed: # For link based requirements we have to pull the # tree down and inspect to assess the version #, so # its handled way down. if not (self.force_reinstall or req_to_install.link): try: - finder.find_requirement(req_to_install, self.upgrade) + finder.find_requirement( + req_to_install, upgrade_allowed) except BestVersionAlreadyInstalled: - skip_reason = 'up-to-date' best_installed = True except DistributionNotFound: # No distribution found, so we squash the @@ -423,6 +436,15 @@ def _check_skip_installed(self, req_to_install, finder): req_to_install.conflicts_with = \ req_to_install.satisfied_by req_to_install.satisfied_by = None + + # Figure out a nice message to say why we're skipping this. + if best_installed: + skip_reason = 'already up-to-date' + elif self.upgrade_strategy == "non-eager": + skip_reason = 'not upgraded as not directly required' + else: + skip_reason = 'already satisfied' + return skip_reason else: return None @@ -520,7 +542,10 @@ def _prepare_file(self, % (req_to_install, req_to_install.source_dir) ) req_to_install.populate_link( - finder, self.upgrade, require_hashes) + finder, + self._is_upgrade_allowed(req_to_install), + require_hashes + ) # We can't hit this spot and have populate_link return None. # req_to_install.satisfied_by is None here (because we're # guarded) and upgrade has no impact except when satisfied_by diff --git a/tests/data/packages/README.txt b/tests/data/packages/README.txt index c36d77f1b0e..b2f61b5138c 100644 --- a/tests/data/packages/README.txt +++ b/tests/data/packages/README.txt @@ -108,3 +108,8 @@ requires_wheelbroken_upper -------------------------- Requires wheelbroken and upper - used for testing implicit wheel building during install. + +require_simple-1.0.tar.gz +------------------------ +contains "require_simple" package which requires simple>=2.0 - used for testing +if dependencies are handled correctly. diff --git a/tests/data/packages/require_simple-1.0.tar.gz b/tests/data/packages/require_simple-1.0.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..c4eca04a783de0cc5b102062dad0552b8c6b30bc GIT binary patch literal 760 zcmV}pp6|72-%bT4vcadl~OWnXh?ZE$R5Eio=IE_7jX0PUD>Z<{a> z#(B-B@VXamrIHv3SgMps)BdYlTCM8#qN)&09EJ*p851qveok7Nk~LYdco$n<7S;mqFMV#gdx9kq>8cjnrjCr%wW*)t}Rv5O!T*vc$>h}z4`#s+`h-b7w zS*TDFV&pO$O-oTkEB5{Eef2}C^_d|5^nbxsnRkZcs7e3(Grz9#KXdE;cc@F5K`8wH z=$!K?3Su$NE=(0h!6;Om^a-6m6cZ6hK8cGs6Sv08GEPSN$ute{{JBT}byK$){cn~3 z9ow^A{m6tlmRU;XaiFp+DToe^OsZ?k0U5_) z6y)LXDvWqxnse)(2vgqQt%uX@fl1~}xSU-g`@1^6ZjG_fe|?e!H{TTf^PTI@UiF<0 z-MOgtRVVdp=kD#_yxvFdYF7UzM<1+DgZIa+>EB__Qvc69y+Hph@P(^TH+`Y??OLLT zM2CPm&|*F$_1G|LJ5KejA(YcYayB>`9Dg5}AG4HOxmM=F^t#l^L@qyDJPqSyVf`Er zxhU>@_Ldhz8Ru$#R%JX%LN(50y0AdIt=#`RJc_K@fS~(N`roeqa~ssGdCv#)R7oo=iC4f?;{a{l+IkN&R}s%GC@ zJ@qqJ1w_>-m>WdQ$~_~Vb1~v#I1Q3mTor3KSFPgOO%JY9(EnQf-{+}j>OZxY^v@j6 zh5p+B6(9QFQ2%$6Z=>~}+P&ra?`s7?|83O&#l#h<0Qz62{|C>7x1RqxJ(r^Xx55VV qUkmeJ00000000000000000000000000002sY4IE1Z+2S%PyhfL4UJR) literal 0 HcmV?d00001 diff --git a/tests/functional/test_install_upgrade.py b/tests/functional/test_install_upgrade.py index f4a78f0ca7c..9d98eabea08 100644 --- a/tests/functional/test_install_upgrade.py +++ b/tests/functional/test_install_upgrade.py @@ -23,6 +23,112 @@ def test_no_upgrade_unless_requested(script): ) +def test_invalid_upgrade_strategy_causes_error(script): + """ + It errors out when the upgrade-strategy is an invalid/unrecognised one + + """ + result = script.pip_install_local( + '--upgrade', '--upgrade-strategy=bazinga', 'simple', + expect_error=True + ) + + assert result.returncode + assert "upgrade strategy provided: 'bazinga'" in result.stderr + + +def test_non_eager_does_not_upgrade_dependecies_if_existing_version_satisfies(script): + """ + It doesn't upgrade a dependency if it already satisfies the requirements. + + """ + script.pip_install_local('simple==2.0', expect_error=True) + result = script.pip_install_local( + '--upgrade', '--upgrade-strategy=non-eager', 'require_simple', + expect_error=True + ) + + assert ( + (script.site_packages / 'require_simple-1.0-py%s.egg-info' % pyversion) + not in result.files_deleted + ), "should have installed require_simple==1.0" + assert ( + (script.site_packages / 'simple-2.0-py%s.egg-info' % pyversion) + not in result.files_deleted + ), "should not have uninstalled simple==2.0" + + +def test_non_eager_upgrade_dependecies_if_existing_version_does_not_satisfy(script): + """ + It does upgrade a dependency if it already satisfies the requirements. + + """ + script.pip_install_local('simple==1.0', expect_error=True) + result = script.pip_install_local( + '--upgrade', '--upgrade-strategy=non-eager', 'require_simple', + expect_error=True + ) + + assert ( + (script.site_packages / 'require_simple-1.0-py%s.egg-info' % pyversion) + not in result.files_deleted + ), "should have installed require_simple==1.0" + assert ( + script.site_packages / 'simple-3.0-py%s.egg-info' % + pyversion in result.files_created + ), "should have installed simple==3.0" + assert ( + script.site_packages / 'simple-1.0-py%s.egg-info' % + pyversion in result.files_deleted + ), "should have uninstalled simple==1.0" + + +def test_eager_does_upgrade_dependecies_if_existing_version_satisfies(script): + """ + It doesn't upgrade a dependency if it already satisfies the requirements. + + """ + script.pip_install_local('simple==2.0', expect_error=True) + result = script.pip_install_local( + '--upgrade', '--upgrade-strategy=eager', 'require_simple', + expect_error=True + ) + + assert ( + (script.site_packages / 'require_simple-1.0-py%s.egg-info' % pyversion) + not in result.files_deleted + ), "should have installed require_simple==1.0" + assert ( + (script.site_packages / 'simple-2.0-py%s.egg-info' % pyversion) + in result.files_deleted + ), "should have uninstalled simple==2.0" + + +def test_eager_upgrade_dependecies_if_existing_version_does_not_satisfy(script): + """ + It does upgrade a dependency if it already satisfies the requirements. + + """ + script.pip_install_local('simple==1.0', expect_error=True) + result = script.pip_install_local( + '--upgrade', '--upgrade-strategy=eager', 'require_simple', + expect_error=True + ) + + assert ( + (script.site_packages / 'require_simple-1.0-py%s.egg-info' % pyversion) + not in result.files_deleted + ), "should have installed require_simple==1.0" + assert ( + script.site_packages / 'simple-3.0-py%s.egg-info' % + pyversion in result.files_created + ), "should have installed simple==3.0" + assert ( + script.site_packages / 'simple-1.0-py%s.egg-info' % + pyversion in result.files_deleted + ), "should have uninstalled simple==1.0" + + @pytest.mark.network def test_upgrade_to_specific_version(script): """ From 61d8d6ab270cd5f96db5c87dcee3411f5a32f082 Mon Sep 17 00:00:00 2001 From: "Pradyun S. Gedam" Date: Sat, 17 Sep 2016 10:58:10 +0530 Subject: [PATCH 2/7] Shorten test names, correct descriptions --- tests/functional/test_install_upgrade.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/functional/test_install_upgrade.py b/tests/functional/test_install_upgrade.py index 9d98eabea08..c101bba8ffe 100644 --- a/tests/functional/test_install_upgrade.py +++ b/tests/functional/test_install_upgrade.py @@ -37,7 +37,7 @@ def test_invalid_upgrade_strategy_causes_error(script): assert "upgrade strategy provided: 'bazinga'" in result.stderr -def test_non_eager_does_not_upgrade_dependecies_if_existing_version_satisfies(script): +def test_non_eager_does_not_upgrade_dependecies_when_satisfied(script): """ It doesn't upgrade a dependency if it already satisfies the requirements. @@ -58,9 +58,9 @@ def test_non_eager_does_not_upgrade_dependecies_if_existing_version_satisfies(sc ), "should not have uninstalled simple==2.0" -def test_non_eager_upgrade_dependecies_if_existing_version_does_not_satisfy(script): +def test_non_eager_does_upgrade_dependecies_when_no_longer_satisfied(script): """ - It does upgrade a dependency if it already satisfies the requirements. + It does upgrade a dependency if it no longer satisfies the requirements. """ script.pip_install_local('simple==1.0', expect_error=True) @@ -83,9 +83,9 @@ def test_non_eager_upgrade_dependecies_if_existing_version_does_not_satisfy(scri ), "should have uninstalled simple==1.0" -def test_eager_does_upgrade_dependecies_if_existing_version_satisfies(script): +def test_eager_does_upgrade_dependecies_when_currently_satisfied(script): """ - It doesn't upgrade a dependency if it already satisfies the requirements. + It does upgrade a dependency even if it already satisfies the requirements. """ script.pip_install_local('simple==2.0', expect_error=True) @@ -104,9 +104,9 @@ def test_eager_does_upgrade_dependecies_if_existing_version_satisfies(script): ), "should have uninstalled simple==2.0" -def test_eager_upgrade_dependecies_if_existing_version_does_not_satisfy(script): +def test_eager_does_upgrade_dependecies_when_no_longer_satisfied(script): """ - It does upgrade a dependency if it already satisfies the requirements. + It does upgrade a dependency if it no longer satisfies the requirements. """ script.pip_install_local('simple==1.0', expect_error=True) From 25d9866742206de604b09a65710c60168bf85527 Mon Sep 17 00:00:00 2001 From: "Pradyun S. Gedam" Date: Sat, 17 Sep 2016 23:56:45 +0530 Subject: [PATCH 3/7] Delegate the checking of strategy to option parser --- pip/commands/install.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/pip/commands/install.py b/pip/commands/install.py index f25535ae5a0..b88bcb165d9 100644 --- a/pip/commands/install.py +++ b/pip/commands/install.py @@ -96,6 +96,7 @@ def __init__(self, *args, **kw): '--upgrade-strategy', dest='upgrade_strategy', default='non-eager', + choices=['non-eager', 'eager'], help='Determines how dependency upgrading should be handled. ' '"eager" - dependencies are upgraded regardless of ' 'whether the currently installed version satisfies the ' @@ -236,14 +237,6 @@ def run(self, options, args): if options.build_dir: options.build_dir = os.path.abspath(options.build_dir) - allowed_strategies = ["eager", "non-eager"] - if options.upgrade_strategy not in allowed_strategies: - raise CommandError( - "pip does not know upgrade strategy provided: '%s'" % ( - options.upgrade_strategy, - ) - ) - options.src_dir = os.path.abspath(options.src_dir) install_options = options.install_options or [] if options.use_user_site: From 09fe69912f5bbc4cd4ef538f10b0506524db70f7 Mon Sep 17 00:00:00 2001 From: "Pradyun S. Gedam" Date: Sat, 17 Sep 2016 23:56:57 +0530 Subject: [PATCH 4/7] Switch the default upgrade strategy --- pip/commands/install.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pip/commands/install.py b/pip/commands/install.py index b88bcb165d9..f5e862d4199 100644 --- a/pip/commands/install.py +++ b/pip/commands/install.py @@ -95,7 +95,7 @@ def __init__(self, *args, **kw): cmd_opts.add_option( '--upgrade-strategy', dest='upgrade_strategy', - default='non-eager', + default='eager', choices=['non-eager', 'eager'], help='Determines how dependency upgrading should be handled. ' '"eager" - dependencies are upgraded regardless of ' From 6550631b50fc9d51b1cf9572c52908a09983b47e Mon Sep 17 00:00:00 2001 From: "Pradyun S. Gedam" Date: Sun, 18 Sep 2016 11:16:48 +0530 Subject: [PATCH 5/7] Correct the test for change in error message --- tests/functional/test_install_upgrade.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/functional/test_install_upgrade.py b/tests/functional/test_install_upgrade.py index c101bba8ffe..7ac0b8f7a5f 100644 --- a/tests/functional/test_install_upgrade.py +++ b/tests/functional/test_install_upgrade.py @@ -34,7 +34,7 @@ def test_invalid_upgrade_strategy_causes_error(script): ) assert result.returncode - assert "upgrade strategy provided: 'bazinga'" in result.stderr + assert "invalid choice" in result.stderr def test_non_eager_does_not_upgrade_dependecies_when_satisfied(script): From 21c98efee5caff74e33c4b635b12a1880be1202e Mon Sep 17 00:00:00 2001 From: "Pradyun S. Gedam" Date: Sun, 18 Sep 2016 16:59:48 +0530 Subject: [PATCH 6/7] Rename non-eager to only-if-needed --- pip/commands/install.py | 6 +++--- pip/req/req_set.py | 4 ++-- tests/functional/test_install_upgrade.py | 8 ++++---- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/pip/commands/install.py b/pip/commands/install.py index f5e862d4199..9a90e6f0ac3 100644 --- a/pip/commands/install.py +++ b/pip/commands/install.py @@ -96,13 +96,13 @@ def __init__(self, *args, **kw): '--upgrade-strategy', dest='upgrade_strategy', default='eager', - choices=['non-eager', 'eager'], + choices=['only-if-needed', 'eager'], help='Determines how dependency upgrading should be handled. ' '"eager" - dependencies are upgraded regardless of ' 'whether the currently installed version satisfies the ' 'requirements of the upgraded package(s). ' - '"non-eager" - are upgraded only when they do not satisfy ' - 'the requirements of the upgraded package(s).' + '"only-if-needed" - are upgraded only when they do not ' + 'satisfy the requirements of the upgraded package(s).' ) cmd_opts.add_option( diff --git a/pip/req/req_set.py b/pip/req/req_set.py index e149a802838..85f8de595d1 100644 --- a/pip/req/req_set.py +++ b/pip/req/req_set.py @@ -381,7 +381,7 @@ def prepare_files(self, finder): def _is_upgrade_allowed(self, req): return self.upgrade and ( self.upgrade_strategy == "eager" or ( - self.upgrade_strategy == "non-eager" and req.is_direct + self.upgrade_strategy == "only-if-needed" and req.is_direct ) ) @@ -440,7 +440,7 @@ def _check_skip_installed(self, req_to_install, finder): # Figure out a nice message to say why we're skipping this. if best_installed: skip_reason = 'already up-to-date' - elif self.upgrade_strategy == "non-eager": + elif self.upgrade_strategy == "only-if-needed": skip_reason = 'not upgraded as not directly required' else: skip_reason = 'already satisfied' diff --git a/tests/functional/test_install_upgrade.py b/tests/functional/test_install_upgrade.py index 7ac0b8f7a5f..10c95717a31 100644 --- a/tests/functional/test_install_upgrade.py +++ b/tests/functional/test_install_upgrade.py @@ -37,14 +37,14 @@ def test_invalid_upgrade_strategy_causes_error(script): assert "invalid choice" in result.stderr -def test_non_eager_does_not_upgrade_dependecies_when_satisfied(script): +def test_only_if_needed_does_not_upgrade_deps_when_satisfied(script): """ It doesn't upgrade a dependency if it already satisfies the requirements. """ script.pip_install_local('simple==2.0', expect_error=True) result = script.pip_install_local( - '--upgrade', '--upgrade-strategy=non-eager', 'require_simple', + '--upgrade', '--upgrade-strategy=only-if-needed', 'require_simple', expect_error=True ) @@ -58,14 +58,14 @@ def test_non_eager_does_not_upgrade_dependecies_when_satisfied(script): ), "should not have uninstalled simple==2.0" -def test_non_eager_does_upgrade_dependecies_when_no_longer_satisfied(script): +def test_only_if_needed_does_upgrade_deps_when_no_longer_satisfied(script): """ It does upgrade a dependency if it no longer satisfies the requirements. """ script.pip_install_local('simple==1.0', expect_error=True) result = script.pip_install_local( - '--upgrade', '--upgrade-strategy=non-eager', 'require_simple', + '--upgrade', '--upgrade-strategy=only-if-needed', 'require_simple', expect_error=True ) From 932ecd2dd1e38d23a4ac24643137db531733667e Mon Sep 17 00:00:00 2001 From: "Pradyun S. Gedam" Date: Sun, 18 Sep 2016 17:00:07 +0530 Subject: [PATCH 7/7] Remove the repetitive "already" --- pip/req/req_set.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pip/req/req_set.py b/pip/req/req_set.py index 85f8de595d1..aca8c40efd9 100644 --- a/pip/req/req_set.py +++ b/pip/req/req_set.py @@ -485,7 +485,7 @@ def _prepare_file(self, 'req_to_install.satisfied_by is set to %r' % (req_to_install.satisfied_by,)) logger.info( - 'Requirement already %s: %s', skip_reason, + 'Requirement %s: %s', skip_reason, req_to_install) else: if (req_to_install.link and