From 0a1343606703d2418055167498681a749a017769 Mon Sep 17 00:00:00 2001 From: Richard Si Date: Sat, 6 Jul 2024 13:26:20 -0400 Subject: [PATCH 1/2] markers: Consistently fallback to Python comparison rules In the dependency specifiers specification (originally PEP 508), when there is no PEP 440 defined behaviour for evaluating a marker because one or both sides are not valid versions [specifiers], a fallback to Python comparison behaviour is mandated: > The operators that are not in perform the > same as they do for strings in Python. The operators use > the version comparison rules of the Version specifier specification > when those are defined (that is when both sides have a valid version > specifier). If there is no defined behaviour of this specification and > the operator exists in Python, then the operator falls back to the > Python behaviour. Otherwise an error should be raised. However, this fallback has been broken for a while. When the right side has PEP 440 defined semantics (being a valid specifier), but the left side is an invalid version, an InvalidVersion error is raised. This patch suppresses said InvalidVersion error and fallbacks as expected. --- CHANGELOG.rst | 5 ++++- src/packaging/markers.py | 12 +++++++++++- tests/test_markers.py | 37 +++++++++++++++++++++++++++++++------ 3 files changed, 46 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 620ee1b8..90844dce 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -4,7 +4,10 @@ Changelog *unreleased* ~~~~~~~~~~~~ -No unreleased changes. +* Consistently fallback to Python string comparison behaviour when evaluating + a marker where there is no PEP 440 defined behaviour, notably when the right + side has PEP 440 defined semantics, but the left side is an invalid + version (:issue:`774`) 24.1 - 2024-06-10 ~~~~~~~~~~~~~~~~~ diff --git a/src/packaging/markers.py b/src/packaging/markers.py index 7ac7bb69..08baa692 100644 --- a/src/packaging/markers.py +++ b/src/packaging/markers.py @@ -15,6 +15,7 @@ from ._tokenizer import ParserSyntaxError from .specifiers import InvalidSpecifier, Specifier from .utils import canonicalize_name +from .version import InvalidVersion __all__ = [ "InvalidMarker", @@ -178,9 +179,18 @@ def _eval_op(lhs: str, op: Op, rhs: str) -> bool: try: spec = Specifier("".join([op.serialize(), rhs])) except InvalidSpecifier: + # As per the specification, if there is no PEP 440 defined + # behaviour because the right side is not a valid version + # specifier, fallback to Python string comparision behaviour. pass else: - return spec.contains(lhs, prereleases=True) + try: + return spec.contains(lhs, prereleases=True) + except InvalidVersion: + # Even though there is PEP 440 defined behaviour for the + # right side, fallback as the left left is not a valid + # version. + pass oper: Operator | None = _operators.get(op.serialize()) if oper is None: diff --git a/tests/test_markers.py b/tests/test_markers.py index 775b51fd..92b2d055 100644 --- a/tests/test_markers.py +++ b/tests/test_markers.py @@ -20,7 +20,6 @@ default_environment, format_full_version, ) -from packaging.version import InvalidVersion VARIABLES = [ "extra", @@ -388,12 +387,38 @@ def test_extra_str_normalization(self): assert str(Marker(rhs)) == f'extra == "{normalized_name}"' def test_python_full_version_untagged_user_provided(self): - """A user-provided python_full_version ending with a + fails to parse.""" - with pytest.raises(InvalidVersion): - Marker("python_full_version < '3.12'").evaluate( - {"python_full_version": "3.11.1+"} - ) + """A user-provided python_full_version ending with a + uses Python behaviour.""" + env = {"python_full_version": "3.11.1+"} + assert Marker("python_full_version < '3.12'").evaluate(env) def test_python_full_version_untagged(self): with mock.patch("platform.python_version", return_value="3.11.1+"): assert Marker("python_full_version < '3.12'").evaluate() + + @pytest.mark.parametrize( + ("marker_string", "environment", "expected"), + [ + ("platform_release >= '20.0'", {"platform_release": "21-foobar"}, True), + ("platform_release >= '8'", {"platform_release": "6.7.0-gentoo"}, False), + ("platform_version == '27'", {"platform_version": "weird string"}, False), + ( + "implementation_version == '3.*'", + {"implementation_version": "2_private"}, + False, + ), + ( + "implementation_version == '3.*'", + {"implementation_version": "3.*"}, + True, + ), + ], + ) + def test_valid_specifier_invalid_version_fallback_to_python( + self, marker_string: str, environment: dict, expected: bool + ): + """If the right operand is a valid version specifier, but the + left operand is not a valid version, fallback to Python string + comparison behaviour. + """ + args = [] if environment is None else [environment] + assert Marker(marker_string).evaluate(*args) == expected From 2ce09d963b6b9b954dfe1a09639e7878acb9925f Mon Sep 17 00:00:00 2001 From: Richard Si Date: Sat, 6 Jul 2024 17:06:52 -0400 Subject: [PATCH 2/2] Fix a typo and improve tests Co-authored-by: David Zaslavsky --- src/packaging/markers.py | 2 +- tests/test_markers.py | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/packaging/markers.py b/src/packaging/markers.py index 08baa692..35b7fd24 100644 --- a/src/packaging/markers.py +++ b/src/packaging/markers.py @@ -188,7 +188,7 @@ def _eval_op(lhs: str, op: Op, rhs: str) -> bool: return spec.contains(lhs, prereleases=True) except InvalidVersion: # Even though there is PEP 440 defined behaviour for the - # right side, fallback as the left left is not a valid + # right side, fallback as the left side is not a valid # version. pass diff --git a/tests/test_markers.py b/tests/test_markers.py index 92b2d055..cf4a5c5e 100644 --- a/tests/test_markers.py +++ b/tests/test_markers.py @@ -401,6 +401,8 @@ def test_python_full_version_untagged(self): ("platform_release >= '20.0'", {"platform_release": "21-foobar"}, True), ("platform_release >= '8'", {"platform_release": "6.7.0-gentoo"}, False), ("platform_version == '27'", {"platform_version": "weird string"}, False), + # This looks weird, but is expected as per Python's lexicographical order. + ("platform_version >= '10'", {"platform_version": "6.7.0-gentoo"}, True), ( "implementation_version == '3.*'", {"implementation_version": "2_private"}, @@ -420,5 +422,4 @@ def test_valid_specifier_invalid_version_fallback_to_python( left operand is not a valid version, fallback to Python string comparison behaviour. """ - args = [] if environment is None else [environment] - assert Marker(marker_string).evaluate(*args) == expected + assert Marker(marker_string).evaluate(environment) == expected