Fix Marker non-version-to-version comparison throwing InvalidVersion #883
Conversation
bc9acc6 to
a0651a2
Compare
|
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 |
|
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? |
|
Does this tie into #774 ? |
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).
As discussed in pypa#774, falling back to lexicographic string comparison when using greater/less-than operators can result in counter-intuitive behaviour on version-like numeric strings.
a0651a2 to
aff2962
Compare
|
@brettcannon Yes, it's the same root cause, I wasn't aware of that. This MR also resolves that issue, but also suffers from counter-intuitive version ordering issue it describes. I've rebased this MR to fix the conflict (following PEP 751 changes in #888) and added another test to demonstrate the lexicographic version order quirk. |
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:
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:
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).