From d77b5c234cdfb1c605bef22f3975fea191efe913 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Fri, 17 Jul 2020 03:23:28 +0530 Subject: [PATCH 01/14] Refactor the logging calls into a dedicated loop --- src/pip/_internal/commands/install.py | 29 ++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 454c0b70c..bf3d07bde 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -539,24 +539,39 @@ class InstallCommand(RequirementCommand): def _warn_about_conflicts(self, conflict_details): # type: (ConflictDetails) -> None package_set, (missing, conflicting) = conflict_details + parts = [] # type: List[str] # 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]: - logger.critical( - "%s %s requires %s, which is not installed.", - project_name, version, dependency[1], + message = ( + "{name} {version} requires {requirement}, " + "which is not installed." + ).format( + name=project_name, + version=version, + requirement=dependency[1], ) + parts.append(message) for project_name in conflicting: version = package_set[project_name][0] for dep_name, dep_version, req in conflicting[project_name]: - logger.critical( - "%s %s has requirement %s, but you'll have %s %s which is " - "incompatible.", - project_name, version, req, dep_name, dep_version, + message = ( + "{name} {version} requires {requirement}, but you'll have " + "{dep_name} {dep_version} which is incompatible." + ).format( + name=project_name, + version=version, + requirement=req, + dep_name=dep_name, + dep_version=dep_version, ) + parts.append(message) + + for message in parts: + logger.critical(message) def get_lib_location_guesses( From 42c62a08f7cf5d7debfd84d4358ccf5a0ae8f583 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Fri, 17 Jul 2020 04:01:46 +0530 Subject: [PATCH 02/14] Short circuit when there's nothing to report --- src/pip/_internal/commands/install.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index bf3d07bde..9edababe4 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -539,6 +539,9 @@ class InstallCommand(RequirementCommand): def _warn_about_conflicts(self, conflict_details): # type: (ConflictDetails) -> None package_set, (missing, conflicting) = conflict_details + if not missing and not conflicting: + return + parts = [] # type: List[str] # NOTE: There is some duplication here, with commands/check.py From efdb66ed16d6794b04a1f0acf3e11bdb495b888c Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Fri, 17 Jul 2020 06:11:48 +0530 Subject: [PATCH 03/14] Add messaging variation based on "new resolver" usage --- src/pip/_internal/commands/install.py | 35 +++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 9edababe4..2aa9a34e7 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -444,7 +444,10 @@ class InstallCommand(RequirementCommand): items.append(item) if conflicts is not None: - self._warn_about_conflicts(conflicts) + self._warn_about_conflicts( + conflicts, + new_resolver='2020-resolver' in options.features_enabled, + ) installed_desc = ' '.join(items) if installed_desc: @@ -536,13 +539,36 @@ class InstallCommand(RequirementCommand): ) return None - def _warn_about_conflicts(self, conflict_details): - # type: (ConflictDetails) -> None + def _warn_about_conflicts(self, conflict_details, new_resolver): + # type: (ConflictDetails, bool) -> None package_set, (missing, conflicting) = conflict_details if not missing and not conflicting: return parts = [] # type: List[str] + if new_resolver: + # NOTE: trailing newlines here are intentional + parts.append( + "Pip will install or upgrade your package(s) and its " + "dependencies without taking into account other packages you " + "already have installed. This may cause an uncaught " + "dependency conflict.\n" + ) + parts.append( + "If you would like pip to take your other packages into " + "account, please tell us here: https://forms.gle/cWKMoDs8sUVE29hz9\n" + ) + else: + parts.append( + "After October 2020 you may experience errors when installing " + "or updating packages. This is because pip will change the " + "way that it resolves dependency conflicts.\n" + ) + parts.append( + "We recommend you use --use-feature=2020-resolver to test " + "your packages with the new resolver before it becomes the " + "default.\n" + ) # NOTE: There is some duplication here, with commands/check.py for project_name in missing: @@ -573,8 +599,7 @@ class InstallCommand(RequirementCommand): ) parts.append(message) - for message in parts: - logger.critical(message) + logger.critical("\n".join(parts)) def get_lib_location_guesses( From 43da7c82834d524247124f1477852c870e887263 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Fri, 17 Jul 2020 06:22:08 +0530 Subject: [PATCH 04/14] Make the YAML tests happier --- tests/yaml/conflict_1.yml | 7 ++----- tests/yaml/conflicting_triangle.yml | 4 +--- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/tests/yaml/conflict_1.yml b/tests/yaml/conflict_1.yml index 847bb2b6a..df1c6818b 100644 --- a/tests/yaml/conflict_1.yml +++ b/tests/yaml/conflict_1.yml @@ -11,12 +11,9 @@ cases: response: - error: code: 0 - stderr: ['requirement', 'is\s+incompatible'] + stderr: ['dependency', 'incompatible'] skip: old - # -- currently the error message is: - # a 1.0.0 has requirement B==1.0.0, but you'll have b 2.0.0 which is - # incompatible. - # -- better would be: + # -- a good error message would be: # A 1.0.0 has incompatible requirements B==1.0.0, B==2.0.0 - diff --git a/tests/yaml/conflicting_triangle.yml b/tests/yaml/conflicting_triangle.yml index 666c37363..e8e88b347 100644 --- a/tests/yaml/conflicting_triangle.yml +++ b/tests/yaml/conflicting_triangle.yml @@ -14,7 +14,5 @@ cases: - C 1.0.0 - error: code: 0 - stderr: ['requirement c==1\.0\.0', 'is incompatible'] + stderr: ['c==1\.0\.0', 'incompatible'] skip: old - # -- currently the error message is: - # a 1.0.0 has requirement C==1.0.0, but you'll have c 2.0.0 which is incompatible. From 8db354260af5fb9de7135fc808fbab954401f03f Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Mon, 27 Jul 2020 13:29:00 +0530 Subject: [PATCH 05/14] Move the form link to make the linter happy --- src/pip/_internal/commands/install.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 2aa9a34e7..3739e20df 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -554,9 +554,10 @@ class InstallCommand(RequirementCommand): "already have installed. This may cause an uncaught " "dependency conflict.\n" ) + form_link = "https://forms.gle/cWKMoDs8sUVE29hz9" parts.append( "If you would like pip to take your other packages into " - "account, please tell us here: https://forms.gle/cWKMoDs8sUVE29hz9\n" + "account, please tell us here: {}\n".format(form_link) ) else: parts.append( From 4e4951066d369ebb1923e50ecb87493012492b3d Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Mon, 27 Jul 2020 13:52:54 +0530 Subject: [PATCH 06/14] Reverse if statement's condition --- src/pip/_internal/commands/install.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 3739e20df..3cefa5ae6 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -546,7 +546,18 @@ class InstallCommand(RequirementCommand): return parts = [] # type: List[str] - if new_resolver: + if not new_resolver: + parts.append( + "After October 2020 you may experience errors when installing " + "or updating packages. This is because pip will change the " + "way that it resolves dependency conflicts.\n" + ) + parts.append( + "We recommend you use --use-feature=2020-resolver to test " + "your packages with the new resolver before it becomes the " + "default.\n" + ) + else: # NOTE: trailing newlines here are intentional parts.append( "Pip will install or upgrade your package(s) and its " @@ -559,17 +570,6 @@ class InstallCommand(RequirementCommand): "If you would like pip to take your other packages into " "account, please tell us here: {}\n".format(form_link) ) - else: - parts.append( - "After October 2020 you may experience errors when installing " - "or updating packages. This is because pip will change the " - "way that it resolves dependency conflicts.\n" - ) - parts.append( - "We recommend you use --use-feature=2020-resolver to test " - "your packages with the new resolver before it becomes the " - "default.\n" - ) # NOTE: There is some duplication here, with commands/check.py for project_name in missing: From 1b2ae22e7b0b2d1963bec769d4f070bfbc6d932a Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Mon, 27 Jul 2020 14:13:59 +0530 Subject: [PATCH 07/14] Don't print that form link after the end of month. --- src/pip/_internal/commands/install.py | 3 ++- src/pip/_internal/utils/datetime.py | 12 ++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 src/pip/_internal/utils/datetime.py diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 3cefa5ae6..8c2c32fd4 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -21,6 +21,7 @@ from pip._internal.locations import distutils_scheme from pip._internal.operations.check import check_install_conflicts from pip._internal.req import install_given_reqs from pip._internal.req.req_tracker import get_requirement_tracker +from pip._internal.utils.datetime import today_is_later_than from pip._internal.utils.deprecation import deprecated from pip._internal.utils.distutils_args import parse_distutils_args from pip._internal.utils.filesystem import test_writable_dir @@ -557,7 +558,7 @@ class InstallCommand(RequirementCommand): "your packages with the new resolver before it becomes the " "default.\n" ) - else: + elif not today_is_later_than(year=2020, month=7, day=31): # NOTE: trailing newlines here are intentional parts.append( "Pip will install or upgrade your package(s) and its " diff --git a/src/pip/_internal/utils/datetime.py b/src/pip/_internal/utils/datetime.py new file mode 100644 index 000000000..b638646c8 --- /dev/null +++ b/src/pip/_internal/utils/datetime.py @@ -0,0 +1,12 @@ +"""For when pip wants to check the date or time. +""" + +import datetime + + +def today_is_later_than(year, month, day): + # type: (int, int, int) -> bool + today = datetime.date.today() + given = datetime.date(year, month, day) + + return today > given From a89ede7da3ed1ae332095811877559443c0fc49b Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Mon, 27 Jul 2020 19:22:15 +0530 Subject: [PATCH 08/14] tests: Check only the last output lines --- tests/functional/test_check.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/functional/test_check.py b/tests/functional/test_check.py index 4e9a144bf..5cb41a97e 100644 --- a/tests/functional/test_check.py +++ b/tests/functional/test_check.py @@ -3,9 +3,11 @@ from tests.lib import create_test_package_with_setup def matches_expected_lines(string, expected_lines): # Ignore empty lines - output_lines = set(filter(None, string.splitlines())) - # Match regardless of order - return set(output_lines) == set(expected_lines) + output_lines = list(filter(None, string.splitlines())) + # We'll match the last n lines, given n lines to match. + last_few_output_lines = output_lines[-len(expected_lines):] + # And order does not matter + return set(last_few_output_lines) == set(expected_lines) def test_basic_check_clean(script): From e4f8e0a2b87bd3ad90fc883b6133512acbf9a0c8 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Mon, 27 Jul 2020 19:23:59 +0530 Subject: [PATCH 09/14] Change {matches -> contains}_expected_lines This also updates invocations that can't be translated as-is into a different equivalent check. --- tests/functional/test_install_check.py | 27 +++++++++----------------- 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/tests/functional/test_install_check.py b/tests/functional/test_install_check.py index 017d6256b..d13e93f5d 100644 --- a/tests/functional/test_install_check.py +++ b/tests/functional/test_install_check.py @@ -1,10 +1,7 @@ from tests.lib import create_test_package_with_setup -def matches_expected_lines(string, expected_lines, exact=True): - if exact: - return set(string.splitlines()) == set(expected_lines) - # If not exact, check that all expected lines are present +def contains_expected_lines(string, expected_lines): return set(expected_lines) <= set(string.splitlines()) @@ -41,8 +38,7 @@ def test_check_install_canonicalization(script, deprecated_python): "ERROR: pkga 1.0 requires SPECIAL.missing, which is not installed.", ] # Deprecated python versions produce an extra warning on stderr - assert matches_expected_lines( - result.stderr, expected_lines, exact=not deprecated_python) + assert contains_expected_lines(result.stderr, expected_lines) assert result.returncode == 0 # Install the second missing package and expect that there is no warning @@ -51,8 +47,7 @@ def test_check_install_canonicalization(script, deprecated_python): result = script.pip( 'install', '--no-index', special_path, '--quiet', ) - assert matches_expected_lines( - result.stderr, [], exact=not deprecated_python) + assert "requires" not in result.stderr assert result.returncode == 0 # Double check that all errors are resolved in the end @@ -60,7 +55,7 @@ def test_check_install_canonicalization(script, deprecated_python): expected_lines = [ "No broken requirements found.", ] - assert matches_expected_lines(result.stdout, expected_lines) + assert contains_expected_lines(result.stdout, expected_lines) assert result.returncode == 0 @@ -85,33 +80,29 @@ def test_check_install_does_not_warn_for_out_of_graph_issues( # Install a package without it's dependencies result = script.pip('install', '--no-index', pkg_broken_path, '--no-deps') - # Deprecated python versions produce an extra warning on stderr - assert matches_expected_lines( - result.stderr, [], exact=not deprecated_python) + assert "requires" not in result.stderr # Install conflict package result = script.pip( 'install', '--no-index', pkg_conflict_path, allow_stderr_error=True, ) - assert matches_expected_lines(result.stderr, [ - "ERROR: broken 1.0 requires missing, which is not installed.", + assert contains_expected_lines(result.stderr, [ ( "ERROR: broken 1.0 has requirement conflict<1.0, but " "you'll have conflict 1.0 which is incompatible." ), - ], exact=not deprecated_python) + ]) # Install unrelated package result = script.pip( 'install', '--no-index', pkg_unrelated_path, '--quiet', ) # should not warn about broken's deps when installing unrelated package - assert matches_expected_lines( - result.stderr, [], exact=not deprecated_python) + assert "requires" not in result.stderr result = script.pip('check', expect_error=True) expected_lines = [ "broken 1.0 requires missing, which is not installed.", "broken 1.0 has requirement conflict<1.0, but you have conflict 1.0.", ] - assert matches_expected_lines(result.stdout, expected_lines) + assert contains_expected_lines(result.stdout, expected_lines) From c631de61b9ac2517bbe3b7f469f7ba59e80af437 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Mon, 27 Jul 2020 19:24:47 +0530 Subject: [PATCH 10/14] Drop no-longer-used deprecated_python fixture --- tests/functional/test_install_check.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/functional/test_install_check.py b/tests/functional/test_install_check.py index d13e93f5d..060af80ef 100644 --- a/tests/functional/test_install_check.py +++ b/tests/functional/test_install_check.py @@ -5,7 +5,7 @@ def contains_expected_lines(string, expected_lines): return set(expected_lines) <= set(string.splitlines()) -def test_check_install_canonicalization(script, deprecated_python): +def test_check_install_canonicalization(script): pkga_path = create_test_package_with_setup( script, name='pkgA', @@ -59,8 +59,7 @@ def test_check_install_canonicalization(script, deprecated_python): assert result.returncode == 0 -def test_check_install_does_not_warn_for_out_of_graph_issues( - script, deprecated_python): +def test_check_install_does_not_warn_for_out_of_graph_issues(script): pkg_broken_path = create_test_package_with_setup( script, name='broken', From b9ff93f7ba59cb49cb4ebe4bb5aa929bd2f7fbca Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Mon, 27 Jul 2020 19:25:19 +0530 Subject: [PATCH 11/14] Update test messages to reflect new reality --- tests/functional/test_install_check.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/functional/test_install_check.py b/tests/functional/test_install_check.py index 060af80ef..88609422b 100644 --- a/tests/functional/test_install_check.py +++ b/tests/functional/test_install_check.py @@ -35,7 +35,7 @@ def test_check_install_canonicalization(script): allow_stderr_error=True, ) expected_lines = [ - "ERROR: pkga 1.0 requires SPECIAL.missing, which is not installed.", + "pkga 1.0 requires SPECIAL.missing, which is not installed.", ] # Deprecated python versions produce an extra warning on stderr assert contains_expected_lines(result.stderr, expected_lines) @@ -86,8 +86,9 @@ def test_check_install_does_not_warn_for_out_of_graph_issues(script): 'install', '--no-index', pkg_conflict_path, allow_stderr_error=True, ) assert contains_expected_lines(result.stderr, [ + "broken 1.0 requires missing, which is not installed.", ( - "ERROR: broken 1.0 has requirement conflict<1.0, but " + "broken 1.0 requires conflict<1.0, but " "you'll have conflict 1.0 which is incompatible." ), ]) From 3962f9d2b89a0bdb02bbf6c4d820f93e75353876 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Mon, 27 Jul 2020 19:44:28 +0530 Subject: [PATCH 12/14] Moar tests getting updated --- tests/functional/test_install.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index 2657a8aee..2185251d2 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -1697,7 +1697,7 @@ def test_install_conflict_results_in_warning(script, data): result2 = script.pip( 'install', '--no-index', pkgB_path, allow_stderr_error=True, ) - assert "pkga 1.0 has requirement pkgb==1.0" in result2.stderr, str(result2) + assert "pkga 1.0 requires pkgb==1.0" in result2.stderr, str(result2) assert "Successfully installed pkgB-2.0" in result2.stdout, str(result2) From 3f88476e4fd046f572e7f5cc39d7fcdff328cb04 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Mon, 27 Jul 2020 20:06:55 +0530 Subject: [PATCH 13/14] This counts as a fix right? --- tests/yaml/conflict_1.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/yaml/conflict_1.yml b/tests/yaml/conflict_1.yml index df1c6818b..c0a561995 100644 --- a/tests/yaml/conflict_1.yml +++ b/tests/yaml/conflict_1.yml @@ -11,7 +11,7 @@ cases: response: - error: code: 0 - stderr: ['dependency', 'incompatible'] + stderr: ['incompatible'] skip: old # -- a good error message would be: # A 1.0.0 has incompatible requirements B==1.0.0, B==2.0.0 From 9033824cbcc4b6ed0cc29d0f4fbd3af721121695 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Tue, 28 Jul 2020 11:23:39 +0530 Subject: [PATCH 14/14] Python 2 *sigh* --- src/pip/_internal/utils/datetime.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/pip/_internal/utils/datetime.py b/src/pip/_internal/utils/datetime.py index b638646c8..4d0503c2f 100644 --- a/src/pip/_internal/utils/datetime.py +++ b/src/pip/_internal/utils/datetime.py @@ -1,6 +1,8 @@ """For when pip wants to check the date or time. """ +from __future__ import absolute_import + import datetime