-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Split find_matches into generation and sorting #8234
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,8 @@ | |
|
||
|
||
class Resolver(BaseResolver): | ||
_allowed_strategies = {"eager", "only-if-needed", "to-satisfy-only"} | ||
|
||
def __init__( | ||
self, | ||
preparer, # type: RequirementPreparer | ||
|
@@ -47,30 +49,27 @@ def __init__( | |
py_version_info=None, # type: Optional[Tuple[int, ...]] | ||
): | ||
super(Resolver, self).__init__() | ||
|
||
assert upgrade_strategy in self._allowed_strategies | ||
|
||
self.factory = Factory( | ||
finder=finder, | ||
preparer=preparer, | ||
make_install_req=make_install_req, | ||
force_reinstall=force_reinstall, | ||
ignore_installed=ignore_installed, | ||
ignore_requires_python=ignore_requires_python, | ||
upgrade_strategy=upgrade_strategy, | ||
py_version_info=py_version_info, | ||
) | ||
self.ignore_dependencies = ignore_dependencies | ||
self.upgrade_strategy = upgrade_strategy | ||
self._result = None # type: Optional[Result] | ||
|
||
def resolve(self, root_reqs, check_supported_wheels): | ||
# type: (List[InstallRequirement], bool) -> RequirementSet | ||
|
||
# The factory should not have retained state from any previous usage. | ||
# In theory this could only happen if self was reused to do a second | ||
# resolve, which isn't something we do at the moment. We assert here | ||
# in order to catch the issue if that ever changes. | ||
# The persistent state that we care about is `root_reqs`. | ||
assert len(self.factory.root_reqs) == 0, "Factory is being re-used" | ||
|
||
constraints = {} # type: Dict[str, SpecifierSet] | ||
roots = set() | ||
pradyunsg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
requirements = [] | ||
for req in root_reqs: | ||
if req.constraint: | ||
|
@@ -82,6 +81,8 @@ def resolve(self, root_reqs, check_supported_wheels): | |
else: | ||
constraints[name] = req.specifier | ||
else: | ||
if req.is_direct and req.name: | ||
roots.add(canonicalize_name(req.name)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if ther would be edge cases where an “unnamed” requirement messes up priority logic later. Would it be a good idea if we resolve all names eagerly here? We have to do it soon for resolvelib anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly, I'm not sure. I'm really cautious about preparing too early, because it feels so easy to end up with another mess like canonical names1, where we keep going "let's resolve names now, because we don't know if it's a problem". That's why I like the new candidate approach of having a lazy property. Hopefully when we switch over to the project model, projects will do the same. I'd like to understand why we need to resolve the name of an unnamed requirement here, if we do need to (we're talking about 1 I added that |
||
requirements.append( | ||
self.factory.make_requirement_from_install_req(req) | ||
) | ||
|
@@ -90,6 +91,8 @@ def resolve(self, root_reqs, check_supported_wheels): | |
factory=self.factory, | ||
constraints=constraints, | ||
ignore_dependencies=self.ignore_dependencies, | ||
upgrade_strategy=self.upgrade_strategy, | ||
roots=roots, | ||
) | ||
reporter = BaseReporter() | ||
resolver = RLResolver(provider, reporter) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it’s better to add an
is_installed()
on candidates… or even abstract out thekeep_installed()
function into candidate subclasses.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't abstract out
keep_installed()
out since it depends on theupgrade_strategy
.+1 on adding
is_installed()
-- although, as a read-only property feels a lot cleaner.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered that, but wasn't sure if it was too intrusive (it felt a bit specialised). But as both of you seem OK with the idea, I'll do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I made
is_installed
an attribute on the class, and added an equivalent property to theCandidate
class. That seems to work, although mypy failed to catch that I forgot to add the attribute toRequiresPythonCandidate
. I understand why (there's a default implementation inCandidate
) but is there really no way to say in mypy "every subclass of this class needs to have an attributeis_installed
"? Or to say "every object of typeCandidate
must have anis_instance
attribute" and have class attributes count?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that’s what Protocol is for, but it’s a pain to use Protocol on comment-style annotations. Python 2 😞