Fix handling of duplicate dependencies with different constraints

poetry/console/commands/debug/resolve.py

@@ -78,6 +78,9 @@ class DebugResolveCommand(Command):
                     package.name, package.version
                 )
             )
+            if package.requirements:
+                for req_name, req_value in package.requirements.items():
+                    self.line("    - {}: {}".format(req_name, req_value))
 
     def _determine_requirements(self, requires):  # type: (List[str]) -> List[str]
         if not requires:

poetry/installation/installer.py

@@ -3,6 +3,7 @@ import sys
 from typing import List
 from typing import Union
 
+from poetry.io import NullIO
 from poetry.packages import Dependency
 from poetry.packages import Locker
 from poetry.packages import Package
@@ -182,6 +183,23 @@ class Installer:
 
         self._populate_local_repo(local_repo, ops, locked_repository)
 
+        with self._package.with_python_versions(
+            ".".join([str(i) for i in self._venv.version_info[:3]])
+        ):
+            # We resolve again by only using the lock file
+            pool = Pool()
+            pool.add_repository(local_repo)
+
+            solver = Solver(
+                self._package,
+                pool,
+                self._installed_repository,
+                locked_repository,
+                NullIO(),
+            )
+
+            ops = solver.solve()
+
         # We need to filter operations so that packages
         # not compatible with the current system,
         # or optional and not requested, are dropped
@@ -365,13 +383,17 @@ class Installer:
                         local_repo.remove_package(pkg)
                         local_repo.add_package(op.target_package)
                     elif op.job_type == "uninstall":
-                        local_repo.remove_package(op.package)
+                        if pkg.version == package.version:
+                            local_repo.remove_package(op.package)
                     else:
                         # Even though the package already exists
                         # in the lock file we will prefer the new one
                         # to force updates
-                        local_repo.remove_package(pkg)
-                        local_repo.add_package(package)
+                        if pkg.version == package.version:
+                            local_repo.remove_package(pkg)
+                            local_repo.add_package(package)
+                        else:
+                            local_repo.add_package(package)
 
                     acted_on = True
 

poetry/mixology/__init__.py

@@ -4,5 +4,4 @@ from .version_solver import VersionSolver
 def resolve_version(root, provider, locked=None, use_latest=None):
     solver = VersionSolver(root, provider, locked=locked, use_latest=use_latest)
 
-    with provider.progress():
-        return solver.solve()
+    return solver.solve()

poetry/packages/locker.py

@@ -174,7 +174,25 @@ class Locker:
             if dependency.is_optional() and not dependency.is_activated():
                 continue
 
