Fix Python requirements not properly set when resolving dependencies

CHANGELOG.md

@@ -6,6 +6,7 @@
 
 - Fixed handling of `in` environment markers with commas.
 - Fixed a `UnicodeDecodeError` when an error occurs in venv.
+- Fixed Python requirements not properly set when resolving dependencies.
 
 
 ## [0.10.1] - 2018-05-28

poetry/packages/dependency.py

@@ -44,6 +44,8 @@ class Dependency(object):
         self._extras = []
         self._in_extras = []
 
+        self._activated = not self._optional
+
         self.is_root = False
 
     @property
@@ -106,6 +108,9 @@ class Dependency(object):
     def is_optional(self):
         return self._optional
 
+    def is_activated(self):
+        return self._activated
+
     def is_vcs(self):
         return False
 
@@ -244,13 +249,16 @@ class Dependency(object):
         """
         Set the dependency as mandatory.
         """
-        self._optional = False
+        self._activated = True
 
     def deactivate(self):
         """
         Set the dependency as optional.
         """
-        self._optional = True
+        if not self._optional:
+            self._optional = True
+
+        self._activated = False
 
     def with_constraint(self, constraint):
         new = Dependency(

poetry/packages/locker.py

@@ -171,7 +171,7 @@ class Locker:
     def _dump_package(self, package):  # type: (poetry.packages.Package) -> dict
         dependencies = {}
         for dependency in package.requires:
-            if dependency.is_optional():
+            if dependency.is_optional() and not dependency.is_activated():
                 continue
 
             dependencies[dependency.pretty_name] = str(dependency.pretty_constraint)

poetry/puzzle/provider.py

@@ -303,7 +303,7 @@ class Provider:
             return [
                 r
                 for r in package.requires
-                if not r.is_optional() and r.name not in self.UNSAFE_PACKAGES
+                if not r.is_activated() and r.name not in self.UNSAFE_PACKAGES
             ]
         else:
             return Dependencies(package, self)
@@ -324,7 +324,7 @@ class Provider:
         return [
             r
             for r in package.requires
-            if not r.is_optional()
+            if r.is_activated()
             and self._package.python_constraint.allows_any(r.python_constraint)
             and self._package.platform_constraint.matches(package.platform_constraint)
             and r.name not in self.UNSAFE_PACKAGES

poetry/puzzle/solver.py

@@ -41,8 +41,9 @@ class Solver:
         requested = self._package.all_requires
 
         for package in packages:
+            graph = self._build_graph(self._package, packages)
             category, optional, python, platform = self._get_tags_for_package(
-                package, packages, requested
+                package, graph
             )
 
             package.category = category
@@ -106,164 +107,111 @@ class Solver:
             ),
         )
 
-    def _get_graph_for_package(self, package, packages, requested, original=None):
+    def _build_graph(self, package, packages, previous=None, dep=None):
+        if not previous:
+            category = "dev"
+            optional = True
+            python_version = None
+            platform = None
+        else:
+            category = dep.category
+            optional = dep.is_optional() and not dep.is_activated()
+            python_version = (
+                dep.python_versions
+                if previous.python_constraint.allows_all(dep.python_constraint)
+                else previous.python_versions
+            )
+            platform = (
+                dep.platform
+                if previous.platform_constraint.matches(dep.platform_constraint)
+                and dep.platform != "*"
+                else previous.platform
+            )
+
         graph = {
-            package.name: {
-                "category": "dev",
-                "optional": True,
-                "python_version": None,
-                "platform": None,
-                "dependencies": {},
-                "parents": {},
-            }
+            "name": package.name,
+            "category": category,
+            "optional": optional,
+            "python_version": python_version,
+            "platform": platform,
+            "children": [],
         }
 
-        roots = []
-        for dep in requested:
-            if dep.name == package.name:
-                roots.append(dep)
+        if previous and previous is not dep and previous.name == dep.name:
+            return graph
 
-        origins = []
-        for pkg in packages:
-            for dep in pkg.all_requires:
-                if original and original.name == pkg.name:
-                    # Circular dependency
+        for dependency in package.all_requires:
+            if dependency.is_optional():
+                if not package.is_root() and (not dep or not dep.extras):
                     continue
 
-                if dep.name == package.name:
-                    origins.append((pkg, dep))
-
-        if roots and (not origins or len(roots) > 1):
-            # Root dependency
-            if len(roots) == 1:
-                root = roots[0]
-            else:
-                root1 = [r for r in roots if r.category == "main"][0]
-                root2 = [r for r in roots if r.category == "dev"][0]
-                if root1.extras == root2.extras or original is None:
-                    root = root1
-                else:
-                    root1_extra_dependencies = []
-                    for extra in root1.extras:
-                        if extra in package.extras:
-                            for dep in package.extras[extra]:
-                                root1_extra_dependencies.append(dep.name)
-
-                    root2_extra_dependencies = []
-                    for extra in root2.extras:
-                        if extra in package.extras:
-                            for dep in package.extras[extra]:
-                                root2_extra_dependencies.append(dep.name)
-
-                    if (
-                        original.name in root1_extra_dependencies
-                        and original.name in root2_extra_dependencies
-                    ):
-                        root = root1
-                    elif original.name in root2_extra_dependencies:
-                        root = root2
+                is_activated = False
+                for group, extras in package.extras.items():
+                    if dep:
+                        extras = dep.extras
+                    elif package.is_root():
+                        extras = package.extras
                     else:
