Skip to content

New Resolver: Add support for direct URLs with extras #8291

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

Merged
merged 6 commits into from
May 22, 2020
Merged

New Resolver: Add support for direct URLs with extras #8291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
775f9ff
Add a helper for making ExplicitRequirement objects
pradyunsg May 21, 2020
95347df
Directly require BaseCandidate in ExtrasCandidate
pradyunsg May 21, 2020
9506a28
ExtrasCandidate depends on exact base, before optional dependencies
pradyunsg May 21, 2020
901898c
Make mypy happy
pradyunsg May 21, 2020
24e4cf7
Update comments and documentation
pradyunsg May 21, 2020
50c9ea2
This works now!
pradyunsg May 21, 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
15 changes: 6 additions & 9 deletions src/pip/_internal/resolution/resolvelib/candidates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,8 +347,8 @@ class ExtrasCandidate(Candidate):
to treat it as a separate node in the dependency graph.
2. When we're getting the candidate's dependencies,
a) We specify that we want the extra dependencies as well.
b) We add a dependency on the base candidate (matching the name and
version). See below for why this is needed.
b) We add a dependency on the base candidate.
See below for why this is needed.
3. We return None for the underlying InstallRequirement, as the base
candidate will provide it, and we don't want to end up with duplicates.

Expand Down Expand Up @@ -417,20 +417,17 @@ def iter_dependencies(self):
extra
)

# Add a dependency on the exact base
# (See note 2b in the class docstring)
yield factory.make_requirement_from_candidate(self.base)
Copy link
Member

Choose a reason for hiding this comment

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

This would clone a pkg>=1.0 without pinning the version if base is not pinned. I think we need to introduce some kind of abstraction to BaseCandidate to handle this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I follow. self.base is a candidate.

Copy link
Member

Choose a reason for hiding this comment

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

I sort of recall there’s a thing that may go wrong with this, but I forgot what that is now :( Maybe it’s no longer a problem with the new find_matches() implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, there is a different problem if constraints are involved. We don’t allow constraints to filter direct requirements, so given the following scenario:

# constraints.txt
foo>=1.0

# requirements.txt
foo[add]==1.0

This will fail with installation from path or url cannot be constrained to a version because we are adding a URL requirement to represent the base.

I’m thinking maybe we should get rid of the check altogether, since constraining a URL requirement by checking the version the URL points to kind of still makes sense? @pfmoore

Copy link
Member

Choose a reason for hiding this comment

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

I think that's probably true. I suspect the limitation was there because the old resolver couldn't cope with that situation.

But if we relax that restriction, we'll get additional test failures, and I think we should start being much stricter about not allowing regressions like that (I've just submitted #8297 which should help a bit here, and the fact that the new markers mean that the test suite now succeeds on the new resolver will help as well) so we'd need to fix those tests at the same time (and not by just adding the "fails with the new resolver" flag 🙂 - maybe an xfail on the new resolver?).

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it’s no longer a problem with the new find_matches() implementation.

I think it was a much bigger problem before we had the new implementation, but maybe not so much now. Let's assume it works for the moment, and then when stuff breaks, we'll have a better idea of where there's still an issue (and we can blame @pradyunsg 🙂)

Copy link
Member Author

Choose a reason for hiding this comment

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

we can blame @pradyunsg

Oh no. 😮


for r in self.base.dist.requires(valid_extras):
requirement = factory.make_requirement_from_spec_matching_extras(
str(r), self.base._ireq, valid_extras,
)
if requirement:
yield requirement

# Add a dependency on the exact base.
# (See note 2b in the class docstring)
# FIXME: This does not work if the base candidate is specified by
# link, e.g. "pip install .[dev]" will fail.
spec = "{}=={}".format(self.base.name, self.base.version)
yield factory.make_requirement_from_spec(spec, self.base._ireq)

def get_install_requirement(self):
# type: () -> Optional[InstallRequirement]
# We don't return anything here, because we always
Expand Down
8 changes: 6 additions & 2 deletions src/pip/_internal/resolution/resolvelib/factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,12 +180,16 @@ def make_requirement_from_install_req(self, ireq):
# TODO: Get name and version from ireq, if possible?
# Specifically, this might be needed in "name @ URL"
# syntax - need to check where that syntax is handled.
cand = self._make_candidate_from_link(
candidate = self._make_candidate_from_link(
ireq.link, extras=set(ireq.extras), parent=ireq,
)
return ExplicitRequirement(cand)
return self.make_requirement_from_candidate(candidate)
Copy link
Member

Choose a reason for hiding this comment

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

Not really related, but this might read better if we flip the conditionals (if not ireq.link:)

Copy link
Member

Choose a reason for hiding this comment

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

Didn't I see exactly that change in a different PR I was reviewing a few minutes ago? There may be merge conflicts in our future... But yes, I agree switching the order reads better.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, #8066

Copy link
Member

Choose a reason for hiding this comment

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

Seems like my coding style is fairly consistent 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to make the same change, but resisted the urge in favor of keeping the PR smaller. :)

Having this come in via #8066 works well for me.

return SpecifierRequirement(ireq, factory=self)

def make_requirement_from_candidate(self, candidate):
# type: (Candidate) -> ExplicitRequirement
return ExplicitRequirement(candidate)

def make_requirement_from_spec(self, specifier, comes_from):
# type: (str, InstallRequirement) -> Requirement
ireq = self._make_install_req_from_spec(specifier, comes_from)
Expand Down
4 changes: 1 addition & 3 deletions tests/functional/test_new_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,9 +825,7 @@ def _wheel_from_index(script, name, version, requires, extras):
@pytest.mark.parametrize(
"pkg_builder",
[
pytest.param(
_local_with_setup, marks=pytest.mark.xfail(strict=True),
),
_local_with_setup,
_direct_wheel,
_wheel_from_index,
],
Expand Down