-            dependencies[dependency.pretty_name] = str(dependency.pretty_constraint)
+            if dependency.pretty_name not in dependencies:
+                dependencies[dependency.pretty_name] = []
+
+            constraint = {"version": str(dependency.pretty_constraint)}
+
+            if not dependency.python_constraint.is_any():
+                constraint["python"] = str(dependency.python_constraint)
+
+            if dependency.platform != "*":
+                constraint["platform"] = dependency.platform
+
+            if len(constraint) == 1:
+                dependencies[dependency.pretty_name].append(constraint["version"])
+            else:
+                dependencies[dependency.pretty_name].append(constraint)
+
+        for name, constraints in dependencies.items():
+            if len(constraints) == 1:
+                dependencies[name] = constraints[0]
 
         data = {
             "name": package.pretty_name,

poetry/packages/package.py

@@ -2,6 +2,7 @@
 import copy
 import re
 
+from contextlib import contextmanager
 from typing import Union
 
 from poetry.semver import Version
@@ -309,6 +310,29 @@ class Package(object):
     def to_dependency(self):
         return Dependency(self.name, self._version)
 
+    @contextmanager
+    def with_python_versions(self, python_versions):
+        original_python_versions = self.python_versions
+
+        self.python_versions = python_versions
+
+        yield
+
+        self.python_versions = original_python_versions
+
+    def clone(self):  # type: () -> Package
+        clone = Package(self.pretty_name, self.version)
+        clone.category = self.category
+        clone.optional = self.optional
+        clone.python_versions = self.python_versions
+        clone.platform = self.platform
+        clone.extras = self.extras
+
+        for dep in self.requires:
+            clone.requires.append(dep)
+
+        return clone
+
     def __hash__(self):
         return hash((self._name, self._version))
 

poetry/puzzle/exceptions.py

+class CompatibilityError(Exception):
+    def __init__(self, *constraints):
+        self._constraints = list(constraints)
+
+    @property
+    def constraints(self):
+        return self._constraints
+
+
 class SolverProblemError(Exception):
     def __init__(self, error):
         self._error = error

poetry/puzzle/provider.py

+import logging
 import os
 import pkginfo
 import shutil
@@ -33,6 +34,10 @@ from poetry.utils.venv import Venv
 from poetry.vcs.git import Git
 
 from .dependencies import Dependencies
+from .exceptions import CompatibilityError
+
+
+logger = logging.getLogger(__name__)
 
 
 class Indicator(ProgressIndicator):
@@ -65,6 +70,7 @@ class Provider:
         self._python_constraint = package.python_constraint
         self._search_for = {}
         self._is_debugging = self._io.is_debug() or self._io.is_very_verbose()
+        self._in_progress = False
 
     @property
     def pool(self):  # type: () -> Pool
@@ -321,9 +327,8 @@ class Provider:
         # will become:
         #   - enum34; python_version=="2.7" or python_version=="3.3"
         #
-        # TODO: If the duplicate dependencies have different constraints
-        # we should notify the resolver in some way to make it split the
-        # current graph.
+        # If the duplicate dependencies have different constraints
+        # we have to split the dependency graph.
         #
         # An example of this is:
         #   - pypiwin32 (220); sys_platform == "win32" and python_version >= "3.6"
@@ -342,6 +347,8 @@ class Provider:
                     dependencies.append(deps[0])
                     continue
 
+                self.debug("Duplicate dependencies for {}".format(dep_name))
+
                 # Regrouping by constraint
                 by_constraint = {}
                 for dep in deps:
@@ -380,15 +387,24 @@ class Provider:
                     continue
 
                 if len(by_constraint) == 1:
+                    self.debug("Merging requirements for {}".format(str(deps[0])))
                     dependencies.append(list(by_constraint.values())[0][0])
                     continue
 
-                # At this point, we have one dependency by constraint
-                # So we add them to the dependency set
+                # At this point, we raise an exception that will
+                # tell the solver to enter compatibility mode
+                # which means it will resolve for each minor
+                # Python version supported
+                python_constraints = []
                 for constraint, _deps in by_constraint.items():
-                    _dep = _deps[0]
+                    python_constraints.append(_deps[0].python_versions)
 
-                    dependencies.append(_dep)
+                self.debug(
+                    "<warning>Uncompatible constraints for {}.</warning>".format(
+                        dep_name
+                    )
+                )
+                raise CompatibilityError(*python_constraints)
 
         package.requires = dependencies
 
@@ -400,19 +416,6 @@ class Provider:
     def output(self):
         return self._io
 
-    def before_resolution(self):
-        self._io.write("<info>Resolving dependencies</>")
-
-        if self.is_debugging():
-            self._io.new_line()
-
-    def indicate_progress(self):
-        if not self.is_debugging():
-            self._io.write(".")
-
-    def after_resolution(self):
-        self._io.new_line()
-
     def debug(self, message, depth=0):
         if self.is_debugging():
             debug_info = str(message)
@@ -438,3 +441,5 @@ class Provider:
 
             with indicator.auto():
                 yield
+
+        self._in_progress = False

poetry/puzzle/solver.py

@@ -6,6 +6,7 @@ from poetry.packages.constraints.generic_constraint import GenericConstraint
 
 from poetry.semver import parse_constraint
 
+from .exceptions import CompatibilityError
 from .exceptions import SolverProblemError
 
 from .operations import Install
@@ -23,41 +24,13 @@ class Solver:
         self._installed = installed
         self._locked = locked
         self._io = io
+        self._provider = Provider(self._package, self._pool, self._io)
 
     def solve(self, use_latest=None):  # type: (...) -> List[Operation]
-        provider = Provider(self._package, self._pool, self._io)
-        locked = {}
-        for package in self._locked.packages:
-            locked[package.name] = package
-
-        try:
-            result = resolve_version(
-                self._package, provider, locked=locked, use_latest=use_latest
-            )
-        except SolveFailure as e:
-            raise SolverProblemError(e)
+        with self._provider.progress():
+            packages = self._solve(use_latest=use_latest)
 
-        packages = result.packages
         requested = self._package.all_requires
-        graph = self._build_graph(self._package, packages)
-
-        for package in packages:
-            category, optional, python, platform = self._get_tags_for_package(
-                package, graph
-            )
-
-            package.category = category
-            package.optional = optional
-
-            # If requirements are empty, drop them
-            requirements = {}
-            if python is not None and python != "*":
-                requirements["python"] = python
-
-            if platform is not None and platform != "*":
-                requirements["platform"] = platform
-
-            package.requirements = requirements
 
         operations = []
         for package in packages:
@@ -104,9 +77,74 @@ class Solver:
             key=lambda o: (
                 1 if o.package.name in requested_names else 0,
                 o.package.name,
+                o.package.version,
             ),
         )
 
