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

Conversation

ichard26
Copy link
Member

@ichard26 ichard26 commented Jul 6, 2024

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 <marker_op> operators that are not in <version_cmp> perform the same as they do for strings in Python. The <version_cmp> 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.

>>> from packaging.markers import Marker
>>> m = Marker('platform_release >= "20.0"')
>>> m.evaluate({'platform_release': '6.7.0-gentoo'})
True

Fixes #774. Ref pypa/pip#12825.

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 <marker_op> operators that are not in <version_cmp> perform the
> same as they do for strings in Python. The <version_cmp> 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.
Comment on lines 389 to +392
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)
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.

Copy link

@diazona diazona left a comment

Choose a reason for hiding this comment

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

Hopefully it's not out of line since I'm not a project maintainer, but (since I did report the original issue) I left some comments. Feel free to use them or ignore them as you like!

src/packaging/markers.py Outdated Show resolved Hide resolved
tests/test_markers.py Outdated Show resolved Hide resolved
[
("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! 😅

Co-authored-by: David Zaslavsky <[email protected]>
@ichard26
Copy link
Member Author

ichard26 commented Jul 6, 2024

I saw your comment on the linked issue re. this introducing potential footguns or surprising behaviour -- #774 (comment). I do think it's still better to get packaging in compliance with the specification so at least environment markers can be used in some way. If someone wants to pursue an amendment to the spec. on Python Discourse, they are welcome to do that. I don't have the appetite to do that myself.

I suppose making this situation work would make the migration harder so that would be one argument to reject this PR (although behaviour changes already happened after the removal of legacy versions and specifiers). I'll leave it to folks who are better versed in the specifications and migration than me to weigh in.

@ichard26
Copy link
Member Author

ichard26 commented Jul 6, 2024

Thanks for the review by the way!

@diazona
Copy link

diazona commented Jul 6, 2024

You're welcome! And yes I absolutely agree that the right thing to do here is to get packaging in compliance with the specification.

I suppose I might look into proposing a change to the specification in the future, but it'd be a long and complex process and I'm not sure I have the appetite for it either. (Plus I don't have the credibility in the Python community to get a proposal like this taken seriously.)

@pfmoore
Copy link
Member

pfmoore commented Jul 8, 2024

Plus I don't have the credibility in the Python community to get a proposal like this taken seriously

For what it’s worth your suggested alternative behaviour seems plausible to me and I’m sure it would be taken seriously as a proposal. But it would trigger a debate on what the “right” thing to do is, and I can understand and sympathise if you don’t have the appetite for that amount of work on this.

But let’s get the behaviour spec compliant in the first instance.

@ichard26
Copy link
Member Author

ichard26 commented Jul 9, 2024

Closing for the time being pending a resolution to #774 (comment).

@ichard26 ichard26 closed this Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Marker referencing 'platform_release' fails to evaluate on Linux systems with non-PEP 440 kernel versions
4 participants