From c5d8d9b3bf9d7d4c264416e9702473c63a8f070c Mon Sep 17 00:00:00 2001 From: John Sirois Date: Tue, 10 Dec 2024 18:58:43 -0800 Subject: [PATCH] Fix `--elide-unused-requires-dist`: don't expose partial extras. (#2618) A combination of `extra` environment marker evaluation and trimming of any requires-dists that are not locked is used to fully prune the locked graph of unused requires-dist entries. --- CHANGES.md | 17 ++ pex/resolve/lockfile/model.py | 2 +- pex/resolve/lockfile/requires_dist.py | 155 +++++++++++++++++-- pex/version.py | 2 +- tests/resolve/lockfile/test_requires_dist.py | 20 ++- 5 files changed, 173 insertions(+), 23 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 871ea19d6..b9ef305a6 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,22 @@ # Release Notes +## 2.25.2 + +This release fixes the `--elide-unused-requires-dist` lock option once +again. The fix in 2.25.1 could lead to locked requirements having only +a partial graph of extras which would allow a subsequent subset of those +partial extras to silently resolve an incomplete set of dependencies. + +In addition, the Pex REPL for PEXes without entry points or else when +forced with `PEX_INTERPRETER=1` is now fixed such that readline support +always works. Previously, the yellow foreground color applied to the PS1 +and PS2 prompts would interfere with the tracked cursor position in some +Pythons; so the yellow foreground color for those prompts is now +dropped. + +* Fix `--elide-unused-requires-dist`: don't expose partial extras. (#2618) +* Fix Pex REPL prompt. (#2617) + ## 2.25.1 This is a hotfix release that fixes a bug in the implementation of the diff --git a/pex/resolve/lockfile/model.py b/pex/resolve/lockfile/model.py index f2be6e820..36293fa34 100644 --- 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(locked_resolve) + requires_dist.remove_unused_requires_dist(resolve_requirements, 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 index 3f692c5fc..b10b8c728 100644 --- a/pex/resolve/lockfile/requires_dist.py +++ b/pex/resolve/lockfile/requires_dist.py @@ -3,36 +3,163 @@ from __future__ import absolute_import -from pex.resolve.locked_resolve import LockedResolve +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.sorted_tuple import SortedTuple -from pex.typing import TYPE_CHECKING +from pex.third_party.packaging.markers import Marker, Variable +from pex.typing import TYPE_CHECKING, cast 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 -def remove_unused_requires_dist(locked_resolve): - # type: (LockedResolve) -> LockedResolve +_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)) + - locked_projects = { - locked_req.pin.project_name for locked_req in locked_resolve.locked_requirements +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 } + 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.get(requirement.project_name) + if not locked_req: + continue + + for dep in locked_req.requires_dists: + if dep.project_name in locked_req_by_project_name and _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 - 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, + requires_dist_by_locked_req[locked_requirement], key=str ), ) for locked_requirement in locked_resolve.locked_requirements diff --git a/pex/version.py b/pex/version.py index 2549afdc6..00c0c1246 100644 --- 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.1" +__version__ = "2.25.2" diff --git a/tests/resolve/lockfile/test_requires_dist.py b/tests/resolve/lockfile/test_requires_dist.py index 744a9e612..8e65d1e9f 100644 --- 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( - locked_resolve_with_no_extras + resolve_requirements=[req("foo")], locked_resolve=locked_resolve_with_no_extras ) @@ -58,7 +58,8 @@ def test_remove_unused_requires_dist_simple(): locked_req("bar", "1.0"), locked_req("spam", "1.0"), ) == requires_dist.remove_unused_requires_dist( - locked_resolve( + resolve_requirements=[req("foo")], + locked_resolve=locked_resolve( locked_req("foo", "1.0", "bar", "baz; extra == 'tests'", "spam"), locked_req("bar", "1.0"), locked_req("spam", "1.0"), @@ -74,7 +75,8 @@ def test_remove_unused_requires_dist_mixed_extras(): locked_req("bar", "1.0"), locked_req("spam", "1.0"), ) == requires_dist.remove_unused_requires_dist( - locked_resolve( + resolve_requirements=[req("foo[extra1]")], + 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"), @@ -97,7 +99,8 @@ def test_remove_unused_requires_dist_mixed_markers(): locked_req("baz", "1.0"), locked_req("spam", "1.0"), ) == requires_dist.remove_unused_requires_dist( - locked_resolve( + resolve_requirements=[req("foo[extra1]")], + locked_resolve=locked_resolve( locked_req( "foo", "1.0", @@ -124,7 +127,8 @@ def test_remove_unused_requires_dist_mixed_markers(): locked_req("bar", "1.0"), locked_req("spam", "1.0"), ) == requires_dist.remove_unused_requires_dist( - locked_resolve( + resolve_requirements=[req("foo[extra1]")], + locked_resolve=locked_resolve( locked_req( "foo", "1.0", @@ -151,7 +155,8 @@ def test_remove_unused_requires_dist_complex_markers(): locked_req("bar", "1.0"), locked_req("spam", "1.0"), ) == requires_dist.remove_unused_requires_dist( - locked_resolve( + resolve_requirements=[req("foo")], + locked_resolve=locked_resolve( locked_req( "foo", "1.0", @@ -173,7 +178,8 @@ def test_remove_unused_requires_dist_not_present_due_to_other_markers(): locked_req("bar", "1.0"), locked_req("spam", "1.0"), ) == requires_dist.remove_unused_requires_dist( - locked_resolve( + resolve_requirements=[req("foo")], + locked_resolve=locked_resolve( locked_req( "foo", "1.0",