Skip to content

Make -U non-recursive by default, add --upgrade-recursive option #571

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
17 changes: 13 additions & 4 deletions pip/commands/install.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,12 @@ def __init__(self):
'-U', '--upgrade',
dest='upgrade',
action='store_true',
help='Upgrade all packages to the newest available version')
help='Upgrade packages to the latest version, but not dependencies')
self.parser.add_option(
'-R', '--upgrade-recursive',
dest='upgrade_recursive',
action='store_true',
help='Upgrade packages to the latest version, including dependencies')
self.parser.add_option(
'--force-reinstall',
dest='force_reinstall',
Expand Down Expand Up @@ -216,23 +221,27 @@ def run(self, options, args):

finder = self._build_package_finder(options, index_urls)

upgrade = options.upgrade or options.upgrade_recursive

requirement_set = RequirementSet(
build_dir=options.build_dir,
src_dir=options.src_dir,
download_dir=options.download_dir,
download_cache=options.download_cache,
upgrade=options.upgrade,
upgrade=upgrade,
upgrade_recursive=options.upgrade_recursive,
as_egg=options.as_egg,
ignore_installed=options.ignore_installed,
ignore_dependencies=options.ignore_dependencies,
force_reinstall=options.force_reinstall,
use_user_site=options.use_user_site)

for name in args:
requirement_set.add_requirement(
InstallRequirement.from_line(name, None))
InstallRequirement.from_line(name, None, upgrade=upgrade))
for name in options.editables:
requirement_set.add_requirement(
InstallRequirement.from_editable(name, default_vcs=options.default_vcs))
InstallRequirement.from_editable(name, default_vcs=options.default_vcs, upgrade=upgrade))
for filename in options.requirements:
for req in parse_requirements(filename, finder=finder, options=options):
requirement_set.add_requirement(req)
Expand Down
68 changes: 42 additions & 26 deletions pip/req.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
class InstallRequirement(object):

def __init__(self, req, comes_from, source_dir=None, editable=False,
url=None, as_egg=False, update=True):
url=None, as_egg=False, update=True, upgrade=False):
self.extras = ()
if isinstance(req, string_types):
req = pkg_resources.Requirement.parse(req)
Expand All @@ -60,29 +60,32 @@ def __init__(self, req, comes_from, source_dir=None, editable=False,
self._is_bundle = None
# True if the editable should be updated:
self.update = update
# True if the requirement should be upgraded to the latest version
self.upgrade = upgrade
# Set to True after successful installation
self.install_succeeded = None
# UninstallPathSet of uninstalled distribution (for possible rollback)
self.uninstalled = None
self.use_user_site = False

@classmethod
def from_editable(cls, editable_req, comes_from=None, default_vcs=None):
def from_editable(cls, editable_req, comes_from=None, default_vcs=None, upgrade=False):
name, url, extras_override = parse_editable(editable_req, default_vcs)
if url.startswith('file:'):
source_dir = url_to_path(url)
else:
source_dir = None

res = cls(name, comes_from, source_dir=source_dir, editable=True, url=url)
res = cls(name, comes_from, source_dir=source_dir, editable=True,
url=url, upgrade=upgrade)

if extras_override is not None:
res.extras = extras_override

return res

@classmethod
def from_line(cls, name, comes_from=None):
def from_line(cls, name, comes_from=None, upgrade=False):
"""Creates an InstallRequirement from a name, which might be a
requirement, directory containing 'setup.py', filename, or URL.
"""
Expand Down Expand Up @@ -116,7 +119,7 @@ def from_line(cls, name, comes_from=None):
else:
req = name

return cls(req, comes_from, url=url)
return cls(req, comes_from, url=url, upgrade=upgrade)

def __str__(self):
if self.req:
Expand Down Expand Up @@ -710,7 +713,7 @@ def is_bundle(self):
or os.path.exists(os.path.join(base, 'pyinstall-manifest.txt')))
return self._is_bundle

def bundle_requirements(self):
def bundle_requirements(self, upgrade=False):
for dest_dir in self._bundle_editable_dirs:
package = os.path.basename(dest_dir)
## FIXME: svnism:
Expand All @@ -730,13 +733,11 @@ def bundle_requirements(self):
else:
url = None
yield InstallRequirement(
package, self, editable=True, url=url,
update=False, source_dir=dest_dir)
package, self, editable=True, url=url, update=False,
source_dir=dest_dir, upgrade=upgrade)
for dest_dir in self._bundle_build_dirs:
package = os.path.basename(dest_dir)
yield InstallRequirement(
package, self,
source_dir=dest_dir)
yield InstallRequirement(package, self, source_dir=dest_dir, upgrade=upgrade)

