From 4099a95223920b29cd227994d286930aa87e1087 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Randy=20D=C3=B6ring?= <30527984+radoering@users.noreply.github.com> Date: Fri, 25 Nov 2022 17:13:21 +0100 Subject: [PATCH 1/3] refactor(provider): extract handling of any marker dependencies into separate method --- src/poetry/puzzle/provider.py | 103 ++++++++++++++++++---------------- 1 file changed, 54 insertions(+), 49 deletions(-) diff --git a/src/poetry/puzzle/provider.py b/src/poetry/puzzle/provider.py index 0effa9598a8..ccc2394f754 100644 --- a/src/poetry/puzzle/provider.py +++ b/src/poetry/puzzle/provider.py @@ -740,55 +740,7 @@ def fmt_warning(d: Dependency) -> str: f"Different requirements found for {warnings}." ) - # We need to check if one of the duplicate dependencies - # has no markers. If there is one, we need to change its - # environment markers to the inverse of the union of the - # other dependencies markers. - # For instance, if we have the following dependencies: - # - ipython - # - ipython (1.2.4) ; implementation_name == "pypy" - # - # the marker for `ipython` will become `implementation_name != "pypy"`. - # - # Further, we have to merge the constraints of the requirements - # without markers into the constraints of the requirements with markers. - # for instance, if we have the following dependencies: - # - foo (>= 1.2) - # - foo (!= 1.2.1) ; python == 3.10 - # - # the constraint for the second entry will become (!= 1.2.1, >= 1.2) - any_markers_dependencies = [d for d in deps if d.marker.is_any()] - other_markers_dependencies = [d for d in deps if not d.marker.is_any()] - - marker = other_markers_dependencies[0].marker - for other_dep in other_markers_dependencies[1:]: - marker = marker.union(other_dep.marker) - inverted_marker = marker.invert() - - if any_markers_dependencies: - for dep_any in any_markers_dependencies: - dep_any.marker = inverted_marker - for dep_other in other_markers_dependencies: - dep_other.constraint = dep_other.constraint.intersect( - dep_any.constraint - ) - elif not inverted_marker.is_empty() and self._python_constraint.allows_any( - get_python_constraint_from_marker(inverted_marker) - ): - # if there is no any marker dependency - # and the inverted marker is not empty, - # a dependency with the inverted union of all markers is required - # in order to not miss other dependencies later, for instance: - # - foo (1.0) ; python == 3.7 - # - foo (2.0) ; python == 3.8 - # - bar (2.0) ; python == 3.8 - # - bar (3.0) ; python == 3.9 - # - # the last dependency would be missed without this, - # because the intersection with both foo dependencies is empty - inverted_marker_dep = deps[0].with_constraint(EmptyConstraint()) - inverted_marker_dep.marker = inverted_marker - deps.append(inverted_marker_dep) + self._handle_any_marker_dependencies(deps) overrides = [] overrides_marker_intersection: BaseMarker = AnyMarker() @@ -1021,3 +973,56 @@ def _merge_dependencies_by_marker( ) deps.append(_deps[0].with_constraint(new_constraint)) return deps + + def _handle_any_marker_dependencies(self, dependencies: list[Dependency]) -> None: + """ + We need to check if one of the duplicate dependencies + has no markers. If there is one, we need to change its + environment markers to the inverse of the union of the + other dependencies markers. + For instance, if we have the following dependencies: + - ipython + - ipython (1.2.4) ; implementation_name == "pypy" + + the marker for `ipython` will become `implementation_name != "pypy"`. + + Further, we have to merge the constraints of the requirements + without markers into the constraints of the requirements with markers. + for instance, if we have the following dependencies: + - foo (>= 1.2) + - foo (!= 1.2.1) ; python == 3.10 + + the constraint for the second entry will become (!= 1.2.1, >= 1.2). + """ + any_markers_dependencies = [d for d in dependencies if d.marker.is_any()] + other_markers_dependencies = [d for d in dependencies if not d.marker.is_any()] + + marker = other_markers_dependencies[0].marker + for other_dep in other_markers_dependencies[1:]: + marker = marker.union(other_dep.marker) + inverted_marker = marker.invert() + + if any_markers_dependencies: + for dep_any in any_markers_dependencies: + dep_any.marker = inverted_marker + for dep_other in other_markers_dependencies: + dep_other.constraint = dep_other.constraint.intersect( + dep_any.constraint + ) + elif not inverted_marker.is_empty() and self._python_constraint.allows_any( + get_python_constraint_from_marker(inverted_marker) + ): + # if there is no any marker dependency + # and the inverted marker is not empty, + # a dependency with the inverted union of all markers is required + # in order to not miss other dependencies later, for instance: + # - foo (1.0) ; python == 3.7 + # - foo (2.0) ; python == 3.8 + # - bar (2.0) ; python == 3.8 + # - bar (3.0) ; python == 3.9 + # + # the last dependency would be missed without this, + # because the intersection with both foo dependencies is empty + inverted_marker_dep = dependencies[0].with_constraint(EmptyConstraint()) + inverted_marker_dep.marker = inverted_marker + dependencies.append(inverted_marker_dep) From c6a5f3f293ef48c272ae095b37539b452c2ea39e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Randy=20D=C3=B6ring?= <30527984+radoering@users.noreply.github.com> Date: Fri, 25 Nov 2022 17:02:45 +0100 Subject: [PATCH 2/3] provider: discard any marker dependencies if the resulting marker after merging is empty, not compatible with the project's python constraint or not compatible with the set environment --- src/poetry/puzzle/provider.py | 60 +++++++++------ tests/puzzle/test_solver.py | 137 ++++++++++++++++++++++++++++++++++ 2 files changed, 173 insertions(+), 24 deletions(-) diff --git a/src/poetry/puzzle/provider.py b/src/poetry/puzzle/provider.py index ccc2394f754..7c87f7401a1 100644 --- a/src/poetry/puzzle/provider.py +++ b/src/poetry/puzzle/provider.py @@ -740,7 +740,7 @@ def fmt_warning(d: Dependency) -> str: f"Different requirements found for {warnings}." ) - self._handle_any_marker_dependencies(deps) + deps = self._handle_any_marker_dependencies(deps) overrides = [] overrides_marker_intersection: BaseMarker = AnyMarker() @@ -974,7 +974,9 @@ def _merge_dependencies_by_marker( deps.append(_deps[0].with_constraint(new_constraint)) return deps - def _handle_any_marker_dependencies(self, dependencies: list[Dependency]) -> None: + def _handle_any_marker_dependencies( + self, dependencies: list[Dependency] + ) -> list[Dependency]: """ We need to check if one of the duplicate dependencies has no markers. If there is one, we need to change its @@ -997,32 +999,42 @@ def _handle_any_marker_dependencies(self, dependencies: list[Dependency]) -> Non any_markers_dependencies = [d for d in dependencies if d.marker.is_any()] other_markers_dependencies = [d for d in dependencies if not d.marker.is_any()] + for dep_any in any_markers_dependencies: + for dep_other in other_markers_dependencies: + dep_other.constraint = dep_other.constraint.intersect( + dep_any.constraint + ) + marker = other_markers_dependencies[0].marker for other_dep in other_markers_dependencies[1:]: marker = marker.union(other_dep.marker) inverted_marker = marker.invert() - if any_markers_dependencies: - for dep_any in any_markers_dependencies: - dep_any.marker = inverted_marker - for dep_other in other_markers_dependencies: - dep_other.constraint = dep_other.constraint.intersect( - dep_any.constraint - ) - elif not inverted_marker.is_empty() and self._python_constraint.allows_any( + if ( + not inverted_marker.is_empty() + and self._python_constraint.allows_any( get_python_constraint_from_marker(inverted_marker) + ) + and (not self._env or inverted_marker.validate(self._env.marker_env)) ): - # if there is no any marker dependency - # and the inverted marker is not empty, - # a dependency with the inverted union of all markers is required - # in order to not miss other dependencies later, for instance: - # - foo (1.0) ; python == 3.7 - # - foo (2.0) ; python == 3.8 - # - bar (2.0) ; python == 3.8 - # - bar (3.0) ; python == 3.9 - # - # the last dependency would be missed without this, - # because the intersection with both foo dependencies is empty - inverted_marker_dep = dependencies[0].with_constraint(EmptyConstraint()) - inverted_marker_dep.marker = inverted_marker - dependencies.append(inverted_marker_dep) + if any_markers_dependencies: + for dep_any in any_markers_dependencies: + dep_any.marker = inverted_marker + else: + # If there is no any marker dependency + # and the inverted marker is not empty, + # a dependency with the inverted union of all markers is required + # in order to not miss other dependencies later, for instance: + # - foo (1.0) ; python == 3.7 + # - foo (2.0) ; python == 3.8 + # - bar (2.0) ; python == 3.8 + # - bar (3.0) ; python == 3.9 + # + # the last dependency would be missed without this, + # because the intersection with both foo dependencies is empty. + inverted_marker_dep = dependencies[0].with_constraint(EmptyConstraint()) + inverted_marker_dep.marker = inverted_marker + dependencies.append(inverted_marker_dep) + else: + dependencies = other_markers_dependencies + return dependencies diff --git a/tests/puzzle/test_solver.py b/tests/puzzle/test_solver.py index d55ea5b2915..7d05ffae884 100644 --- a/tests/puzzle/test_solver.py +++ b/tests/puzzle/test_solver.py @@ -1480,6 +1480,143 @@ def test_solver_duplicate_dependencies_different_constraints_merge_no_markers( ) +def test_solver_duplicate_dependencies_different_constraints_discard_no_markers1( + solver: Solver, repo: Repository, package: ProjectPackage +) -> None: + """ + Initial dependencies: + A (>=1.0) + A (<1.2) ; python >= 3.10 + A (<1.1) ; python < 3.10 + + Merged dependencies: + A (>=1.0) ; + A (>=1.0,<1.2) ; python >= 3.10 + A (>=1.0,<1.1) ; python < 3.10 + + The dependency with an empty marker has to be ignored. + """ + package.add_dependency(Factory.create_dependency("A", ">=1.0")) + package.add_dependency( + Factory.create_dependency("A", {"version": "<1.2", "python": ">=3.10"}) + ) + package.add_dependency( + Factory.create_dependency("A", {"version": "<1.1", "python": "<3.10"}) + ) + package.add_dependency(Factory.create_dependency("B", "*")) + + package_a10 = get_package("A", "1.0") + package_a11 = get_package("A", "1.1") + package_a12 = get_package("A", "1.2") + package_b = get_package("B", "1.0") + package_b.add_dependency(Factory.create_dependency("A", "*")) + + repo.add_package(package_a10) + repo.add_package(package_a11) + repo.add_package(package_a12) + repo.add_package(package_b) + + transaction = solver.solve() + + check_solver_result( + transaction, + [ + # only a10 and a11, not a12 + {"job": "install", "package": package_a10}, + {"job": "install", "package": package_a11}, + {"job": "install", "package": package_b}, + ], + ) + + +def test_solver_duplicate_dependencies_different_constraints_discard_no_markers2( + solver: Solver, repo: Repository, package: ProjectPackage +) -> None: + """ + Initial dependencies: + A (>=1.0) + A (<1.2) ; python == 3.10 + + Merged dependencies: + A (>=1.0) ; python != 3.10 + A (>=1.0,<1.2) ; python == 3.10 + + The first dependency has to be ignored + because it is not compatible with the project's python constraint. + """ + set_package_python_versions(solver.provider, "~3.10") + package.add_dependency(Factory.create_dependency("A", ">=1.0")) + package.add_dependency( + Factory.create_dependency("A", {"version": "<1.2", "python": "3.10"}) + ) + package.add_dependency(Factory.create_dependency("B", "*")) + + package_a10 = get_package("A", "1.0") + package_a11 = get_package("A", "1.1") + package_a12 = get_package("A", "1.2") + package_b = get_package("B", "1.0") + package_b.add_dependency(Factory.create_dependency("A", "*")) + + repo.add_package(package_a10) + repo.add_package(package_a11) + repo.add_package(package_a12) + repo.add_package(package_b) + + transaction = solver.solve() + + check_solver_result( + transaction, + [ + {"job": "install", "package": package_a11}, # only a11, not a12 + {"job": "install", "package": package_b}, + ], + ) + + +def test_solver_duplicate_dependencies_different_constraints_discard_no_markers3( + solver: Solver, repo: Repository, package: ProjectPackage +) -> None: + """ + Initial dependencies: + A (>=1.0) + A (<1.2) ; python == 3.10 + + Merged dependencies: + A (>=1.0) ; python != 3.10 + A (>=1.0,<1.2) ; python == 3.10 + + The first dependency has to be ignored + because it is not compatible with the current environment. + """ + package.add_dependency(Factory.create_dependency("A", ">=1.0")) + package.add_dependency( + Factory.create_dependency("A", {"version": "<1.2", "python": "3.10"}) + ) + package.add_dependency(Factory.create_dependency("B", "*")) + + package_a10 = get_package("A", "1.0") + package_a11 = get_package("A", "1.1") + package_a12 = get_package("A", "1.2") + package_b = get_package("B", "1.0") + package_b.add_dependency(Factory.create_dependency("A", "*")) + + repo.add_package(package_a10) + repo.add_package(package_a11) + repo.add_package(package_a12) + repo.add_package(package_b) + + with solver.use_environment(MockEnv((3, 10, 0))): + transaction = solver.solve() + + check_solver_result( + transaction, + [ + {"job": "install", "package": package_a11}, # only a11, not a12 + {"job": "install", "package": package_b}, + ], + ) + + def test_solver_duplicate_dependencies_ignore_overrides_with_empty_marker_intersection( solver: Solver, repo: Repository, package: ProjectPackage ): From ee80212456b64bdba673dc495d0f9240959282d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Randy=20D=C3=B6ring?= <30527984+radoering@users.noreply.github.com> Date: Sat, 26 Nov 2022 16:57:01 +0100 Subject: [PATCH 3/3] provider: raise error if there are incompatible constraints in the requirements of a package --- src/poetry/puzzle/provider.py | 29 +++++++++++++++++++++++------ tests/puzzle/test_solver.py | 24 ++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/src/poetry/puzzle/provider.py b/src/poetry/puzzle/provider.py index 7c87f7401a1..9c7457f7e7c 100644 --- a/src/poetry/puzzle/provider.py +++ b/src/poetry/puzzle/provider.py @@ -59,6 +59,18 @@ logger = logging.getLogger(__name__) +class IncompatibleConstraintsError(Exception): + """ + Exception when there are duplicate dependencies with incompatible constraints. + """ + + def __init__(self, package: Package, *dependencies: Dependency) -> None: + constraints = "\n".join(dep.to_pep_508() for dep in dependencies) + super().__init__( + f"Incompatible constraints in requirements of {package}:\n{constraints}" + ) + + class Indicator(ProgressIndicator): CONTEXT: str | None = None @@ -740,7 +752,7 @@ def fmt_warning(d: Dependency) -> str: f"Different requirements found for {warnings}." ) - deps = self._handle_any_marker_dependencies(deps) + deps = self._handle_any_marker_dependencies(package, deps) overrides = [] overrides_marker_intersection: BaseMarker = AnyMarker() @@ -975,7 +987,7 @@ def _merge_dependencies_by_marker( return deps def _handle_any_marker_dependencies( - self, dependencies: list[Dependency] + self, package: Package, dependencies: list[Dependency] ) -> list[Dependency]: """ We need to check if one of the duplicate dependencies @@ -999,11 +1011,16 @@ def _handle_any_marker_dependencies( any_markers_dependencies = [d for d in dependencies if d.marker.is_any()] other_markers_dependencies = [d for d in dependencies if not d.marker.is_any()] - for dep_any in any_markers_dependencies: + if any_markers_dependencies: for dep_other in other_markers_dependencies: - dep_other.constraint = dep_other.constraint.intersect( - dep_any.constraint - ) + new_constraint = dep_other.constraint + for dep_any in any_markers_dependencies: + new_constraint = new_constraint.intersect(dep_any.constraint) + if new_constraint.is_empty(): + raise IncompatibleConstraintsError( + package, dep_other, *any_markers_dependencies + ) + dep_other.constraint = new_constraint marker = other_markers_dependencies[0].marker for other_dep in other_markers_dependencies[1:]: diff --git a/tests/puzzle/test_solver.py b/tests/puzzle/test_solver.py index 7d05ffae884..04f2dd70b35 100644 --- a/tests/puzzle/test_solver.py +++ b/tests/puzzle/test_solver.py @@ -1,5 +1,7 @@ from __future__ import annotations +import re + from pathlib import Path from typing import TYPE_CHECKING from typing import Any @@ -19,6 +21,7 @@ from poetry.packages import DependencyPackage from poetry.puzzle import Solver from poetry.puzzle.exceptions import SolverProblemError +from poetry.puzzle.provider import IncompatibleConstraintsError from poetry.repositories.repository import Repository from poetry.repositories.repository_pool import RepositoryPool from poetry.utils.env import MockEnv @@ -1480,6 +1483,27 @@ def test_solver_duplicate_dependencies_different_constraints_merge_no_markers( ) +def test_solver_duplicate_dependencies_different_constraints_conflict( + solver: Solver, repo: Repository, package: ProjectPackage +) -> None: + package.add_dependency(Factory.create_dependency("A", ">=1.1")) + package.add_dependency( + Factory.create_dependency("A", {"version": "<1.1", "python": "3.10"}) + ) + + repo.add_package(get_package("A", "1.0")) + repo.add_package(get_package("A", "1.1")) + repo.add_package(get_package("A", "1.2")) + + expectation = ( + "Incompatible constraints in requirements of root (1.0):\n" + 'A (<1.1) ; python_version == "3.10"\n' + "A (>=1.1)" + ) + with pytest.raises(IncompatibleConstraintsError, match=re.escape(expectation)): + solver.solve() + + def test_solver_duplicate_dependencies_different_constraints_discard_no_markers1( solver: Solver, repo: Repository, package: ProjectPackage ) -> None: