Fix --elide-unused-requires-dist for inactive deps. (#2615)

jsirois · web-flow · commit f50af8f61cef · 2024-12-09T17:05:26.000-08:00
The feature released broken in 2.25.0 for any locked requirement that
had dependencies that would never be activated due to non-"extra"
environment markers.
diff --git a/CHANGES.md b/CHANGES.md
@@ -1,5 +1,11 @@
 # Release Notes
 
+## 2.25.1
+
+This is a hotfix release that fixes a bug in the implementation of the
+`--elide-unused-requires-dist` lock option introduced in Pex 2.25.0.
+
+* Fix `--elide-unused-requires-dist` for unactivated deps. (#2615)
 ## 2.25.0
 
 This release adds support for
diff --git a/pex/resolve/lockfile/model.py b/pex/resolve/lockfile/model.py
@@ -121,7 +121,7 @@ def extract_requirement(req):
             overridden=SortedTuple(overridden),
             locked_resolves=SortedTuple(
                 (
-                    requires_dist.remove_unused_requires_dist(resolve_requirements, locked_resolve)
+                    requires_dist.remove_unused_requires_dist(locked_resolve)
                     if elide_unused_requires_dist
                     else locked_resolve
                 )
diff --git a/pex/resolve/lockfile/requires_dist.py b/pex/resolve/lockfile/requires_dist.py
@@ -3,158 +3,36 @@
 
 from __future__ import absolute_import
 
-import operator
-from collections import defaultdict, deque
-
-from pex.dist_metadata import Requirement
-from pex.exceptions import production_assert
-from pex.orderedset import OrderedSet
-from pex.pep_503 import ProjectName
-from pex.resolve.locked_resolve import LockedRequirement, LockedResolve
+from pex.resolve.locked_resolve import LockedResolve
 from pex.sorted_tuple import SortedTuple
-from pex.third_party.packaging.markers import Marker, Variable
-from pex.typing import TYPE_CHECKING, cast
+from pex.typing import TYPE_CHECKING
 
 if TYPE_CHECKING:
-    from typing import Callable, DefaultDict, Dict, Iterable, List, Optional, Tuple, Union
-
     import attr  # vendor:skip
-
-    EvalExtra = Callable[[ProjectName], bool]
 else:
     from pex.third_party import attr
 
 
-_OPERATORS = {
-    "in": lambda lhs, rhs: lhs in rhs,
-    "not in": lambda lhs, rhs: lhs not in rhs,
-    "<": operator.lt,
-    "<=": operator.le,
-    "==": operator.eq,
-    "!=": operator.ne,
-    ">=": operator.ge,
-    ">": operator.gt,
-}
-
-
-class _Op(object):
-    def __init__(self, lhs):
-        self.lhs = lhs  # type: EvalExtra
-        self.rhs = None  # type: Optional[EvalExtra]
-
-
-class _And(_Op):
-    def __call__(self, extra):
-        # type: (ProjectName) -> bool
-        production_assert(self.rhs is not None)
-        return self.lhs(extra) and cast("EvalExtra", self.rhs)(extra)
-
-
-class _Or(_Op):
-    def __call__(self, extra):
-        # type: (ProjectName) -> bool
-        production_assert(self.rhs is not None)
-        return self.lhs(extra) or cast("EvalExtra", self.rhs)(extra)
-
-
-def _parse_extra_item(
-    stack,  # type: List[EvalExtra]
-    item,  # type: Union[str, List, Tuple]
-    marker,  # type: Marker
-):
-    # type: (...) -> None
-
-    if item == "and":
-        stack.append(_And(stack.pop()))
-    elif item == "or":
-        stack.append(_Or(stack.pop()))
-    elif isinstance(item, list):
-        for element in item:
-            _parse_extra_item(stack, element, marker)
-    elif isinstance(item, tuple):
-        lhs, op, rhs = item
-        if isinstance(lhs, Variable) and "extra" == str(lhs):
-            check = lambda extra: _OPERATORS[str(op)](extra, ProjectName(str(rhs)))
-        elif isinstance(rhs, Variable) and "extra" == str(rhs):
-            check = lambda extra: _OPERATORS[str(op)](extra, ProjectName(str(lhs)))
-        else:
-            # Any other condition could potentially be true.
-            check = lambda _: True
-        if stack:
-            production_assert(isinstance(stack[-1], _Op))
-            cast(_Op, stack[-1]).rhs = check
-        else:
-            stack.append(check)
-    else:
-        raise ValueError("Marker is invalid: {marker}".format(marker=marker))
+def remove_unused_requires_dist(locked_resolve):
+    # type: (LockedResolve) -> LockedResolve
 
-
-def _parse_extra_check(marker):
-    # type: (Marker) -> EvalExtra
-    checks = []  # type: List[EvalExtra]
-    for item in marker._markers:
-        _parse_extra_item(checks, item, marker)
-    production_assert(len(checks) == 1)
-    return checks[0]
-
-
-_EXTRA_CHECKS = {}  # type: Dict[str, EvalExtra]
-
-
-def _parse_marker_for_extra_check(marker):
-    # type: (Marker) -> EvalExtra
-    maker_str = str(marker)
-    eval_extra = _EXTRA_CHECKS.get(maker_str)
-    if not eval_extra:
-        eval_extra = _parse_extra_check(marker)
-        _EXTRA_CHECKS[maker_str] = eval_extra
-    return eval_extra
-
-
-def _evaluate_for_extras(
-    marker,  # type: Optional[Marker]
-    extras,  # type: Iterable[str]
-):
-    # type: (...) -> bool
-    if not marker:
-        return True
-    eval_extra = _parse_marker_for_extra_check(marker)
-    return any(eval_extra(ProjectName(extra)) for extra in (extras or [""]))
-
-
-def remove_unused_requires_dist(
-    resolve_requirements,  # type: Iterable[Requirement]
-    locked_resolve,  # type: LockedResolve
-):
-    # type: (...) -> LockedResolve
-
-    locked_req_by_project_name = {
-        locked_req.pin.project_name: locked_req for locked_req in locked_resolve.locked_requirements
+    locked_projects = {
+        locked_req.pin.project_name for locked_req in locked_resolve.locked_requirements
     }
-    requires_dist_by_locked_req = defaultdict(
-        OrderedSet
-    )  # type: DefaultDict[LockedRequirement, OrderedSet[Requirement]]
-    seen = set()
-    requirements = deque(resolve_requirements)
-    while requirements:
-        requirement = requirements.popleft()
-        if requirement in seen:
-            continue
-
-        seen.add(requirement)
-        locked_req = locked_req_by_project_name[requirement.project_name]
-        for dep in locked_req.requires_dists:
-            if _evaluate_for_extras(dep.marker, requirement.extras):
-                requires_dist_by_locked_req[locked_req].add(dep)
-                requirements.append(dep)
-
     return attr.evolve(
         locked_resolve,
         locked_requirements=SortedTuple(
             attr.evolve(
                 locked_requirement,
                 requires_dists=SortedTuple(
-                    requires_dist_by_locked_req[locked_requirement], key=str
+                    (
+                        requires_dist
+                        for requires_dist in locked_requirement.requires_dists
+                        # Otherwise, the requirement markers were never selected in the resolve
+                        # process; so the requirement was not locked.
+                        if requires_dist.project_name in locked_projects
+                    ),
+                    key=str,
                 ),
             )
             for locked_requirement in locked_resolve.locked_requirements
diff --git a/pex/version.py b/pex/version.py
@@ -1,4 +1,4 @@
 # Copyright 2015 Pex project contributors.
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 
-__version__ = "2.25.0"
+__version__ = "2.25.1"
diff --git a/tests/resolve/lockfile/test_requires_dist.py b/tests/resolve/lockfile/test_requires_dist.py
@@ -46,7 +46,7 @@ def test_remove_unused_requires_dist_noop():
         locked_req("baz", "1.0"),
     )
     assert locked_resolve_with_no_extras == requires_dist.remove_unused_requires_dist(
-        resolve_requirements=[req("foo")], locked_resolve=locked_resolve_with_no_extras
+        locked_resolve_with_no_extras
     )
 
 
@@ -58,8 +58,7 @@ def test_remove_unused_requires_dist_simple():
         locked_req("bar", "1.0"),
         locked_req("spam", "1.0"),
     ) == requires_dist.remove_unused_requires_dist(
-        resolve_requirements=[req("foo")],
-        locked_resolve=locked_resolve(
+        locked_resolve(
             locked_req("foo", "1.0", "bar", "baz; extra == 'tests'", "spam"),
             locked_req("bar", "1.0"),
             locked_req("spam", "1.0"),
@@ -75,8 +74,7 @@ def test_remove_unused_requires_dist_mixed_extras():
         locked_req("bar", "1.0"),
         locked_req("spam", "1.0"),
     ) == requires_dist.remove_unused_requires_dist(
-        resolve_requirements=[req("foo[extra1]")],
-        locked_resolve=locked_resolve(
+        locked_resolve(
             locked_req("foo", "1.0", "bar; extra == 'extra1'", "baz; extra == 'tests'", "spam"),
             locked_req("bar", "1.0"),
             locked_req("spam", "1.0"),
@@ -99,8 +97,7 @@ def test_remove_unused_requires_dist_mixed_markers():
         locked_req("baz", "1.0"),
         locked_req("spam", "1.0"),
     ) == requires_dist.remove_unused_requires_dist(
-        resolve_requirements=[req("foo[extra1]")],
-        locked_resolve=locked_resolve(
+        locked_resolve(
             locked_req(
                 "foo",
                 "1.0",
@@ -127,8 +124,7 @@ def test_remove_unused_requires_dist_mixed_markers():
         locked_req("bar", "1.0"),
         locked_req("spam", "1.0"),
     ) == requires_dist.remove_unused_requires_dist(
-        resolve_requirements=[req("foo[extra1]")],
-        locked_resolve=locked_resolve(
+        locked_resolve(
             locked_req(
                 "foo",
                 "1.0",
@@ -155,8 +151,7 @@ def test_remove_unused_requires_dist_complex_markers():
         locked_req("bar", "1.0"),
         locked_req("spam", "1.0"),
     ) == requires_dist.remove_unused_requires_dist(
-        resolve_requirements=[req("foo")],
-        locked_resolve=locked_resolve(
+        locked_resolve(
             locked_req(
                 "foo",
                 "1.0",
@@ -168,3 +163,28 @@ def test_remove_unused_requires_dist_complex_markers():
             locked_req("spam", "1.0"),
         ),
     )
+
+
+def test_remove_unused_requires_dist_not_present_due_to_other_markers():
+    # type: () -> None
+
+    assert locked_resolve(
+        locked_req("foo", "1.0", "bar", "spam"),
+        locked_req("bar", "1.0"),
+        locked_req("spam", "1.0"),
+    ) == requires_dist.remove_unused_requires_dist(
+        locked_resolve(
+            locked_req(
+                "foo",
+                "1.0",
+                "bar",
+                "baz; python_version < '3'",
+                "spam",
+            ),
+            locked_req("bar", "1.0"),
+            locked_req("spam", "1.0"),
+        ),
+    ), (
+        "Here we simulate a lock where baz is not present in the lock since the lock was for "
+        "Python 3. We expect the lack of a locked baz to not trip up the elision process."
+    )

Original file line number	Diff line number	Diff line change
`@@ -121,7 +121,7 @@ def extract_requirement(req):`
`121`	`121`	`overridden=SortedTuple(overridden),`
`122`	`122`	`locked_resolves=SortedTuple(`
`123`	`123`	`(`
`124`		`- requires_dist.remove_unused_requires_dist(resolve_requirements, locked_resolve)`
	`124`	`+ requires_dist.remove_unused_requires_dist(locked_resolve)`
`125`	`125`	`if elide_unused_requires_dist`
`126`	`126`	`else locked_resolve`
`127`	`127`	`)`