Skip to content

Fix Marker non-version-to-version comparison throwing InvalidVersion #883

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

h4l
Copy link

@h4l h4l commented Mar 27, 2025

This PR adds (failing) test cases to cover version-to-version and non-version-to-version marker evaluation, according to PEP 508 — Environment Markers. It also changes the Marker evaluation implementation to follow the spec's behaviour rather than crashing when evaluating Marker equality with a valid version on the RHS only.


Current situation

Marker evaluation currently throws an InvalidVersion error when the rhs is a valid version but the lhs is not. For example, consider the dependency:

Requires-Dist: typing-extensions (>=4,<5) ; extra == "v8"

The extra name "v8" is both a valid version and extra name. During evaluation of this requirement, the LHS of the marker can take on the value "", or some other extra name, which are not valid versions.

When this situation occurs, the comparison fails by throwing InvalidVersion, rather then returning False. This is causing pip to crash when attempting to install a package with such a requirement.

Currently, just one of these added cases is failing - the "quux" == "v8" case.

i.e. this is happening:

>>> from packaging.markers import Marker
>>> Marker('extra == "v8"').evaluate({'extra': ''})
...
InvalidVersion: Invalid version: ''
>>> Marker('extra == "v8"').evaluate({'extra': 'foo'})
...
InvalidVersion: Invalid version: 'foo'

I've also added two cases to document the (perhaps surprising) behaviour that is a result of the PEP 508 spec requiring normalized version comparison to apply to extras when both sides are valid versions.

Fix

PEP 508 — Environment Markers section specifies that markers are evaluated using version operator rules when both sides are valid versions, and otherwise regular python operator rules apply.

However, the implementation was failing to handle the case where the RHS was a valid version specifier, but the LHS was not a valid version. In this case, PEP 508 requires the comparison falls back to python rules, but the implementation was throwing InvalidVersion.

The implementation now matches the behaviour in the spec, as we now check that both the LHS and the RHS are versions before attempting version comparison (not just the RHS as before).

h4l added 2 commits March 27, 2025 05:14
This commit adds test cases to cover version-to-version and
non-version-to-version marker evaluation, according to PEP 508,
Environment Markers. https://peps.python.org/pep-0508/#environment-markers

Marker evaluation currently throws an InvalidVersion error when the rhs
is a valid version but the lhs is not. For example, consider the
dependency:

> Requires-Dist: typing-extensions (>=4,<5) ; extra == "v8"

The extra name "v8" is both a valid version and extra name. During
evaluation of this requirement, the LHS of the marker can take on the
value "", or some other extra name, which are not valid versions.

When this situation occurs, the comparison fails by throwing
InvalidVersion, rather then returning False. This is causing pip to
crash when attempting to install a package with such a requirement.

Currently, just one of these added cases is failing - the
"quux" == "v8" case.

I've also added two cases to document the (perhaps surprising) behaviour
that is a result of the PEP 508 spec requiring normalized version
comparison to apply to extras when both sides are valid versions.
As described in my previous commit to add test cases covering this
behaviour, PEP 508 - Environment Markers section specifies that markers
are evaluated using version operator rules when both sides are valid
versions, and otherwise regular python operator rules apply.

However, the implementation was failing to handle the case where the RHS
was a valid version specifier, but the LHS was not a valid version. In
this case, PEP 508 requires the comparison falls back to python rules,
but the implementation was throwing InvalidVersion.

The implementation now matches the behaviour in the spec, as we now
check that both the LHS and the RHS are versions before attempting
version comparison (not just the RHS as before).
@h4l h4l force-pushed the fix-marker-eval-crash branch from bc9acc6 to a0651a2 Compare March 27, 2025 05:15
@h4l
Copy link
Author

h4l commented Mar 27, 2025

h4l force-pushed the fix-marker-eval-crash branch from bc9acc6 to a0651a2
now

The comment on my last test case was incorrect, I switched it to a better example to contrast the preceding test case.

@h4l
Copy link
Author

h4l commented Mar 27, 2025

I should also add that this change should not be changing the interpretation of any existing markers. It should only be allowing markers (with a version-like RHS and non-version-like LHS) that previously failed with an error to evaluate with string comparison semantics. With == this will always evaluate False, so it also shouldn't be allowing any new package requirements to match, just (in the case of a pip install) making the overall process not crash.

@h4l
Copy link
Author

h4l commented Apr 25, 2025

Is there something I can do to help with this? Maybe provide more info? Or report an issue with pip that's affected by this bug?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant