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.

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

Ensure pip-tools unions dependencies of multiple declarations of a package with different extras #1486

Merged
merged 5 commits into from
Oct 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 0 additions & 11 deletions piptools/repositories/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,6 @@ def allow_all_wheels(self) -> Iterator[None]:
Monkey patches pip.Wheel to allow wheels from all platforms and Python versions.
"""

@abstractmethod
def copy_ireq_dependencies(
self, source: InstallRequirement, dest: InstallRequirement
) -> None:
"""
Notifies the repository that `dest` is a copy of `source`, and so it
has the same dependencies. Otherwise, once we prepare an ireq to assign
it its name, we would lose track of those dependencies on combining
that ireq with others.
"""

@property
@abstractmethod
def options(self) -> optparse.Values:
Expand Down
5 changes: 0 additions & 5 deletions piptools/repositories/local.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,3 @@ def get_hashes(self, ireq: InstallRequirement) -> Set[str]:
def allow_all_wheels(self) -> Iterator[None]:
with self.repository.allow_all_wheels():
yield

def copy_ireq_dependencies(
self, source: InstallRequirement, dest: InstallRequirement
) -> None:
self.repository.copy_ireq_dependencies(source, dest)
9 changes: 0 additions & 9 deletions piptools/repositories/pypi.py
Original file line number Diff line number Diff line change
Expand Up @@ -239,15 +239,6 @@ def get_dependencies(self, ireq: InstallRequirement) -> Set[InstallRequirement]:

return self._dependencies_cache[ireq]

def copy_ireq_dependencies(
self, source: InstallRequirement, dest: InstallRequirement
) -> None:
try:
self._dependencies_cache[dest] = self._dependencies_cache[source]
except KeyError:
# `source` may not be in cache yet.
pass

def _get_project(self, ireq: InstallRequirement) -> Any:
"""
Return a dict of a project info from PyPI JSON API for a given
Expand Down
72 changes: 57 additions & 15 deletions piptools/resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,18 +72,15 @@ def combine_install_requirements(

# deepcopy the accumulator so as to not modify the inputs
combined_ireq = copy.deepcopy(source_ireqs[0])
repository.copy_ireq_dependencies(source_ireqs[0], combined_ireq)

for ireq in source_ireqs[1:]:
# NOTE we may be losing some info on dropped reqs here
combined_ireq.req.specifier &= ireq.req.specifier
if combined_ireq.constraint:
# We don't find dependencies for constraint ireqs, so copy them
# from non-constraints:
repository.copy_ireq_dependencies(ireq, combined_ireq)
if combined_ireq.req is not None and ireq.req is not None:
combined_ireq.req.specifier &= ireq.req.specifier
combined_ireq.constraint &= ireq.constraint
# Return a sorted, de-duped tuple of extras
combined_ireq.extras = tuple(sorted({*combined_ireq.extras, *ireq.extras}))
combined_ireq.extras = {*combined_ireq.extras, *ireq.extras}
if combined_ireq.req is not None:
combined_ireq.req.extras = set(combined_ireq.extras)

# InstallRequirements objects are assumed to come from only one source, and
# so they support only a single comes_from entry. This function breaks this
Expand Down Expand Up @@ -208,6 +205,39 @@ def resolve(self, max_rounds: int = 10) -> Set[InstallRequirement]:

return results

def _get_ireq_with_name(
self,
ireq: InstallRequirement,
proxy_cache: Dict[InstallRequirement, InstallRequirement],
) -> InstallRequirement:
"""
Return the given ireq, if it has a name, or a proxy for the given ireq
which has been prepared and therefore has a name.

Preparing the ireq is side-effect-ful and can only be done once for an
instance, so we use a proxy instead. combine_install_requirements may
use the given ireq as a template for its aggregate result, mutating it
further by combining extras, etc. In that situation, we don't want that
aggregate ireq to be prepared prior to mutation, since its dependencies
will be frozen with those of only a subset of extras.

i.e. We both want its name early (via preparation), but we also need to
prepare it after any mutation for combination purposes. So we use a
proxy here for the early preparation.
"""
if ireq.name is not None:
return ireq

if ireq in proxy_cache:
return proxy_cache[ireq]

# get_dependencies has the side-effect of assigning name to ireq
# (so we can group by the name in _group_constraints below).
name_proxy = copy.deepcopy(ireq)
self.repository.get_dependencies(name_proxy)
proxy_cache[ireq] = name_proxy
return name_proxy

def _group_constraints(
self, constraints: Iterable[InstallRequirement]
) -> Iterator[InstallRequirement]:
Expand All @@ -227,19 +257,31 @@ def _group_constraints(

"""
constraints = list(constraints)
for ireq in constraints:
if ireq.name is None:
# get_dependencies has side-effect of assigning name to ireq
# (so we can group by the name below).
self.repository.get_dependencies(ireq)

cache: Dict[InstallRequirement, InstallRequirement] = {}

def key_from_ireq_with_name(ireq: InstallRequirement) -> str:
"""
See _get_ireq_with_name for context.

We use a cache per call here because it should only be necessary
the first time an ireq is passed here (later on in the round, it
will be prepared and dependencies for it calculated), but we can
save time by reusing the proxy between the sort and groupby calls
below.
"""
return key_from_ireq(self._get_ireq_with_name(ireq, cache))

# Sort first by name, i.e. the groupby key. Then within each group,
# sort editables first.
# This way, we don't bother with combining editables, since the first
# ireq will be editable, if one exists.
for _, ireqs in groupby(
sorted(constraints, key=(lambda x: (key_from_ireq(x), not x.editable))),
key=key_from_ireq,
sorted(
constraints,
key=(lambda x: (key_from_ireq_with_name(x), not x.editable)),
),
key=key_from_ireq_with_name,
):
yield combine_install_requirements(self.repository, ireqs)

Expand Down
4 changes: 0 additions & 4 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,6 @@ def allow_all_wheels(self):
# No need to do an actual pip.Wheel mock here.
yield

def copy_ireq_dependencies(self, source, dest):
# No state to update.
pass

@property
def options(self) -> optparse.Values:
"""Not used"""
Expand Down
32 changes: 32 additions & 0 deletions tests/test_cli_compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -1727,6 +1727,38 @@ def test_duplicate_reqs_combined(
assert "test-package-1==0.1" in out.stderr


def test_combine_extras(pip_conf, runner, make_package):
"""
Ensure that multiple declarations of a dependency that specify different
extras produces a requirement for that package with the union of the extras
"""
package_with_extras = make_package(
"package_with_extras",
extras_require={
"extra1": ["small-fake-a==0.1"],
"extra2": ["small-fake-b==0.1"],
},
)

with open("requirements.in", "w") as req_in:
req_in.writelines(
[
"-r ./requirements-second.in\n",
f"{package_with_extras}[extra1]",
]
)

with open("requirements-second.in", "w") as req_sec_in:
req_sec_in.write(f"{package_with_extras}[extra2]")

out = runner.invoke(cli, ["-n"])

assert out.exit_code == 0
assert "package-with-extras" in out.stderr
assert "small-fake-a==" in out.stderr
assert "small-fake-b==" in out.stderr


@pytest.mark.parametrize(
("pkg2_install_requires", "req_in_content", "out_expected_content"),
(
Expand Down
25 changes: 0 additions & 25 deletions tests/test_repository_local.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
import copy

import pytest

from piptools.repositories.local import LocalRequirementsRepository
from piptools.utils import key_from_ireq
from tests.conftest import FakeRepository

EXPECTED = {"sha256:5e6071ee6e4c59e0d0408d366fe9b66781d2cf01be9a6e19a2433bb3c5336330"}

Expand Down Expand Up @@ -57,25 +54,3 @@ def test_toggle_reuse_hashes_local_repository(
captured = capsys.readouterr()
assert captured.out == ""
assert captured.err == ""


class FakeRepositoryChecksForCopy(FakeRepository):
def __init__(self):
super().__init__()
self.copied = []

def copy_ireq_dependencies(self, source, dest):
self.copied.append(source)


def test_local_repository_copy_ireq_dependencies(from_line):
# Ensure that local repository forwards any messages to update its state
# of ireq dependencies.
checker = FakeRepositoryChecksForCopy()
local_repository = LocalRequirementsRepository({}, checker)

src = from_line("small-fake-a==0.1")
dest = copy.deepcopy(src)
local_repository.copy_ireq_dependencies(src, dest)

assert src in checker.copied
42 changes: 42 additions & 0 deletions tests/test_resolver.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import pytest
from pip._internal.utils.urls import path_to_url

from piptools.exceptions import NoCandidateFound
from piptools.resolver import RequirementSummary, combine_install_requirements
Expand Down Expand Up @@ -299,6 +300,47 @@ def test_combine_install_requirements(repository, from_line):
assert str(combined_all.req.specifier) == "<3.2,==3.1.1,>3.0"


def _test_combine_install_requirements_extras(repository, with_extra, without_extra):
combined = combine_install_requirements(repository, [without_extra, with_extra])
assert str(combined) == str(with_extra)
assert combined.extras == with_extra.extras

combined = combine_install_requirements(repository, [with_extra, without_extra])
assert str(combined) == str(with_extra)
assert combined.extras == with_extra.extras


def test_combine_install_requirements_extras_req(repository, from_line, make_package):
"""
Extras should be unioned in combined install requirements
(whether or not InstallRequirement.req is None, and testing either order of the inputs)
"""
with_extra = from_line("edx-opaque-keys[django]==1.0.1")
assert with_extra.req is not None
without_extra = from_line("edx-opaque-keys")
assert without_extra.req is not None

_test_combine_install_requirements_extras(repository, with_extra, without_extra)


def test_combine_install_requirements_extras_no_req(
repository, from_line, make_package
):
"""
Extras should be unioned in combined install requirements
(whether or not InstallRequirement.req is None, and testing either order of the inputs)
"""
test_package = make_package("test-package", extras_require={"extra": []})
local_package_with_extra = from_line(f"{test_package}[extra]")
assert local_package_with_extra.req is None
local_package_without_extra = from_line(path_to_url(test_package))
assert local_package_without_extra.req is None

_test_combine_install_requirements_extras(
repository, local_package_with_extra, local_package_without_extra
)


def test_compile_failure_shows_provenance(resolver, from_line):
"""
Provenance of conflicting dependencies should be printed on failure.
Expand Down