+    def solve_in_compatibility_mode(self, constraints, use_latest=None):
+        locked = {}
+        for package in self._locked.packages:
+            locked[package.name] = package
+
+        packages = []
+        for constraint in constraints:
+            constraint = parse_constraint(constraint)
+            intersection = constraint.intersect(self._package.python_constraint)
+
+            with self._package.with_python_versions(str(intersection)):
+                for package in self._solve(use_latest=use_latest):
+                    if package not in packages:
+                        packages.append(package)
+                        continue
+
+                    current_package = packages[packages.index(package)]
+                    for dep in package.requires:
+                        if dep not in current_package.requires:
+                            current_package.requires.append(dep)
+
+        return list(set(packages))
+
+    def _solve(self, use_latest=None):
+        locked = {}
+        for package in self._locked.packages:
+            locked[package.name] = package
+
+        try:
+            result = resolve_version(
+                self._package, self._provider, locked=locked, use_latest=use_latest
+            )
+
+            packages = result.packages
+        except CompatibilityError as e:
+            return self.solve_in_compatibility_mode(
+                e.constraints, use_latest=use_latest
+            )
+        except SolveFailure as e:
+            raise SolverProblemError(e)
+
+        requested = self._package.all_requires
+        graph = self._build_graph(self._package, packages)
+
+        for package in packages:
+            category, optional, python, platform = self._get_tags_for_package(
+                package, graph
+            )
+
+            package.category = category
+            package.optional = optional
+
+            # If requirements are empty, drop them
+            requirements = {}
+            if python is not None and python != "*":
+                requirements["python"] = python
+
+            if platform is not None and platform != "*":
+                requirements["platform"] = platform
+
+            package.requirements = requirements
+
+        return packages
+
     def _build_graph(self, package, packages, previous=None, dep=None):
         if not previous:
             category = "dev"

poetry/repositories/pool.py

@@ -62,10 +62,6 @@ class Pool(BaseRepository):
         raise NotImplementedError()
 
     def package(self, name, version, extras=None):
-        package = poetry.packages.Package(name, version, version)
-        if package in self._packages:
-            return self._packages[self._packages.index(package)]
-
         for repository in self._repositories:
             package = repository.package(name, version, extras=extras)
             if package:

poetry/repositories/pypi_repository.py

@@ -130,68 +130,61 @@ class PyPiRepository(Repository):
         version,  # type: str
         extras=None,  # type: (Union[list, None])
     ):  # type: (...) -> Union[Package, None]
-        try:
-            index = self._packages.index(Package(name, version, version))
-
-            return self._packages[index]
-        except ValueError:
-            if extras is None:
-                extras = []
-
-            release_info = self.get_release_info(name, version)
-            package = Package(name, version, version)
-            requires_dist = release_info["requires_dist"] or []
-            for req in requires_dist:
-                try:
-                    dependency = dependency_from_pep_508(req)
-                except InvalidMarker:
-                    # Invalid marker
-                    # We strip the markers hoping for the best
-                    req = req.split(";")[0]
-
-                    dependency = dependency_from_pep_508(req)
-                except ValueError:
-                    # Likely unable to parse constraint so we skip it
-                    self._log(
-                        "Invalid constraint ({}) found in {}-{} dependencies, "
-                        "skipping".format(req, package.name, package.version),
-                        level="debug",
-                    )
-                    continue
+        if extras is None:
+            extras = []
 
-                if dependency.extras:
-                    for extra in dependency.extras:
-                        if extra not in package.extras:
-                            package.extras[extra] = []
+        release_info = self.get_release_info(name, version)
+        package = Package(name, version, version)
+        requires_dist = release_info["requires_dist"] or []
+        for req in requires_dist:
+            try:
+                dependency = dependency_from_pep_508(req)
+            except InvalidMarker:
+                # Invalid marker
+                # We strip the markers hoping for the best
+                req = req.split(";")[0]
+
+                dependency = dependency_from_pep_508(req)
+            except ValueError:
+                # Likely unable to parse constraint so we skip it
+                self._log(
+                    "Invalid constraint ({}) found in {}-{} dependencies, "
+                    "skipping".format(req, package.name, package.version),
+                    level="debug",
+                )
+                continue
 
-                        package.extras[extra].append(dependency)
+            if dependency.extras:
+                for extra in dependency.extras:
+                    if extra not in package.extras:
+                        package.extras[extra] = []
 
-                if not dependency.is_optional():
-                    package.requires.append(dependency)
+                    package.extras[extra].append(dependency)
 
