Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an upgrade-strategy option #3972

Merged
merged 7 commits into from
Sep 18, 2016
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions pip/commands/install.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,21 @@ 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='eager',
choices=['non-eager', 'eager'],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to consider whether "non-eager" is clear to a user. The original proposal of "only-if-needed" is not much longer. That's what the pip docs call it now: https://pip.pypa.io/en/latest/user_guide/#only-if-needed-recursive-upgrade

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I'll make the change.

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(
Expand Down Expand Up @@ -278,6 +291,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,
Expand Down
45 changes: 35 additions & 10 deletions pip/req/req_set.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -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()
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.

Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't upgrade_allowed always be true here (line 414)? I guess the same was true of self.upgrade before, though...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered the same initially. Then I decided to keep it as it was with upgrade.

except BestVersionAlreadyInstalled:
skip_reason = 'up-to-date'
best_installed = True
except DistributionNotFound:
# No distribution found, so we squash the
Expand All @@ -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'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gets appended to "Requirement already" instead of to "Requirement". Now it looks a bit weird: Requirement already not upgraded as not directly required

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and same two lines above, gives Requirement already already up-to-date

Copy link
Member Author

@pradyunsg pradyunsg Sep 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that line. 😅 I thought I had changed it...

else:
skip_reason = 'already satisfied'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can't happen - if the above two clauses are false, then the latest version is not installed and strategy is eager. Hence can't skip but need to upgrade.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think about it, yes. This is a need-to-upgrade. I don't know what reasoning I was applying earlier for this...

Copy link
Member Author

@pradyunsg pradyunsg Sep 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... I did that because of the assert on line 484. And because I could never hit that branch in any of my testing earlier, for the previous PR.


return skip_reason
else:
return None
Expand Down Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions tests/data/packages/README.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Binary file added tests/data/packages/require_simple-1.0.tar.gz
Binary file not shown.
106 changes: 106 additions & 0 deletions tests/functional/test_install_upgrade.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 "invalid choice" in result.stderr


def test_non_eager_does_not_upgrade_dependecies_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',
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_does_upgrade_dependecies_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',
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_when_currently_satisfied(script):
"""
It does upgrade a dependency even 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_does_upgrade_dependecies_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=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):
"""
Expand Down