-                        root = root1
-
-            category = root.category
-            optional = root.is_optional()
+                        extras = []
 
-            python_version = str(root.python_constraint)
-            platform = str(root.platform_constraint)
-
-            graph[package.name]["category"] = category
-            graph[package.name]["optional"] = optional
-            graph[package.name]["python_version"] = python_version
-            graph[package.name]["platform"] = platform
+                    if group in extras:
+                        is_activated = True
+                        break
 
-            return graph
+                if not is_activated:
+                    continue
 
-        for pkg, dep in origins:
-            graph[package.name]["dependencies"][pkg.name] = {
-                "constraint": dep.pretty_constraint,
-                "python_version": dep.python_versions,
-                "platform": dep.platform,
-            }
-            graph[package.name]["parents"].update(
-                self._get_graph_for_package(pkg, packages, requested, original=package)
-            )
+            for pkg in packages:
+                if pkg.name == dependency.name:
+                    graph["children"].append(
+                        self._build_graph(pkg, packages, dependency, dep or dependency)
+                    )
 
         return graph
 
-    def _get_tags_for_package(self, package, packages, requested):
-        graph = self._get_graph_for_package(package, packages, requested)[package.name]
-
-        return self._get_tags_from_graph(graph, packages)
-
-    def _get_tags_from_graph(self, graph, packages):
-        category = graph["category"]
-        optional = graph["optional"]
-        python_version = graph["python_version"]
-        platform = graph["platform"]
-
-        if not graph["parents"]:
-            # Root dependency
-            return category, optional, python_version, platform
-
+    def _get_tags_for_package(self, package, graph):
+        categories = ["dev"]
+        optionals = [True]
         python_versions = []
         platforms = []
 
-        for parent_name, parent_graph in graph["parents"].items():
-            dep_python_version = graph["dependencies"][parent_name]["python_version"]
-            dep_platform = graph["dependencies"][parent_name]["platform"]
-
-            for pkg in packages:
-                if pkg.name == parent_name:
-                    (
-                        top_category,
-                        top_optional,
-                        top_python_version,
-                        top_platform,
-                    ) = self._get_tags_from_graph(parent_graph, packages)
-
-                    if category is None or category != "main":
-                        category = top_category
-
-                    optional = optional and top_optional
-
-                    # Take the most restrictive constraints
-                    if top_python_version is not None:
-                        if dep_python_version is not None:
-                            previous = parse_constraint(dep_python_version)
-                            current = parse_constraint(top_python_version)
-
-                            if previous.allows_all(current):
-                                python_versions.append(top_python_version)
-                            else:
-                                python_versions.append(dep_python_version)
-                        else:
-                            python_versions.append(top_python_version)
-                    elif dep_python_version is not None:
-                        python_versions.append(dep_python_version)
-
-                    if top_platform is not None:
-                        if dep_platform is not None:
-                            previous = GenericConstraint.parse(dep_platform)
-                            current = GenericConstraint.parse(top_platform)
-
-                            if top_platform != "*" and previous.matches(current):
-                                platforms.append(top_platform)
-                            else:
-                                platforms.append(dep_platform)
-                        else:
-                            platforms.append(top_platform)
-                    elif dep_platform is not None:
-                        platforms.append(dep_platform)
+        children = graph["children"]
+        for child in children:
+            if child["name"] == package.name:
+                category = child["category"]
+                optional = child["optional"]
+                python_version = child["python_version"]
+                platform = child["platform"]
+            else:
+                (
+                    category,
+                    optional,
+                    python_version,
+                    platform,
+                ) = self._get_tags_for_package(package, child)
+
+            categories.append(category)
+            optionals.append(optional)
+            if python_version is not None:
+                python_versions.append(python_version)
+
+            if platform is not None:
+                platforms.append(platform)
+
+        if "main" in categories:
+            category = "main"
+        else:
+            category = "dev"
 
-                    break
+        optional = all(optionals)
 
         if not python_versions:
             python_version = None
         else:
             # Find the least restrictive constraint
             python_version = python_versions[0]
-            previous = parse_constraint(python_version)
             for constraint in python_versions[1:]:
+                previous = parse_constraint(python_version)
                 current = parse_constraint(constraint)
 
                 if python_version == "*":
@@ -277,8 +225,8 @@ class Solver:
             platform = None
         else:
             platform = platforms[0]
-            previous = GenericConstraint.parse(platform)
             for constraint in platforms[1:]:
+                previous = GenericConstraint.parse(platform)
                 current = GenericConstraint.parse(constraint)
 
                 if platform == "*":

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