def move_bundle_files(self, dest_build_dir, dest_src_dir):
base = self._temp_build_dir
Expand Down Expand Up @@ -812,13 +813,15 @@ def __repr__(self):
class RequirementSet(object):

def __init__(self, build_dir, src_dir, download_dir, download_cache=None,
upgrade=False, ignore_installed=False, as_egg=False,
ignore_dependencies=False, force_reinstall=False, use_user_site=False):
ignore_installed=False, as_egg=False, force_reinstall=False,
ignore_dependencies=False, use_user_site=False, upgrade=False,
upgrade_recursive=False):
self.build_dir = build_dir
self.src_dir = src_dir
self.download_dir = download_dir
self.download_cache = download_cache
self.upgrade = upgrade
self.upgrade_recursive = upgrade_recursive
self.ignore_installed = ignore_installed
self.force_reinstall = force_reinstall
self.requirements = Requirements()
Expand Down Expand Up @@ -921,9 +924,13 @@ def locate_files(self):
else:
install_needed = False
if req_to_install.satisfied_by:
if self.upgrade and not self.upgrade_recursive:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow this logic?
it seems like all we need to notify is this:
'Requirement already satisfied (use --upgrade to upgrade): %s' % req_to_install)
and if so, you don't need to pass in in upgrade as a property of RequirementSet, just upgrade_recursive

Copy link
Author

Choose a reason for hiding this comment

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

I believe this message displays after any installation where the requirement is currently satisfied, not just upgrades. So if you performed

pip install Django

and you had Django 1.2.3 installed, you would see:

Requirement already satisfied (use --upgrade to upgrade): Django in [...]

whereas if Django was a dependency in install_requires that was satisfied but would be force-upgraded with --upgrade-recursive, it would instead display:

Requirement already satisfied (use --upgrade-recursive to upgrade): Django>1.2 in [...]

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I take that message just as an FYI to the user about how to upgrade an individual package, that was satisfied during the current installation, not as a tip on how to redo the last installation command to get certain packages upgraded.

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense. I suppose the reason I did it this way is that with the previous behavior a pip install --upgrade never presented this message, whereas now it can and it struck me as confusing that the user would be directed to perform a --upgrade during a --upgrade (the source of the confusion here being the ambiguity of the argument to pip install)

Copy link
Author

Choose a reason for hiding this comment

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

An upside of presenting the (use --upgrade-recursive to upgrade) notice is that it might ease confusion for people expecting the previous pip install -U behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I see your pt of view. maybe a 3rd person can help decide what is more intuitive in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nice to see the tests confirm the exptected notifications

upgrade_cmd = '--upgrade-recursive'
else:
upgrade_cmd = '--upgrade'
logger.notify('Requirement already satisfied '
'(use --upgrade to upgrade): %s'
% req_to_install)
'(use %s to upgrade): %s'
% (upgrade_cmd, req_to_install))

