Properly propagate dependency markers (#1829)

poetry/packages/dependency.py

@@ -55,6 +55,7 @@ class Dependency(object):
         self._python_constraint = parse_constraint("*")
         self._transitive_python_versions = None
         self._transitive_python_constraint = None
+        self._transitive_marker = None
 
         self._extras = []
         self._in_extras = []
@@ -118,6 +119,17 @@ class Dependency(object):
         self._transitive_python_constraint = parse_constraint(value)
 
     @property
+    def transitive_marker(self):
+        if self._transitive_marker is None:
+            return self.marker
+
+        return self._transitive_marker
+
+    @transitive_marker.setter
+    def transitive_marker(self, value):
+        self._transitive_marker = value
+
+    @property
     def python_constraint(self):
         return self._python_constraint
 

poetry/packages/utils/utils.py

@@ -5,8 +5,13 @@ import re
 from poetry.packages.constraints.constraint import Constraint
 from poetry.packages.constraints.multi_constraint import MultiConstraint
 from poetry.packages.constraints.union_constraint import UnionConstraint
+from poetry.semver import EmptyConstraint
 from poetry.semver import Version
+from poetry.semver import VersionConstraint
+from poetry.semver import VersionRange
 from poetry.semver import VersionUnion
+from poetry.semver import parse_constraint
+from poetry.version.markers import BaseMarker
 from poetry.version.markers import MarkerUnion
 from poetry.version.markers import MultiMarker
 from poetry.version.markers import SingleMarker
@@ -236,3 +241,66 @@ def create_nested_marker(name, constraint):
         marker = '{} {} "{}"'.format(name, op, version)
 
     return marker
+
+
+def get_python_constraint_from_marker(
+    marker,
+):  # type: (BaseMarker) -> VersionConstraint
+    python_marker = marker.only("python_version")
+    if python_marker.is_any():
+        return VersionRange()
+
+    if python_marker.is_empty():
+        return EmptyConstraint()
+
+    markers = convert_markers(marker)
+
+    ors = []
+    for or_ in markers["python_version"]:
+        ands = []
+        for op, version in or_:
+            # Expand python version
+            if op == "==":
+                version = "~" + version
+                op = ""
+            elif op == "!=":
+                version += ".*"
+            elif op in ("<=", ">"):
+                parsed_version = Version.parse(version)
+                if parsed_version.precision == 1:
+                    if op == "<=":
+                        op = "<"
+                        version = parsed_version.next_major.text
+                    elif op == ">":
+                        op = ">="
+                        version = parsed_version.next_major.text
+                elif parsed_version.precision == 2:
+                    if op == "<=":
+                        op = "<"
+                        version = parsed_version.next_minor.text
+                    elif op == ">":
+                        op = ">="
+                        version = parsed_version.next_minor.text
+            elif op in ("in", "not in"):
+                versions = []
+                for v in re.split("[ ,]+", version):
+                    split = v.split(".")
+                    if len(split) in [1, 2]:
+                        split.append("*")
+                        op_ = "" if op == "in" else "!="
+                    else:
+                        op_ = "==" if op == "in" else "!="
+
+                    versions.append(op_ + ".".join(split))
+
+                glue = " || " if op == "in" else ", "
+                if versions:
+                    ands.append(glue.join(versions))
+
+                continue
+
+            ands.append("{}{}".format(op, version))
+
+        ors.append(" ".join(ands))
+
+    return parse_constraint(" || ".join(ors))

poetry/puzzle/provider.py

@@ -28,6 +28,7 @@ from poetry.packages import PackageCollection
 from poetry.packages import URLDependency
 from poetry.packages import VCSDependency
 from poetry.packages import dependency_from_pep_508
+from poetry.packages.utils.utils import get_python_constraint_from_marker
 from poetry.repositories import Pool
 from poetry.utils._compat import PY35
 from poetry.utils._compat import OrderedDict
@@ -489,14 +490,15 @@ class Provider:
             if not package.python_constraint.allows_all(
                 self._package.python_constraint
             ):
-                intersection = package.python_constraint.intersect(
-                    package.dependency.transitive_python_constraint
+                transitive_python_constraint = get_python_constraint_from_marker(
+                    package.dependency.transitive_marker
                 )
-                difference = package.dependency.transitive_python_constraint.difference(
-                    intersection
+                intersection = package.python_constraint.intersect(
+                    transitive_python_constraint
                 )
+                difference = transitive_python_constraint.difference(intersection)
                 if (
-                    package.dependency.transitive_python_constraint.is_any()
+                    transitive_python_constraint.is_any()
                     or self._package.python_constraint.intersect(
                         package.dependency.python_constraint
                     ).is_empty()
@@ -673,12 +675,24 @@ class Provider:
         # Modifying dependencies as needed
         clean_dependencies = []
         for dep in dependencies:
+            if not package.dependency.transitive_marker.without_extras().is_any():
+                marker_intersection = package.dependency.transitive_marker.without_extras().intersect(
+                    dep.marker.without_extras()
+                )
+                if marker_intersection.is_empty():
+                    # The dependency is not needed, since the markers specified
+                    # for the current package selection are not compatible with
+                    # the markers for the current dependency, so we skip it
+                    continue
+
+                dep.transitive_marker = marker_intersection
+
             if not package.dependency.python_constraint.is_any():
                 python_constraint_intersection = dep.python_constraint.intersect(
                     package.dependency.python_constraint
                 )
                 if python_constraint_intersection.is_empty():
-                    # This depencency is not needed under current python constraint.
+                    # This dependency is not needed under current python constraint.
                     continue
                 dep.transitive_python_versions = str(python_constraint_intersection)
 

poetry/puzzle/solver.py

@@ -225,7 +225,7 @@ class Solver:
             intersection = (
                 previous["marker"]
                 .without_extras()
-                .intersect(previous_dep.marker.without_extras())
+                .intersect(previous_dep.transitive_marker.without_extras())
             )
             intersection = intersection.intersect(package.marker.without_extras())
 

poetry/version/markers.py

@@ -175,6 +175,12 @@ class BaseMarker(object):
     def without_extras(self):  # type: () -> BaseMarker
         raise NotImplementedError()
 
+    def exclude(self, marker_name):  # type: (str) -> BaseMarker
+        raise NotImplementedError()
+
+    def only(self, marker_name):  # type: (str) -> BaseMarker
+        raise NotImplementedError()
+
     def __repr__(self):
         return "<{} {}>".format(self.__class__.__name__, str(self))
 
@@ -198,6 +204,12 @@ class AnyMarker(BaseMarker):
     def without_extras(self):
         return self
 
+    def exclude(self, marker_name):  # type: (str) -> AnyMarker
+        return self
+
+    def only(self, marker_name):  # type: (str) -> AnyMarker
+        return self
+
     def __str__(self):
         return ""
 
@@ -233,6 +245,12 @@ class EmptyMarker(BaseMarker):
     def without_extras(self):
         return self
 
+    def exclude(self, marker_name):  # type: (str) -> EmptyMarker
+        return self
+
+    def only(self, marker_name):  # type: (str) -> EmptyMarker
+        return self
+
     def __str__(self):
         return "<empty>"
 
@@ -361,11 +379,20 @@ class SingleMarker(BaseMarker):
         return self._constraint.allows(self._parser(environment[self._name]))
 
     def without_extras(self):
-        if self.name == "extra":
+        return self.exclude("extra")
+
+    def exclude(self, marker_name):  # type: (str) -> BaseMarker
+        if self.name == marker_name:
             return AnyMarker()
 
         return self
 
+    def only(self, marker_name):  # type: (str) -> BaseMarker
+        if self.name != marker_name:
+            return EmptyMarker()
+
+        return self
+
     def __eq__(self, other):
         if not isinstance(other, SingleMarker):
             return False
@@ -410,7 +437,7 @@ class MultiMarker(BaseMarker):
         markers = _flatten_markers(markers, MultiMarker)
 
         for marker in markers:
-            if marker in new_markers or marker.is_empty():
+            if marker in new_markers:
                 continue
 
             if isinstance(marker, SingleMarker):
@@ -426,11 +453,9 @@ class MultiMarker(BaseMarker):
                     intersection = mark.constraint.intersect(marker.constraint)
                     if intersection == mark.constraint:
                         intersected = True
-                        break
                     elif intersection == marker.constraint:
                         new_markers[i] = marker
                         intersected = True
-                        break
                     elif intersection.is_empty():
                         return EmptyMarker()
 
@@ -439,9 +464,12 @@ class MultiMarker(BaseMarker):
 
             new_markers.append(marker)
 
-        if not new_markers:
+        if any(m.is_empty() for m in new_markers) or not new_markers:
             return EmptyMarker()
 
+        if len(new_markers) == 1 and new_markers[0].is_any():
+            return AnyMarker()
+
         return MultiMarker(*new_markers)
 
     @property
@@ -473,10 +501,32 @@ class MultiMarker(BaseMarker):
         return True
 
     def without_extras(self):
+        return self.exclude("extra")
+
+    def exclude(self, marker_name):  # type: (str) -> BaseMarker
+        new_markers = []
+
+        for m in self._markers:
+            if isinstance(m, SingleMarker) and m.name == marker_name:
+                # The marker is not relevant since it must be excluded
+                continue
+
+            marker = m.exclude(marker_name)
+
+            if not marker.is_empty():
+                new_markers.append(marker)
+
+        return self.of(*new_markers)
+
+    def only(self, marker_name):  # type: (str) -> BaseMarker
         new_markers = []
 
         for m in self._markers:
-            marker = m.without_extras()
+            if isinstance(m, SingleMarker) and m.name != marker_name:
+                # The marker is not relevant since it's not one we want
+                continue
+
+            marker = m.only(marker_name)
 
             if not marker.is_empty():
                 new_markers.append(marker)
@@ -550,7 +600,7 @@ class MarkerUnion(BaseMarker):
 
             markers.append(marker)
 
-        if len(markers) == 1 and markers[0].is_any():
+        if any(m.is_any() for m in markers):
             return AnyMarker()
 
         return MarkerUnion(*markers)
@@ -604,15 +654,37 @@ class MarkerUnion(BaseMarker):
         return False
 
     def without_extras(self):
+        return self.exclude("extra")
+
+    def exclude(self, marker_name):  # type: (str) -> BaseMarker
         new_markers = []
 
         for m in self._markers:
-            marker = m.without_extras()
+            if isinstance(m, SingleMarker) and m.name == marker_name:
+                # The marker is not relevant since it must be excluded
+                continue
+
+            marker = m.exclude(marker_name)
 
             if not marker.is_empty():
                 new_markers.append(marker)
 
-        return MarkerUnion(*new_markers)
+        return self.of(*new_markers)
+
+    def only(self, marker_name):  # type: (str) -> BaseMarker
+        new_markers = []
+
+        for m in self._markers:
+            if isinstance(m, SingleMarker) and m.name != marker_name:
+                # The marker is not relevant since it's not one we want
+                continue
+
+            marker = m.only(marker_name)
+
+            if not marker.is_empty():
+                new_markers.append(marker)
+
+        return self.of(*new_markers)
 
     def __eq__(self, other):
         if not isinstance(other, MarkerUnion):

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

@@ -23,6 +23,7 @@ C = "1.5"
 [[package]]
 name = "C"
 version = "1.5"
+marker = "python_version >= \"2.7\""
 description = ""
 category = "main"
 optional = false

tests/installation/test_installer.py

@@ -1183,8 +1183,8 @@ def test_run_install_duplicate_dependencies_different_constraints_with_lock_upda
                     "checksum": [],
                     "dependencies": {
                         "B": [
-                            {"version": "^1.0", "python": "<4.0"},
-                            {"version": "^2.0", "python": ">=4.0"},
+                            {"version": "^1.0", "python": "<2.7"},
+                            {"version": "^2.0", "python": ">=2.7"},
                         ]
                     },
                 },
@@ -1197,7 +1197,7 @@ def test_run_install_duplicate_dependencies_different_constraints_with_lock_upda
                     "python-versions": "*",
                     "checksum": [],
                     "dependencies": {"C": "1.2"},
-                    "requirements": {"python": "<4.0"},
+                    "requirements": {"python": "<2.7"},
                 },
                 {
                     "name": "B",
@@ -1208,7 +1208,7 @@ def test_run_install_duplicate_dependencies_different_constraints_with_lock_upda
                     "python-versions": "*",
                     "checksum": [],
                     "dependencies": {"C": "1.5"},
-                    "requirements": {"python": ">=4.0"},
+                    "requirements": {"python": ">=2.7"},
                 },
                 {
                     "name": "C",

tests/puzzle/test_solver.py

@@ -1903,3 +1903,28 @@ def test_ignore_python_constraint_no_overlap_dependencies(solver, repo, package)
     check_solver_result(
         ops, [{"job": "install", "package": pytest}],
     )
+
+
+def test_solver_properly_propagates_markers(solver, repo, package):
+    package.python_versions = "~2.7 || ^3.4"
+    package.add_dependency(
+        "A",
+        {
+            "version": "^1.0",
+            "markers": "python_version >= '3.6' and implementation_name != 'pypy'",
+        },
+    )
+
+    package_a = get_package("A", "1.0.0")
+    package_a.python_versions = ">=3.6"
+
+    repo.add_package(package_a)
+
+    ops = solver.solve()
+
+    check_solver_result(ops, [{"job": "install", "package": package_a}])
+
+    assert (
+        str(ops[0].package.marker)
+        == 'python_version >= "3.6" and implementation_name != "pypy"'
+    )

tests/version/test_markers.py

@@ -310,6 +310,24 @@ def test_marker_union_intersect_marker_union():
     )
 
 
+def test_marker_union_intersect_marker_union_drops_unnecessary_markers():
+    m = parse_marker(
+        'python_version >= "2.7" and python_version < "2.8" '
+        'or python_version >= "3.4" and python_version < "4.0"'
+    )
+    m2 = parse_marker(
+        'python_version >= "2.7" and python_version < "2.8" '
+        'or python_version >= "3.4" and python_version < "4.0"'
+    )
+
+    intersection = m.intersect(m2)
+    expected = (
+        'python_version >= "2.7" and python_version < "2.8" '
+        'or python_version >= "3.4" and python_version < "4.0"'
+    )
+    assert expected == str(intersection)
+
+
 def test_marker_union_intersect_multi_marker():
     m = parse_marker('sys_platform == "darwin" or python_version < "3.4"')
 
@@ -479,3 +497,110 @@ def test_parse_version_like_markers(marker, env):
     m = parse_marker(marker)
 
     assert m.validate(env)
+
+
+@pytest.mark.parametrize(
+    "marker, expected",
+    [
+        ('python_version >= "3.6"', 'python_version >= "3.6"'),
+        ('python_version >= "3.6" and extra == "foo"', 'python_version >= "3.6"'),
+        (
+            'python_version >= "3.6" and (extra == "foo" or extra == "bar")',
+            'python_version >= "3.6"',
+        ),
+        (
+            'python_version >= "3.6" and (extra == "foo" or extra == "bar") or implementation_name == "pypy"',
+            'python_version >= "3.6" or implementation_name == "pypy"',
+        ),
+        (
+            'python_version >= "3.6" and extra == "foo" or implementation_name == "pypy" and extra == "bar"',
+            'python_version >= "3.6" or implementation_name == "pypy"',
+        ),
+        (
+            'python_version >= "3.6" or extra == "foo" and implementation_name == "pypy" or extra == "bar"',
+            'python_version >= "3.6" or implementation_name == "pypy"',
+        ),
+    ],
+)
+def test_without_extras(marker, expected):
+    m = parse_marker(marker)
+
+    assert expected == str(m.without_extras())
+
+
+@pytest.mark.parametrize(
+    "marker, excluded, expected",
+    [
+        ('python_version >= "3.6"', "implementation_name", 'python_version >= "3.6"'),
+        ('python_version >= "3.6"', "python_version", "*"),
+        (
+            'python_version >= "3.6" and extra == "foo"',
+            "extra",
+            'python_version >= "3.6"',
+        ),
+        (
+            'python_version >= "3.6" and (extra == "foo" or extra == "bar")',
+            "python_version",
+            '(extra == "foo" or extra == "bar")',
+        ),
+        (
+            'python_version >= "3.6" and (extra == "foo" or extra == "bar") or implementation_name == "pypy"',
+            "python_version",
+            '(extra == "foo" or extra == "bar") or implementation_name == "pypy"',
+        ),
+        (
+            'python_version >= "3.6" and extra == "foo" or implementation_name == "pypy" and extra == "bar"',
+            "implementation_name",
+            'python_version >= "3.6" and extra == "foo" or extra == "bar"',
+        ),
+        (
+            'python_version >= "3.6" or extra == "foo" and implementation_name == "pypy" or extra == "bar"',
+            "implementation_name",
+            'python_version >= "3.6" or extra == "foo" or extra == "bar"',
+        ),
+    ],
+)
+def test_exclude(marker, excluded, expected):
+    m = parse_marker(marker)
+
+    if expected == "*":
+        assert m.exclude(excluded).is_any()
+    else:
+        assert expected == str(m.exclude(excluded))
+
+
+@pytest.mark.parametrize(
+    "marker, only, expected",
+    [
+        ('python_version >= "3.6"', "python_version", 'python_version >= "3.6"'),
+        (
+            'python_version >= "3.6" and extra == "foo"',
+            "python_version",
+            'python_version >= "3.6"',
+        ),
+        (
+            'python_version >= "3.6" and (extra == "foo" or extra == "bar")',
+            "extra",
+            '(extra == "foo" or extra == "bar")',
+        ),
+        (
+            'python_version >= "3.6" and (extra == "foo" or extra == "bar") or implementation_name == "pypy"',
+            "implementation_name",
+            'implementation_name == "pypy"',
+        ),
+        (
+            'python_version >= "3.6" and extra == "foo" or implementation_name == "pypy" and extra == "bar"',
+            "implementation_name",
+            'implementation_name == "pypy"',
+        ),
+        (
+            'python_version >= "3.6" or extra == "foo" and implementation_name == "pypy" or extra == "bar"',
+            "implementation_name",
+            'implementation_name == "pypy"',
+        ),
+    ],
+)
+def test_only(marker, only, expected):
+    m = parse_marker(marker)
+
+    assert expected == str(m.only(only))