@@ -17,7 +17,7 @@ python-versions = "*"
 platform = "*"
 
 [package.requirements]
-python = ">=2.4,<2.5"
+python = "~2.4"
 
 [[package]]
 name = "C"
@@ -32,7 +32,7 @@ platform = "*"
 D = "^1.2"
 
 [package.requirements]
-python = ">=2.7,<2.8 || >=3.4,<4.0"
+python = "~2.7 || ^3.4"
 
 [[package]]
 name = "D"
@@ -44,7 +44,10 @@ python-versions = "*"
 platform = "*"
 
 [package.requirements]
-python = ">=2.7,<2.8 || >=3.4,<4.0"
+python = "~2.7 || ^3.4"
+
+[extras]
+foo = ["A"]
 
 [metadata]
 python-versions = "~2.7 || ^3.4"

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

@@ -46,6 +46,9 @@ platform = "*"
 [package.requirements]
 platform = "darwin"
 
+[extras]
+foo = ["A"]
+
 [metadata]
 python-versions = "*"
 platform = "*"

tests/installation/test_installer.py

@@ -316,6 +316,7 @@ def test_run_with_optional_and_python_restricted_dependencies(
     repo.add_package(package_c13)
     repo.add_package(package_d)
 
+    package.extras = {"foo": [get_dependency("A", "~1.0")]}
     package.add_dependency("A", {"version": "~1.0", "optional": True})
     package.add_dependency("B", {"version": "^1.0", "python": "~2.4"})
     package.add_dependency("C", {"version": "^1.0", "python": "~2.7 || ^3.4"})
@@ -350,6 +351,7 @@ def test_run_with_optional_and_platform_restricted_dependencies(
     repo.add_package(package_c13)
     repo.add_package(package_d)
 
+    package.extras = {"foo": [get_dependency("A", "~1.0")]}
     package.add_dependency("A", {"version": "~1.0", "optional": True})
     package.add_dependency("B", {"version": "^1.0", "platform": "custom"})
     package.add_dependency("C", {"version": "^1.0", "platform": "darwin"})

tests/puzzle/test_solver.py

@@ -540,9 +540,9 @@ def test_solver_sub_dependencies_with_requirements(solver, repo, package):
 
 
 def test_solver_sub_dependencies_with_requirements_complex(solver, repo, package):
-    package.add_dependency("A")
-    package.add_dependency("B")
-    package.add_dependency("C")
+    package.add_dependency("A", {"version": "^1.0", "python": "<5.0"})
+    package.add_dependency("B", {"version": "^1.0", "python": "<5.0"})
+    package.add_dependency("C", {"version": "^1.0", "python": "<4.0"})
 
     package_a = get_package("A", "1.0")
     package_b = get_package("B", "1.0")
@@ -551,11 +551,11 @@ def test_solver_sub_dependencies_with_requirements_complex(solver, repo, package
     package_e = get_package("E", "1.0")
     package_f = get_package("F", "1.0")
 
-    package_a.add_dependency("B", "^1.0")
+    package_a.add_dependency("B", {"version": "^1.0", "python": "<4.0"})
     package_a.add_dependency("D", {"version": "^1.0", "python": "<4.0"})
     package_b.add_dependency("E", {"version": "^1.0", "platform": "win32"})
-    package_b.add_dependency("F")
-    package_c.add_dependency("F", "^1.0")
+    package_b.add_dependency("F", {"version": "^1.0", "python": "<5.0"})
+    package_c.add_dependency("F", {"version": "^1.0", "python": "<4.0"})
     package_d.add_dependency("F")
 
     repo.add_package(package_a)
@@ -579,11 +579,17 @@ def test_solver_sub_dependencies_with_requirements_complex(solver, repo, package
         ],
     )
 
+    op = ops[0]
+    assert op.package.requirements == {"python": "<4.0"}
+
     op = ops[1]
-    assert op.package.requirements == {"platform": "win32"}
+    assert op.package.requirements == {"platform": "win32", "python": "<5.0"}
 
     op = ops[2]
-    assert op.package.requirements == {}
+    assert op.package.requirements == {"python": "<5.0"}
+
+    op = ops[4]
+    assert op.package.requirements == {"python": "<5.0"}
 
 
 def test_solver_sub_dependencies_with_not_supported_python_version(
@@ -730,3 +736,26 @@ def test_solver_with_dependency_and_prerelease_sub_dependencies(solver, repo, pa
             {"job": "install", "package": package_a},
         ],
     )
+
+
+def test_solver_circular_dependency(solver, repo, package):
+    package.add_dependency("A")
+
+    package_a = get_package("A", "1.0")
+    package_a.add_dependency("B", "^1.0")
+
+    package_b = get_package("B", "1.0")
+    package_b.add_dependency("A", "^1.0")
+
+    repo.add_package(package_a)
+    repo.add_package(package_b)
+
+    ops = solver.solve()
+
+    check_solver_result(
+        ops,
+        [
+            {"job": "install", "package": package_b},
+            {"job": "install", "package": package_a},
+        ],
+    )