if req_to_install.editable:
if req_to_install.source_dir is None:
Expand Down Expand Up @@ -953,11 +960,11 @@ def prepare_files(self, finder, force_root_egg_info=False, bundle=False):
if not self.ignore_installed and not req_to_install.editable:
req_to_install.check_if_exists()
if req_to_install.satisfied_by:
if self.upgrade:
if req_to_install.upgrade:
if not self.force_reinstall and not req_to_install.url:
try:
url = finder.find_requirement(
req_to_install, self.upgrade)
req_to_install, upgrade=True)
except BestVersionAlreadyInstalled:
best_installed = True
install = False
Expand All @@ -979,9 +986,13 @@ def prepare_files(self, finder, force_root_egg_info=False, bundle=False):
logger.notify('Requirement already up-to-date: %s'
% req_to_install)
else:
if self.upgrade and not self.upgrade_recursive:
upgrade_cmd = '--upgrade-recursive'
else:
upgrade_cmd = '--upgrade'
logger.notify('Requirement already satisfied '
'(use --upgrade to upgrade): %s'
% req_to_install)
'(use %s to upgrade): %s'
% (upgrade_cmd, req_to_install))
if req_to_install.editable:
logger.notify('Obtaining %s' % req_to_install)
elif install:
Expand Down Expand Up @@ -1023,7 +1034,7 @@ def prepare_files(self, finder, force_root_egg_info=False, bundle=False):
if req_to_install.url is None:
if not_found:
raise not_found
url = finder.find_requirement(req_to_install, upgrade=self.upgrade)
url = finder.find_requirement(req_to_install, upgrade=req_to_install.upgrade)
else:
## FIXME: should req_to_install.url already be a link?
url = Link(req_to_install.url)
Expand All @@ -1044,7 +1055,7 @@ def prepare_files(self, finder, force_root_egg_info=False, bundle=False):
is_bundle = req_to_install.is_bundle
if is_bundle:
req_to_install.move_bundle_files(self.build_dir, self.src_dir)
for subreq in req_to_install.bundle_requirements():
for subreq in req_to_install.bundle_requirements(upgrade=self.upgrade_recursive):
reqs.append(subreq)
self.add_requirement(subreq)
elif self.is_download:
Expand All @@ -1068,7 +1079,7 @@ def prepare_files(self, finder, force_root_egg_info=False, bundle=False):
# repeat check_if_exists to uninstall-on-upgrade (#14)
req_to_install.check_if_exists()
if req_to_install.satisfied_by:
if self.upgrade or self.ignore_installed:
if req_to_install.upgrade or self.ignore_installed:
#don't uninstall conflict if user install and and conflict is not user install
if not (self.use_user_site and not dist_in_usersite(req_to_install.satisfied_by)):
req_to_install.conflicts_with = req_to_install.satisfied_by
Expand All @@ -1092,7 +1103,7 @@ def prepare_files(self, finder, force_root_egg_info=False, bundle=False):
if self.has_requirement(name):
## FIXME: check for conflict
continue
subreq = InstallRequirement(req, req_to_install)
subreq = InstallRequirement(req, req_to_install, upgrade=self.upgrade_recursive)
reqs.append(subreq)
self.add_requirement(subreq)
if not self.has_requirement(req_to_install.name):
Expand Down Expand Up @@ -1344,16 +1355,21 @@ def parse_requirements(filename, finder=None, comes_from=None, options=None):
elif line.startswith('--no-index'):
finder.index_urls = []
else:
# Use getattr since uninstall doesn't set these options
upgrade = (getattr(options, 'upgrade', False)
or getattr(options, 'upgrade_recursive', False))
comes_from = '-r %s (line %s)' % (filename, line_number)
if line.startswith('-e') or line.startswith('--editable'):
if line.startswith('-e'):
line = line[2:].strip()
else:
line = line[len('--editable'):].strip().lstrip('=')
req = InstallRequirement.from_editable(
line, comes_from=comes_from, default_vcs=options.default_vcs)
req = InstallRequirement.from_editable(line,
comes_from=comes_from,
default_vcs=options.default_vcs,
upgrade=upgrade)
else:
req = InstallRequirement.from_line(line, comes_from)
req = InstallRequirement.from_line(line, comes_from, upgrade=upgrade)
yield req


Expand Down
1 change: 1 addition & 0 deletions tests/packages/FSPkgUsesInitools/fspkg/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#
25 changes: 25 additions & 0 deletions tests/packages/FSPkgUsesInitools/setup.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
from setuptools import setup, find_packages

version = '0.1'

setup(name='FSPkgUsesInitools',
version=version,
description="File system test package",
long_description="""\
File system test package""",
classifiers=[], # Get strings from http://pypi.python.org/pypi?%3Aaction=list_classifiers
keywords='pip tests',
author='pip',
author_email='[email protected]',
url='http://pip.openplans.org',
license='',
packages=find_packages(exclude=['ez_setup', 'examples', 'tests']),
include_package_data=True,
zip_safe=False,
install_requires=[
"INItools",
],
entry_points="""
# -*- Entry points: -*-
""",
)
1 change: 1 addition & 0 deletions tests/packages/FSPkgUsesNewishInitools/fspkg/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#
25 changes: 25 additions & 0 deletions tests/packages/FSPkgUsesNewishInitools/setup.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
from setuptools import setup, find_packages

version = '0.1'

setup(name='FSPkgUsesNewishInitools',
version=version,
description="File system test package",
long_description="""\
File system test package""",
classifiers=[], # Get strings from http://pypi.python.org/pypi?%3Aaction=list_classifiers
keywords='pip tests',
author='pip',
author_email='[email protected]',
url='http://pip.openplans.org',
license='',
packages=find_packages(exclude=['ez_setup', 'examples', 'tests']),
include_package_data=True,
zip_safe=False,
install_requires=[
"INItools>=0.3",
],
entry_points="""
# -*- Entry points: -*-
""",
)
6 changes: 6 additions & 0 deletions tests/test_pip.py
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,12 @@ def run(self, *args, **kw):
assert not isinstance(cwd, Path)
return TestPipResult(super(TestPipEnvironment, self).run(cwd=cwd, *args, **kw), verbose=self.verbose)

def get_pkg_version(self, pkg_name):
result = self.run('python', '-c',
"from pkg_resources import get_distribution; "
"print(get_distribution('%s').version)" % pkg_name)
return result.stdout.strip()

def __del__(self):
rmtree(str(self.root_path), ignore_errors=True)

Expand Down
Loading