From ffb1909cce7e52379656bc0b54df247e883ffde4 Mon Sep 17 00:00:00 2001 From: Richard Frank Date: Tue, 12 Oct 2021 19:50:52 -0400 Subject: [PATCH 1/5] Partially revert "Fix a regression where multiple declarations of a dependency" (commit d720fe605b7a0c88be8a9afba5029b1408c8295b) This brings back copy_ireq_dependencies and combines its use with that of the proxies-for-naming. Editable and url_requirement ireqs are not cached by the Repository, so when dependencies are queried for them once, no dependencies will be found later for the copy made by combine_install_requirements. copy_ireq_dependencies links those dependencies at the time of copy. --- piptools/repositories/base.py | 11 +++++++++++ piptools/repositories/local.py | 5 +++++ piptools/repositories/pypi.py | 9 +++++++++ piptools/resolver.py | 5 +++++ tests/conftest.py | 4 ++++ tests/test_repository_local.py | 25 +++++++++++++++++++++++++ tests/test_resolver.py | 19 +++++++++++++++++++ 7 files changed, 78 insertions(+) diff --git a/piptools/repositories/base.py b/piptools/repositories/base.py index 3ffec38d6..734ac6f57 100644 --- a/piptools/repositories/base.py +++ b/piptools/repositories/base.py @@ -47,6 +47,17 @@ 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: diff --git a/piptools/repositories/local.py b/piptools/repositories/local.py index 754b6076a..40edca7ef 100644 --- a/piptools/repositories/local.py +++ b/piptools/repositories/local.py @@ -95,3 +95,8 @@ 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) diff --git a/piptools/repositories/pypi.py b/piptools/repositories/pypi.py index ffaad4187..e198c743f 100644 --- a/piptools/repositories/pypi.py +++ b/piptools/repositories/pypi.py @@ -241,6 +241,15 @@ 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 diff --git a/piptools/resolver.py b/piptools/resolver.py index 7bd3a5478..32e3e9466 100644 --- a/piptools/resolver.py +++ b/piptools/resolver.py @@ -72,11 +72,16 @@ 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 if combined_ireq.req is not None and ireq.req is not None: 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) combined_ireq.constraint &= ireq.constraint combined_ireq.extras = {*combined_ireq.extras, *ireq.extras} if combined_ireq.req is not None: diff --git a/tests/conftest.py b/tests/conftest.py index 09750512e..321210930 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -89,6 +89,10 @@ 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""" diff --git a/tests/test_repository_local.py b/tests/test_repository_local.py index 9d18c4e33..9381770a2 100644 --- a/tests/test_repository_local.py +++ b/tests/test_repository_local.py @@ -1,7 +1,10 @@ +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"} @@ -54,3 +57,25 @@ 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 diff --git a/tests/test_resolver.py b/tests/test_resolver.py index bf28ed6e4..438293339 100644 --- a/tests/test_resolver.py +++ b/tests/test_resolver.py @@ -284,6 +284,25 @@ def test_iter_dependencies_ignores_constraints(resolver, from_line): next(res._iter_dependencies(ireq)) +def test_iter_dependencies_after_combine_install_requirements( + pypi_repository, base_resolver, make_package, from_line +): + res = base_resolver([], repository=pypi_repository) + + sub_deps = ["click"] + package_a = make_package("package-a", install_requires=sub_deps) + package_b = make_package("package-b", install_requires=["package-a"]) + + local_package_a = from_line(path_to_url(package_a)) + assert [dep.name for dep in res._iter_dependencies(local_package_a)] == sub_deps + + package_a_from_b = from_line("package-a", comes_from=path_to_url(package_b)) + combined = combine_install_requirements( + pypi_repository, [local_package_a, package_a_from_b] + ) + assert [dep.name for dep in res._iter_dependencies(combined)] == sub_deps + + def test_combine_install_requirements(repository, from_line): celery30 = from_line("celery>3.0", comes_from="-r requirements.in") celery31 = from_line("celery==3.1.1", comes_from=from_line("fake-package")) From 806918b8b63458688a3823729b5a9b980e4d3840 Mon Sep 17 00:00:00 2001 From: Richard Frank Date: Sat, 16 Oct 2021 14:44:52 -0400 Subject: [PATCH 2/5] Failing test for case of resolver combining extras after the first round of resolution. By this point, we've frozen the dependencies. --- tests/test_resolver.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/test_resolver.py b/tests/test_resolver.py index 438293339..19c63d12b 100644 --- a/tests/test_resolver.py +++ b/tests/test_resolver.py @@ -303,6 +303,32 @@ def test_iter_dependencies_after_combine_install_requirements( assert [dep.name for dep in res._iter_dependencies(combined)] == sub_deps +@pytest.mark.xfail( + reason="resolver does not yet support combining extras after the first round" +) +def test_iter_dependencies_after_combine_install_requirements_extras( + pypi_repository, base_resolver, make_package, from_line +): + res = base_resolver([], repository=pypi_repository) + + package_a = make_package( + "package-a", extras_require={"click": ["click"], "celery": ["celery"]} + ) + package_b = make_package("package-b", install_requires=["package-a"]) + + local_package_a = from_line(path_to_url(package_a)) + assert [dep.name for dep in res._iter_dependencies(local_package_a)] == [] + + package_a_from_b = from_line("package-a[click]", comes_from=path_to_url(package_b)) + package_a_with_other_extra = from_line("package-a[celery]") + combined = combine_install_requirements( + pypi_repository, [local_package_a, package_a_from_b, package_a_with_other_extra] + ) + + dependency_names = {dep.name for dep in res._iter_dependencies(combined)} + assert {"celery", "click"}.issubset(dependency_names) + + def test_combine_install_requirements(repository, from_line): celery30 = from_line("celery>3.0", comes_from="-r requirements.in") celery31 = from_line("celery==3.1.1", comes_from=from_line("fake-package")) From 9eb0dfdbe0e797fb469d057a1959e6fc3b5d5d4d Mon Sep 17 00:00:00 2001 From: Richard Frank Date: Sun, 17 Oct 2021 18:42:46 -0400 Subject: [PATCH 3/5] Integration test for #1505 --- tests/test_cli_compile.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/test_cli_compile.py b/tests/test_cli_compile.py index 63544bb13..15fdcf7d5 100644 --- a/tests/test_cli_compile.py +++ b/tests/test_cli_compile.py @@ -1727,6 +1727,34 @@ def test_duplicate_reqs_combined( assert "test-package-1==0.1" in out.stderr +def test_local_duplicate_subdependency_combined(runner, make_package): + """ + Test pip-compile tracks subdependencies properly when install requirements + are combined, especially when local paths are passed as urls, and those reqs + are combined after getting dependencies. + + Regression test for issue GH-1505. + """ + package_a = make_package("project-a", install_requires=["pip-tools==6.3.0"]) + package_b = make_package("project-b", install_requires=["project-a"]) + + with open("requirements.in", "w") as req_in: + req_in.writelines( + [ + f"file://{package_a}#egg=project-a\n", + f"file://{package_b}#egg=project-b", + ] + ) + + out = runner.invoke(cli, ["-n"]) + + assert out.exit_code == 0 + assert "project-b" in out.stderr + assert "project-a" in out.stderr + assert "pip-tools==6.3.0" in out.stderr + assert "click" in out.stderr # dependency of pip-tools + + def test_combine_extras(pip_conf, runner, make_package): """ Ensure that multiple declarations of a dependency that specify different From c9b9db4ee3f46250c36612c4350ff1e5c60b519a Mon Sep 17 00:00:00 2001 From: Andy Kluger Date: Tue, 12 Oct 2021 20:53:06 -0400 Subject: [PATCH 4/5] Integrate microcat49's attribute copying fix for lost local req info Fixes #1054 Co-authored-by: Andrew --- piptools/resolver.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/piptools/resolver.py b/piptools/resolver.py index 32e3e9466..ac95e63e9 100644 --- a/piptools/resolver.py +++ b/piptools/resolver.py @@ -87,6 +87,11 @@ def combine_install_requirements( if combined_ireq.req is not None: combined_ireq.req.extras = set(combined_ireq.extras) + for attr in ("link", "local_file_path", "original_link"): + setattr( + combined_ireq, attr, getattr(combined_ireq, attr) or getattr(ireq, attr) + ) + # InstallRequirements objects are assumed to come from only one source, and # so they support only a single comes_from entry. This function breaks this # model. As a workaround, we deterministically choose a single source for From 036e7c36283391f2ca27188e590b5b7b5b2e36b7 Mon Sep 17 00:00:00 2001 From: Richard Frank Date: Thu, 21 Oct 2021 20:04:21 -0400 Subject: [PATCH 5/5] Added test for lost local req info --- tests/test_resolver.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/test_resolver.py b/tests/test_resolver.py index 19c63d12b..73ce3a02a 100644 --- a/tests/test_resolver.py +++ b/tests/test_resolver.py @@ -386,6 +386,22 @@ def test_combine_install_requirements_extras_no_req( ) +def test_combine_install_requirements_with_paths(repository, from_line, make_package): + name = "fake_package_b" + version = "1.0.0" + + test_package = make_package(name, version=version) + fake_package = from_line(f"{name} @ {path_to_url(test_package)}") + fake_package_name = from_line(f"{name}=={version}", comes_from=from_line(name)) + + for pair in [(fake_package, fake_package_name), (fake_package_name, fake_package)]: + combined = combine_install_requirements(repository, pair) + assert str(combined.specifier) == str(fake_package_name.specifier) + assert str(combined.link) == str(fake_package.link) + assert str(combined.local_file_path) == str(fake_package.local_file_path) + assert str(combined.original_link) == str(fake_package.original_link) + + def test_compile_failure_shows_provenance(resolver, from_line): """ Provenance of conflicting dependencies should be printed on failure.