Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

markers: Consistently fallback to Python comparison rules #816

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
~~~~~~~~~~~~~~~~~
Expand Down
12 changes: 11 additions & 1 deletion src/packaging/markers.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from ._tokenizer import ParserSyntaxError
from .specifiers import InvalidSpecifier, Specifier
from .utils import canonicalize_name
from .version import InvalidVersion

__all__ = [
"InvalidMarker",
Expand Down Expand Up @@ -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 side is not a valid
# version.
pass

oper: Operator | None = _operators.get(op.serialize())
if oper is None:
Expand Down
38 changes: 32 additions & 6 deletions tests/test_markers.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
default_environment,
format_full_version,
)
from packaging.version import InvalidVersion

VARIABLES = [
"extra",
Expand Down Expand Up @@ -388,12 +387,39 @@ 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)
Comment on lines 389 to +392
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, this fallbacks to Python comparison semantics so it technically works now.

cc @sbidoul for visibility that you're okay with this change.


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),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very much a personal opinion, so feel free to ignore it, but it might be nice to have at least one test case that shows string comparison giving the opposite result from how a human would do a version comparison. Like the one you put in the PR description:

("platform_release >= '20'", {"platform_release": "6.7.0-gentoo"}, True)

I'm imagining a different algorithm that extracts the longest prefix of each string which constitutes a valid PEP 440 version number and then compares based on that, which I think would pass all the existing test cases, but of course that is not the algorithm PEP 508 specifies so IMO it'd be nice to have a test case that would catch that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree -- this would be good to add here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did do that in 2ce09d9 (#816). I forgot to mention that, sorry! 😅

# 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"},
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.
"""
assert Marker(marker_string).evaluate(environment) == expected
Loading