From 782913725fde4804004d65013f9b7fca7282767e Mon Sep 17 00:00:00 2001 From: Devesh Kumar Singh Date: Wed, 15 Apr 2020 19:06:31 +0530 Subject: [PATCH 1/6] Canonicalize req name while doing pre-install package search --- news/5021.bugfix | 1 + src/pip/_internal/req/req_install.py | 8 ++++++-- 2 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 news/5021.bugfix diff --git a/news/5021.bugfix b/news/5021.bugfix new file mode 100644 index 00000000000..4de60b18e76 --- /dev/null +++ b/news/5021.bugfix @@ -0,0 +1 @@ +Package name should be normalized before we use it to search if it's already installed or not diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py index 7538c9546ca..f58a24d74c1 100644 --- a/src/pip/_internal/req/req_install.py +++ b/src/pip/_internal/req/req_install.py @@ -428,12 +428,16 @@ def check_if_exists(self, use_user_site): """ if self.req is None: return + + # Canonicalize requirement name to use normalized + # names while searching for already installed packages + no_marker = Requirement(str(self.req)) + no_marker.marker = None + no_marker.name = canonicalize_name(no_marker.name) # get_distribution() will resolve the entire list of requirements # anyway, and we've already determined that we need the requirement # in question, so strip the marker so that we don't try to # evaluate it. - no_marker = Requirement(str(self.req)) - no_marker.marker = None try: self.satisfied_by = pkg_resources.get_distribution(str(no_marker)) except pkg_resources.DistributionNotFound: From 40261a475ff8bdfba6f1557f1ea4d9ab7cf75b89 Mon Sep 17 00:00:00 2001 From: Devesh Kumar Singh Date: Wed, 15 Apr 2020 23:00:37 +0530 Subject: [PATCH 2/6] Add unit tests to verify pkg name normalization --- tests/functional/test_install.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index cb291a61ef6..cf63d0dc3d4 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -1882,3 +1882,20 @@ def test_install_skip_work_dir_pkg(script, data): assert 'Requirement already satisfied: simple' not in result.stdout assert 'Successfully installed simple' in result.stdout + + +def test_install_verify_package_name_normalization(script): + """ + Test that install of a package again using a name which + normalizes to the original package name, is a no-op + since the package is already installed + """ + pkg_path = create_test_package_with_setup( + script, name='simple-package', version='1.0') + result = script.pip('install', '-e', '.', + expect_stderr=True, cwd=pkg_path) + assert 'Successfully installed simple-package' in result.stdout + + result = script.pip('install', 'simple.package') + assert 'Requirement already satisfied: simple.package' in result.stdout + From 2bdec6c9fd6cf18ceddbfc5b3911455c3e4c34b7 Mon Sep 17 00:00:00 2001 From: Devesh Kumar Singh Date: Thu, 16 Apr 2020 00:01:53 +0530 Subject: [PATCH 3/6] Parametrize unit test --- tests/functional/test_install.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index cf63d0dc3d4..2cff6ca2229 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -1884,7 +1884,10 @@ def test_install_skip_work_dir_pkg(script, data): assert 'Successfully installed simple' in result.stdout -def test_install_verify_package_name_normalization(script): +@pytest.mark.parametrize('package_name', ('simple-package', 'simple_package', + 'simple.package')) +def test_install_verify_package_name_normalization(script, package_name): + """ Test that install of a package again using a name which normalizes to the original package name, is a no-op @@ -1896,6 +1899,6 @@ def test_install_verify_package_name_normalization(script): expect_stderr=True, cwd=pkg_path) assert 'Successfully installed simple-package' in result.stdout - result = script.pip('install', 'simple.package') - assert 'Requirement already satisfied: simple.package' in result.stdout - + result = script.pip('install', package_name) + assert 'Requirement already satisfied: {}'.format( + package_name) in result.stdout From 04fedfe53ccf3a9dde0f7646d204b4acda23fc25 Mon Sep 17 00:00:00 2001 From: Devesh Kumar Singh Date: Fri, 15 May 2020 00:14:15 +0530 Subject: [PATCH 4/6] Create custom get_distribution function --- src/pip/_internal/commands/search.py | 6 ++-- src/pip/_internal/req/req_install.py | 23 ++++++++-------- src/pip/_internal/self_outdated_check.py | 14 ++++++---- src/pip/_internal/utils/misc.py | 35 ++++++++++++++++++++++++ tests/functional/test_search.py | 3 +- tests/unit/test_self_check_outdated.py | 3 +- 6 files changed, 62 insertions(+), 22 deletions(-) diff --git a/src/pip/_internal/commands/search.py b/src/pip/_internal/commands/search.py index 3e75254812a..c01d96a2473 100644 --- a/src/pip/_internal/commands/search.py +++ b/src/pip/_internal/commands/search.py @@ -19,7 +19,8 @@ from pip._internal.network.xmlrpc import PipXmlrpcTransport from pip._internal.utils.compat import get_terminal_size from pip._internal.utils.logging import indent_log -from pip._internal.utils.misc import write_output + +from pip._internal.utils.misc import get_distribution, write_output from pip._internal.utils.typing import MYPY_CHECK_RUNNING if MYPY_CHECK_RUNNING: @@ -31,6 +32,7 @@ {'name': str, 'summary': str, 'versions': List[str]}, ) + logger = logging.getLogger(__name__) @@ -139,7 +141,7 @@ def print_results(hits, name_column_width=None, terminal_width=None): try: write_output(line) if name in installed_packages: - dist = pkg_resources.get_distribution(name) + dist = get_distribution(name) with indent_log(): if dist.version == latest: write_output('INSTALLED: %s (latest)', dist.version) diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py index f58a24d74c1..2aa853642c5 100644 --- a/src/pip/_internal/req/req_install.py +++ b/src/pip/_internal/req/req_install.py @@ -41,6 +41,7 @@ display_path, dist_in_site_packages, dist_in_usersite, + get_distribution, get_installed_version, hide_url, redact_auth_from_url, @@ -429,21 +430,23 @@ def check_if_exists(self, use_user_site): if self.req is None: return - # Canonicalize requirement name to use normalized - # names while searching for already installed packages - no_marker = Requirement(str(self.req)) - no_marker.marker = None - no_marker.name = canonicalize_name(no_marker.name) # get_distribution() will resolve the entire list of requirements # anyway, and we've already determined that we need the requirement # in question, so strip the marker so that we don't try to # evaluate it. + no_marker = Requirement(str(self.req)) + no_marker.marker = None + + # pkg_resources uses the canonical name to look up packages, but + # the name passed passed to get_distribution is not canonicalized + # so we have to explicitly convert it to a canonical name + no_marker.name = canonicalize_name(no_marker.name) try: self.satisfied_by = pkg_resources.get_distribution(str(no_marker)) except pkg_resources.DistributionNotFound: return except pkg_resources.VersionConflict: - existing_dist = pkg_resources.get_distribution( + existing_dist = get_distribution( self.req.name ) if use_user_site: @@ -683,13 +686,11 @@ def uninstall(self, auto_confirm=False, verbose=False): """ assert self.req - try: - dist = pkg_resources.get_distribution(self.req.name) - except pkg_resources.DistributionNotFound: + dist = get_distribution(self.req.name) + if not dist: logger.warning("Skipping %s as it is not installed.", self.name) return None - else: - logger.info('Found existing installation: %s', dist) + logger.info('Found existing installation: %s', dist) uninstalled_pathset = UninstallPathSet.from_dist(dist) uninstalled_pathset.remove(auto_confirm, verbose) diff --git a/src/pip/_internal/self_outdated_check.py b/src/pip/_internal/self_outdated_check.py index ec5df3af105..fbd9dfd48b7 100644 --- a/src/pip/_internal/self_outdated_check.py +++ b/src/pip/_internal/self_outdated_check.py @@ -7,7 +7,6 @@ import os.path import sys -from pip._vendor import pkg_resources from pip._vendor.packaging import version as packaging_version from pip._vendor.six import ensure_binary @@ -19,7 +18,11 @@ check_path_owner, replace, ) -from pip._internal.utils.misc import ensure_dir, get_installed_version +from pip._internal.utils.misc import ( + ensure_dir, + get_distribution, + get_installed_version, +) from pip._internal.utils.packaging import get_installer from pip._internal.utils.typing import MYPY_CHECK_RUNNING @@ -110,11 +113,10 @@ def was_installed_by_pip(pkg): This is used not to display the upgrade message when pip is in fact installed by system package manager, such as dnf on Fedora. """ - try: - dist = pkg_resources.get_distribution(pkg) - return "pip" == get_installer(dist) - except pkg_resources.DistributionNotFound: + dist = get_distribution(pkg) + if not dist: return False + return "pip" == get_installer(dist) def pip_self_version_check(session, options): diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index 50a86982e4e..b0a7320a072 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -20,6 +20,7 @@ from pip._vendor import pkg_resources # NOTE: retrying is not annotated in typeshed as on 2017-07-17, which is # why we ignore the type on this import. +from pip._vendor.packaging.utils import canonicalize_name from pip._vendor.retrying import retry # type: ignore from pip._vendor.six import PY2, text_type from pip._vendor.six.moves import input, map, zip_longest @@ -480,6 +481,40 @@ def user_test(d): ] +def search_distribution(req_name): + + # Canonicalize the name before searching in the list of + # installed distributions and also while creating the package + # dictionary to get the Distribution object + req_name = canonicalize_name(req_name) + packages = get_installed_distributions(skip=()) + pkg_dict = {canonicalize_name(p.key): p for p in packages} + return pkg_dict.get(req_name) + + +def get_distribution(req_name): + """Given a requirement name, return the installed Distribution object""" + + # Search the distribution by looking through the working set + dist = search_distribution(req_name) + + # If distribution could not be found, call working_set.require + # to update the working set, and try to find the distribution + # again. + # This might happen for e.g. when you install a package + # twice, once using setup.py develop and again using setup.py install. + # Now when run pip uninstall twice, the package gets removed + # from the working set in the first uninstall, so we have to populate + # the working set again so that pip knows about it and the packages + # gets picked up and is successfully uninstalled the second time too. + if not dist: + try: + pkg_resources.working_set.require(req_name) + except pkg_resources.DistributionNotFound: + return None + return search_distribution(req_name) + + def egg_link_path(dist): # type: (Distribution) -> Optional[str] """ diff --git a/tests/functional/test_search.py b/tests/functional/test_search.py index fce6c5f819c..5918b4f64f9 100644 --- a/tests/functional/test_search.py +++ b/tests/functional/test_search.py @@ -168,7 +168,8 @@ def test_latest_prerelease_install_message(caplog, monkeypatch): dist = pretend.stub(version="1.0.0") get_dist = pretend.call_recorder(lambda x: dist) - monkeypatch.setattr("pip._vendor.pkg_resources.get_distribution", get_dist) + monkeypatch.setattr("pip._internal.commands.search.get_distribution", + get_dist) with caplog.at_level(logging.INFO): print_results(hits) diff --git a/tests/unit/test_self_check_outdated.py b/tests/unit/test_self_check_outdated.py index f33e319cf78..c5e60d92fc4 100644 --- a/tests/unit/test_self_check_outdated.py +++ b/tests/unit/test_self_check_outdated.py @@ -6,7 +6,6 @@ import freezegun import pretend import pytest -from pip._vendor import pkg_resources from pip._internal import self_outdated_check from pip._internal.models.candidate import InstallationCandidate @@ -98,7 +97,7 @@ def test_pip_self_version_check(monkeypatch, stored_time, installed_ver, pretend.call_recorder(lambda *a, **kw: None)) monkeypatch.setattr(logger, 'debug', pretend.call_recorder(lambda s, exc_info=None: None)) - monkeypatch.setattr(pkg_resources, 'get_distribution', + monkeypatch.setattr(self_outdated_check, 'get_distribution', lambda name: MockDistribution(installer)) fake_state = pretend.stub( From ac624f1e4f34037bf199c112780f889e0d0a072e Mon Sep 17 00:00:00 2001 From: Devesh Kumar Singh Date: Sat, 23 May 2020 18:59:38 +0530 Subject: [PATCH 5/6] Reword news entry --- news/5021.bugfix | 2 +- src/pip/_internal/commands/search.py | 2 -- src/pip/_internal/req/req_install.py | 1 - 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/news/5021.bugfix b/news/5021.bugfix index 4de60b18e76..36606fd20f5 100644 --- a/news/5021.bugfix +++ b/news/5021.bugfix @@ -1 +1 @@ -Package name should be normalized before we use it to search if it's already installed or not +Use canonical package names while looking up already installed packages. diff --git a/src/pip/_internal/commands/search.py b/src/pip/_internal/commands/search.py index c01d96a2473..e906ce7667f 100644 --- a/src/pip/_internal/commands/search.py +++ b/src/pip/_internal/commands/search.py @@ -19,7 +19,6 @@ from pip._internal.network.xmlrpc import PipXmlrpcTransport from pip._internal.utils.compat import get_terminal_size from pip._internal.utils.logging import indent_log - from pip._internal.utils.misc import get_distribution, write_output from pip._internal.utils.typing import MYPY_CHECK_RUNNING @@ -32,7 +31,6 @@ {'name': str, 'summary': str, 'versions': List[str]}, ) - logger = logging.getLogger(__name__) diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py index 2aa853642c5..644930a1528 100644 --- a/src/pip/_internal/req/req_install.py +++ b/src/pip/_internal/req/req_install.py @@ -429,7 +429,6 @@ def check_if_exists(self, use_user_site): """ if self.req is None: return - # get_distribution() will resolve the entire list of requirements # anyway, and we've already determined that we need the requirement # in question, so strip the marker so that we don't try to From 6c1030ca95f22e6111d1307e6cf9a032d7a063e7 Mon Sep 17 00:00:00 2001 From: Devesh Kumar Singh Date: Mon, 6 Jul 2020 19:17:26 +0530 Subject: [PATCH 6/6] Fix comment order for retrying module --- src/pip/_internal/utils/misc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index b0a7320a072..7cf9944fdd3 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -18,9 +18,9 @@ from collections import deque from pip._vendor import pkg_resources +from pip._vendor.packaging.utils import canonicalize_name # NOTE: retrying is not annotated in typeshed as on 2017-07-17, which is # why we ignore the type on this import. -from pip._vendor.packaging.utils import canonicalize_name from pip._vendor.retrying import retry # type: ignore from pip._vendor.six import PY2, text_type from pip._vendor.six.moves import input, map, zip_longest