From e3e916be8dcf3443c3bd081c9b1bf38534ab27e3 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Thu, 16 Jul 2020 01:38:36 +0530 Subject: [PATCH 1/5] Add a dedicated type for check_install_conflicts --- src/pip/_internal/operations/check.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/operations/check.py b/src/pip/_internal/operations/check.py index 87c265dada5..5714915bcb2 100644 --- a/src/pip/_internal/operations/check.py +++ b/src/pip/_internal/operations/check.py @@ -29,6 +29,7 @@ MissingDict = Dict[str, List[Missing]] ConflictingDict = Dict[str, List[Conflicting]] CheckResult = Tuple[MissingDict, ConflictingDict] + ConflictDetails = Tuple[PackageSet, CheckResult] PackageDetails = namedtuple('PackageDetails', ['version', 'requires']) @@ -99,7 +100,7 @@ def check_package_set(package_set, should_ignore=None): def check_install_conflicts(to_install): - # type: (List[InstallRequirement]) -> Tuple[PackageSet, CheckResult] + # type: (List[InstallRequirement]) -> ConflictDetails """For checking if the dependency graph would be consistent after \ installing given requirements """ From 67cbd0ca18ccc68a1b87e322154d9718a11c73a6 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Thu, 16 Jul 2020 13:03:26 +0530 Subject: [PATCH 2/5] Break up pip install's "conflict check" function Making this into two functions allows for separating the "check" and "print warnings" step in a follow up commit. --- src/pip/_internal/commands/install.py | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index b2459dfe14b..1dcd702745c 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -40,6 +40,7 @@ from typing import Iterable, List, Optional from pip._internal.models.format_control import FormatControl + from pip._internal.operations.check import ConflictDetails from pip._internal.req.req_install import InstallRequirement from pip._internal.wheel_builder import BinaryAllowedPredicate @@ -371,13 +372,14 @@ def run(self, options, args): requirement_set ) - # Consistency Checking of the package set we're installing. + # Check for conflicts in the package set we're installing. should_warn_about_conflicts = ( not options.ignore_dependencies and options.warn_about_conflicts ) if should_warn_about_conflicts: - self._warn_about_conflicts(to_install) + conflicts = self._determine_conflicts(to_install) + self._warn_about_conflicts(conflicts) # Don't warn about script install locations if # --target has been specified @@ -498,14 +500,22 @@ def _handle_target_dir(self, target_dir, target_temp_dir, upgrade): target_item_dir ) - def _warn_about_conflicts(self, to_install): - # type: (List[InstallRequirement]) -> None + def _determine_conflicts(self, to_install): + # type: (List[InstallRequirement]) -> Optional[ConflictDetails] try: - package_set, _dep_info = check_install_conflicts(to_install) + conflict_details = check_install_conflicts(to_install) except Exception: - logger.error("Error checking for conflicts.", exc_info=True) - return - missing, conflicting = _dep_info + logger.error( + "Error while checking for conflicts. Please file an issue on " + "pip's issue tracker: https://github.com/pypa/pip/issues/new", + exc_info=True + ) + return None + return conflict_details + + def _warn_about_conflicts(self, conflict_details): + # type: (ConflictDetails) -> None + package_set, (missing, conflicting) = conflict_details # NOTE: There is some duplication here from pip check for project_name in missing: From de741fa0dda3683fcb868b0583cb8e0c54ba454b Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Thu, 16 Jul 2020 13:05:51 +0530 Subject: [PATCH 3/5] Clearly note where code duplication exists The duplication of this code isn't really that bad, but saying "pip check" makes it ambigous which file is relevant. Changing to reference the exact filename makes this clearer. --- src/pip/_internal/commands/install.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 1dcd702745c..416e8be4220 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -517,7 +517,7 @@ def _warn_about_conflicts(self, conflict_details): # type: (ConflictDetails) -> None package_set, (missing, conflicting) = conflict_details - # NOTE: There is some duplication here from pip check + # NOTE: There is some duplication here, with commands/check.py for project_name in missing: version = package_set[project_name][0] for dependency in missing[project_name]: From eafbec5aa6c169c41975a4d3327a5286b26fa128 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Thu, 16 Jul 2020 13:08:32 +0530 Subject: [PATCH 4/5] Move conflict warning to just-before success message This is a much better location for these errors, since they're in a much more visible spot. We've had reports in the past of users missing these messages, and changing where we present these warnings should help resolve that issue. We do lose the ability for an advanced user to potentially see the warning and abort installation before the conflicts are introduced, but given that we don't even pause for input, I don't think that's a strong argument and neither do I view this as necessary. --- src/pip/_internal/commands/install.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 416e8be4220..c89f4836e20 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -373,13 +373,13 @@ def run(self, options, args): ) # Check for conflicts in the package set we're installing. + conflicts = None # type: Optional[ConflictDetails] should_warn_about_conflicts = ( not options.ignore_dependencies and options.warn_about_conflicts ) if should_warn_about_conflicts: conflicts = self._determine_conflicts(to_install) - self._warn_about_conflicts(conflicts) # Don't warn about script install locations if # --target has been specified @@ -421,6 +421,10 @@ def run(self, options, args): except Exception: pass items.append(item) + + if conflicts is not None: + self._warn_about_conflicts(conflicts) + installed_desc = ' '.join(items) if installed_desc: write_output( From 7ddbcc2e67889b53a98aa115dfc01542a39c00ce Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Fri, 17 Jul 2020 03:25:53 +0530 Subject: [PATCH 5/5] Return early for clarity --- src/pip/_internal/commands/install.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index c89f4836e20..0b507a30a74 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -507,7 +507,7 @@ def _handle_target_dir(self, target_dir, target_temp_dir, upgrade): def _determine_conflicts(self, to_install): # type: (List[InstallRequirement]) -> Optional[ConflictDetails] try: - conflict_details = check_install_conflicts(to_install) + return check_install_conflicts(to_install) except Exception: logger.error( "Error while checking for conflicts. Please file an issue on " @@ -515,7 +515,6 @@ def _determine_conflicts(self, to_install): exc_info=True ) return None - return conflict_details def _warn_about_conflicts(self, conflict_details): # type: (ConflictDetails) -> None