-            # Adding description
-            package.description = release_info.get("summary", "")
+            if not dependency.is_optional():
+                package.requires.append(dependency)
 
-            if release_info["requires_python"]:
-                package.python_versions = release_info["requires_python"]
+        # Adding description
+        package.description = release_info.get("summary", "")
 
-            if release_info["platform"]:
-                package.platform = release_info["platform"]
+        if release_info["requires_python"]:
+            package.python_versions = release_info["requires_python"]
 
-            # Adding hashes information
-            package.hashes = release_info["digests"]
+        if release_info["platform"]:
+            package.platform = release_info["platform"]
 
-            # Activate extra dependencies
-            for extra in extras:
-                if extra in package.extras:
-                    for dep in package.extras[extra]:
-                        dep.activate()
+        # Adding hashes information
+        package.hashes = release_info["digests"]
 
-                    package.requires += package.extras[extra]
+        # Activate extra dependencies
+        for extra in extras:
+            if extra in package.extras:
+                for dep in package.extras[extra]:
+                    dep.activate()
 
-            self._packages.append(package)
+                package.requires += package.extras[extra]
 
-            return package
+        return package
 
     def search(self, query, mode=0):
         results = []

poetry/repositories/repository.py

@@ -30,7 +30,7 @@ class Repository(BaseRepository):
                                 if dep.name == extra_dep.lower():
                                     dep.activate()
 
-                return package
+                return package.clone()
 
     def find_packages(
         self, name, constraint=None, extras=None, allow_prereleases=False

tests/installation/fixtures/update-with-locked-extras.test

@@ -9,7 +9,7 @@ platform = "*"
 
 [package.dependencies]
 "B" = "^1.0"
-"C" = "^1.0"
+"C" = {"version" = "^1.0", "python" = ">=2.7,<2.8"}
 
 [[package]]
 name = "B"

tests/installation/fixtures/with-duplicate-dependencies.test

+[[package]]
+name = "A"
+version = "1.0"
+description = ""
+category = "main"
+optional = false
+python-versions = "*"
+platform = "*"
+
+[package.dependencies]
+B = [
+    {"version" = "^1.0", "python" = "<4.0"},
+    {"version" = "^2.0", "python" = ">=4.0"},
+]
+
+[[package]]
+name = "B"
+version = "1.0"
+description = ""
+category = "main"
+optional = false
+python-versions = "*"
+platform = "*"
+
+[package.dependencies]
+C = "1.2"
+
+[package.requirements]
+python = "<4.0"
+
+[[package]]
+name = "B"
+version = "2.0"
+description = ""
+category = "main"
+optional = false
+python-versions = "*"
+platform = "*"
+
+[package.dependencies]
+C = "1.5"
+
+[package.requirements]
+python = ">=4.0"
+
+[[package]]
+name = "C"
+version = "1.2"
+description = ""
+category = "main"
+optional = false
+python-versions = "*"
+platform = "*"
+
+[[package]]
+name = "C"
+version = "1.5"
+description = ""
+category = "main"
+optional = false
+python-versions = "*"
+platform = "*"
+
+[metadata]
+python-versions = "*"
+platform = "*"
+content-hash = "123456789"
+
+[metadata.hashes]
+A = []
+B = []
+C = []

tests/installation/fixtures/with-pypi-repository.test

@@ -77,8 +77,8 @@ setuptools = "*"
 attrs = ">=17.4.0"
 more-itertools = ">=4.0.0"
 pluggy = ">=0.5,<0.7"
-funcsigs = "*"
-colorama = "*"
+funcsigs = {"version" = "*", "python" = "<3.0"}
+colorama = {"version" = "*", "platform" = "win32"}
 
 [[package]]
 name = "setuptools"

tests/installation/fixtures/with-python-versions.test

@@ -22,7 +22,7 @@ version = "1.2"
 description = ""
 category = "main"
 optional = false
-python-versions = "~2.7 || ^3.6"
+python-versions = "~2.7 || ^3.3"
 platform = "*"
 
 [metadata]

tests/installation/test_installer.py

@@ -279,7 +279,7 @@ def test_run_with_python_versions(installer, locker, repo, package):
     package_a = get_package("A", "1.0")
     package_b = get_package("B", "1.1")
     package_c12 = get_package("C", "1.2")
-    package_c12.python_versions = "~2.7 || ^3.6"
+    package_c12.python_versions = "~2.7 || ^3.3"
     package_c13 = get_package("C", "1.3")
     package_c13.python_versions = "~3.3"
 
@@ -740,3 +740,39 @@ def test_run_update_with_locked_extras(installer, locker, repo, package):
     expected = fixture("update-with-locked-extras")
 
     assert locker.written_data == expected
+
+
+def test_run_install_duplicate_dependencies_different_constraints(
+    installer, locker, repo, package
+):
+    package.add_dependency("A")
+
+    package_a = get_package("A", "1.0")
+    package_a.add_dependency("B", {"version": "^1.0", "python": "<4.0"})
+    package_a.add_dependency("B", {"version": "^2.0", "python": ">=4.0"})
+
+    package_b10 = get_package("B", "1.0")
+    package_b20 = get_package("B", "2.0")
+    package_b10.add_dependency("C", "1.2")
+    package_b20.add_dependency("C", "1.5")
+
+    package_c12 = get_package("C", "1.2")
+    package_c15 = get_package("C", "1.5")
+
+    repo.add_package(package_a)
+    repo.add_package(package_b10)
+    repo.add_package(package_b20)
+    repo.add_package(package_c12)
+    repo.add_package(package_c15)
+
+    installer.run()
+
+    expected = fixture("with-duplicate-dependencies")
+
+    assert locker.written_data == expected
+
+    installs = installer.installer.installs
+    assert len(installs) == 3
+    assert installs[0] == package_b10
+    assert installs[1] == package_c12
+    assert installs[2] == package_a

tests/puzzle/test_solver.py

@@ -284,9 +284,9 @@ def test_solver_sets_categories(solver, repo, package):
         ],
     )
 
