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.

Sign up for GitHub

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

Make message more user friendly when unable to resolve #8033

Merged
merged 3 commits into from
Apr 15, 2020
Merged

Make message more user friendly when unable to resolve #8033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3642589
Make message more user friendly when unable to resolve
pfmoore Apr 13, 2020
ea391b2
Merge with master
pfmoore Apr 14, 2020
8c118c8
Fix the test to check for canonical name
pfmoore Apr 14, 2020
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
4 changes: 4 additions & 0 deletions src/pip/_internal/resolution/resolvelib/requirements.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ def __init__(self, ireq, factory):
self._factory = factory
self.extras = ireq.req.extras

def __str__(self):
# type: () -> str
return str(self._ireq.req)

def __repr__(self):
# type: () -> str
return "{class_name}({requirement!r})".format(
Expand Down
22 changes: 22 additions & 0 deletions src/pip/_internal/resolution/resolvelib/resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import functools
import logging

from pip._vendor import six
from pip._vendor.packaging.utils import canonicalize_name
Expand All @@ -25,6 +26,9 @@
from pip._internal.resolution.base import InstallRequirementProvider


logger = logging.getLogger(__name__)


class Resolver(BaseResolver):
def __init__(
self,
Expand Down Expand Up @@ -74,9 +78,27 @@ def resolve(self, root_reqs, check_supported_wheels):

try:
self._result = resolver.resolve(requirements)

except ResolutionImpossible as e:
error = self.factory.get_installation_error(e)
if not error:
# TODO: This needs fixing, we need to look at the
# factory.get_installation_error infrastructure, as that
# doesn't really allow for the logger.critical calls I'm
# using here.
for req, parent in e.causes:
logger.critical(
"Could not find a version that satisfies " +
"the requirement " +
str(req) +
("" if parent is None else " (from {})".format(
parent.name
))
)
raise InstallationError(
"No matching distribution found for " +
", ".join([r.name for r, _ in e.causes])
)
Copy link
Member

Choose a reason for hiding this comment

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

Does this output something like:

Could not find a version that satisfies the requirement Django>2.0 (...)
Could not find a version that satisfies the requirement Django<=1.11 (...)

This could feel confusing to users :| Maybe we can do:

Could not find a version that satisfies the requirement:
    Django>2.0 (...)
    Django<=1.11 (...)

Copy link
Member Author

Choose a reason for hiding this comment

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

It might, and yes that would probably be better. My initial concern was mainly to not get a traceback for pip install misspelled_thing, preferably keeping the output as near to the current output as we can (we don't have the information in the exception to include "(from versions: xxx)" at the moment).

I'm happy to iterate on this - I guess my main question right now is whether we want to get something into the beta, or if we can live with "proper messages" being something for the post-20.1 improvements?

Also, the question of how closely we want to match the current output ties into this. @ei8fdb was pretty strongly of the view that we should try to get as close as possible, so I was working on trying to replicate current output - and that's a case I haven't been able to replicate (probably because the current resolver's "first come, first served" basis means that it never hits that situation.

Copy link
Member

Choose a reason for hiding this comment

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

I am definitely in favour of having something for the release. This PR as-is would br much better than a cruel ResolutionImpossible.

raise
six.raise_from(error, e)

Expand Down
35 changes: 35 additions & 0 deletions tests/functional/test_new_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,41 @@ def test_new_resolver_installs_extras(script):
assert_installed(script, base="0.1.0", dep="0.1.0")


def test_new_resolver_installed_message(script):
create_basic_wheel_for_package(script, "A", "1.0")
result = script.pip(
"install", "--unstable-feature=resolver",
"--no-cache-dir", "--no-index",
"--find-links", script.scratch_path,
"A",
expect_stderr=False,
)
assert "Successfully installed A-1.0" in result.stdout, str(result)


def test_new_resolver_no_dist_message(script):
create_basic_wheel_for_package(script, "A", "1.0")
result = script.pip(
"install", "--unstable-feature=resolver",
"--no-cache-dir", "--no-index",
"--find-links", script.scratch_path,
"B",
expect_error=True,
expect_stderr=True,
)

# Full messages from old resolver:
# ERROR: Could not find a version that satisfies the
# requirement xxx (from versions: none)
# ERROR: No matching distribution found for xxx

assert "Could not find a version that satisfies the requirement B" \
in result.stderr, str(result)
# TODO: This reports the canonical name of the project. But the current
# resolver reports the originally specified name (i.e. uppercase B)
assert "No matching distribution found for b" in result.stderr, str(result)


def test_new_resolver_installs_editable(script):
create_basic_wheel_for_package(
script,
Expand Down