-    assert package_c.category == "dev"
-    assert package_b.category == "dev"
-    assert package_a.category == "main"
+    assert ops[0].package.category == "dev"
+    assert ops[2].package.category == "dev"
+    assert ops[1].package.category == "main"
 
 
 def test_solver_respects_root_package_python_versions(solver, repo, package):
@@ -785,3 +785,76 @@ def test_solver_duplicate_dependencies_same_constraint(solver, repo, package):
 
     op = ops[0]
     assert op.package.requirements == {"python": "~2.7 || >=3.4"}
+
+
+def test_solver_duplicate_dependencies_different_constraints(solver, repo, package):
+    package.add_dependency("A")
+
+    package_a = get_package("A", "1.0")
+    package_a.add_dependency("B", {"version": "^1.0", "python": "<3.4"})
+    package_a.add_dependency("B", {"version": "^2.0", "python": ">=3.4"})
+
+    package_b10 = get_package("B", "1.0")
+    package_b20 = get_package("B", "2.0")
+
+    repo.add_package(package_a)
+    repo.add_package(package_b10)
+    repo.add_package(package_b20)
+
+    ops = solver.solve()
+
+    check_solver_result(
+        ops,
+        [
+            {"job": "install", "package": package_b10},
+            {"job": "install", "package": package_b20},
+            {"job": "install", "package": package_a},
+        ],
+    )
+
+    op = ops[0]
+    assert op.package.requirements == {"python": "<3.4"}
+
+    op = ops[1]
+    assert op.package.requirements == {"python": ">=3.4"}
+
+
+def test_solver_duplicate_dependencies_sub_dependencies(solver, repo, package):
+    package.add_dependency("A")
+
+    package_a = get_package("A", "1.0")
+    package_a.add_dependency("B", {"version": "^1.0", "python": "<3.4"})
+    package_a.add_dependency("B", {"version": "^2.0", "python": ">=3.4"})
+
+    package_b10 = get_package("B", "1.0")
+    package_b20 = get_package("B", "2.0")
+    package_b10.add_dependency("C", "1.2")
+    package_b20.add_dependency("C", "1.5")
+
+    package_c12 = get_package("C", "1.2")
+    package_c15 = get_package("C", "1.5")
+
+    repo.add_package(package_a)
+    repo.add_package(package_b10)
+    repo.add_package(package_b20)
+    repo.add_package(package_c12)
+    repo.add_package(package_c15)
+
+    ops = solver.solve()
+
+    check_solver_result(
+        ops,
+        [
+            {"job": "install", "package": package_b10},
+            {"job": "install", "package": package_b20},
+            {"job": "install", "package": package_c12},
+            {"job": "install", "package": package_c15},
+            {"job": "install", "package": package_a},
+        ],
+    )
+
+    op = ops[0]
+    assert op.package.requirements == {"python": "<3.4"}
+
+    op = ops[1]
+    assert op.package.requirements == {"python": ">=3.4"}

tests/semver/test_version.py

@@ -27,7 +27,6 @@ from poetry.semver import VersionRange
 )
 def test_parse_valid(input, version):
     parsed = Version.parse(input)
-    print(parsed.build)
 
     assert parsed == version
     assert parsed.text == input