Marker referencing 'platform_release' fails to evaluate on Linux systems with non-PEP 440 kernel versions #774
Comments
|
The relevant part of PEP 508 seems to be this: Comparisons in marker expressions are typed by the comparison operator. 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 PEP 440 version comparison rules when those are defined (that is when both sides have a valid version specifier). If there is no defined PEP 440 behaviour and the operator exists in Python, then the operator falls back to the Python behaviour. so in this case it should fall back to a string comparison? |
|
Huh, it should indeed. A PR fixing this would be welcome! |
|
I took a stab in PR #816. I hope that there aren't any devious edge cases in the details, although honestly I would not be surprised 🙃 |
|
Thanks! I think the wording of the PEP could lead to what might be considered some odd edge cases, like the example you put where |
|
The odd edge cases are also mentioned here |
|
In packaging<22 (pip<24.1), it seems these cases were handled with something more intuitive than a string comparison, that was not standard compliant. Today, we are in a situation that fails explicitly in such cases which, in a way, is good, or at least better than changing behavior silently. So if I read correctly, no packaging version has ever been standard compliant for these cases. Since the standard will lead to a non-intuitive solution, and (pip) users did rely on the intuitive solution of packaging<22, maybe there is a case to be made to go back to the standard drawing board first, instead of merging #816. Even if that takes quite some time, affected users can stick to packaging<22 or pip<24.1. If we implement the standard today, it will be a breaking changes for folks who relied on the previous behavior, it will likely please no-one, and it will also lead to a situation that will be much harder to change if/when we want to change the standard because it would then be another breaking change. |
|
I agree with @sbidoul that the spec should be updated directly. Updating pip/packaging to be in line with the existing spec first is a step in the wrong direction, because the string comparison fallback does not seem sensible or useful. Is that |
|
Hmm okay... if the spec is to be updated, would the goal be to formalize the behavior that pip/packaging has used in the past? Or would this be an opportunity to rethink the whole thing and come up with a potentially fresh and more sensible way of handling non-PEP 440 version numbers than lexicographic comparison? In the latter case, I'd be willing to help advance a discussion about what the spec should say, if I knew some people with more established reputations than myself - like you all - would be interested in having it. (as per this discussion on the PR) Or, even in the former case (formalize the historical behavior) I'd be happy to help too, but I think I have little value to add in that situation. Some ideas I've been throwing around in my head
This is not to derail the issue with discussion of what a change proposal (if there is one) should be; just making the point that there's enough material to start that discussion in whatever venue is appropriate. |
|
FYI the spec for environment markers can be found at https://packaging.python.org/en/latest/specifications/dependency-specifiers/#environment-markers and it mirrors what's in PEP 508, but the spec is authoritative.
That means it hasn't been that way since 2021, so a good amount of users have moved on.
If someone wants to bring this up on discuss.python.org and try to get consensus then please feel free to! We don't have to rush to fix this, but we shouldn't leave it in a non-compliant state forever, either.
It will at least please me as this issue can be closed. 😁
The problem w/ that is it assumes that even that is lax enough to parse every Linux distro version. The string fallback has the feature of being simple and it never fails for anyone. But if we try to be clever it could still lead to complaints about why doesn't my Linux distro's version get parsed? And as the project that's going to have to field those complaints, it doesn't make me want to try and change the spec to add a potential 3rd layer to the marker comparison logic. |
I disagree on this point, pip was vendoring packaging < 22 until pretty recently, and the majority of users would be using packaging via
Personally, I noticed because |
|
Yeah, only pip users who have moved to pip 24.1+ have been subject to these rules. And IMO pip users most likely to struggle with these rules are the ones using OS distributed versions of pip, on LTS versions of their OS, and may not upgrade to pip 24.1+ for several years still, e.g. I work with a team currently standardized on pip 19.1, so there may be a very long tail of users hitting this problem. |
Fair enough, my mistake as I thought pip was keeping up more. Regardless, this project is not in the business of going against standards, so someone will have to get the spec changed for us to implement something different than what's on packaging.python.org. |
FYI updating to packaging 22.0+ was a difficult vendoring for pip pypa/pip#11715 / pypa/pip#12300 |
`platform_release` parsing is more complicated than other fields. It can be either a PEP-440 version, or a non-compliant version string. If both lhs and rhs were correctly parsed as PEP-440 versions run regular version comparison, otherwise compare lexicographically See: - pypa/packaging#774 - astral-sh/uv#3917 (comment)
`platform_release` parsing is more complicated than other fields. It can be either a PEP-440 version, or a non-compliant version string. If both lhs and rhs were correctly parsed as PEP-440 versions run regular version comparison, otherwise compare lexicographically See: - pypa/packaging#774 - astral-sh/uv#3917 (comment)
`platform_release` parsing is more complicated than other fields. It can be either a PEP-440 version, or a non-compliant version string. If both lhs and rhs were correctly parsed as PEP-440 versions run regular version comparison, otherwise compare lexicographically See: - pypa/packaging#774 - astral-sh/uv#3917 (comment)
`platform_release` parsing is more complicated than other fields. It can be either a PEP-440 version, or a non-compliant version string. If both lhs and rhs were correctly parsed as PEP-440 versions run regular version comparison, otherwise compare lexicographically See: - pypa/packaging#774 - astral-sh/uv#3917 (comment)
`platform_release` parsing is more complicated than other fields. It can be either a PEP-440 version, or a non-compliant version string. If both lhs and rhs were correctly parsed as PEP-440 versions run regular version comparison, otherwise compare lexicographically See: - pypa/packaging#774 - astral-sh/uv#3917 (comment)
Ideally this would use 'platform_version >= 6.2' (Windows 7 is version 6.1) but that, and 'platform_release != 7', cause InvalidVersion errors with pip >= 24.1 (see pypa/packaging#774).
|
Going on a slight tangent, this issue seems to be exacerbated by the markers being evaluated eagerly. A number of packages have markers ordered like Such a change could be a good intermediate solution as updating the standard could take a while. |
|
This has broken my project(s) for months, can someone fix this? Looks like there is already a pull request mitigation above. |
Does it look like #834 would help? |
|
Yes, because the error happens on Linux and the test is only valid on Windows: Seems the boolean AND should exit at the first False. |
|
I'm eager for the mitigation to get merged as well, but can't see any contributor info on etiquette to getting a maintainer to look at it. Pinging @brettcannon since you've reviewed the last bunch of stuff that's been merged. |
|
Some arguments against that change in #834:
It feels like a bandaid solution, I can't support it. |
|
I would like to offer some counterpoints:
A PR to follow the spec #816 was rejected as not being user friendly. So if #834 is rejected for not being explicitly condoned by the spec, even though it's more user friendly, that leaves no way forward unless someone experienced enough with packaging specifications and interacting with the packaging community and has an interest in fixing this to update the spec and packaging. And so far no packaging maintainers has been interested in doing that, it would be extremely onerous on a packaging user to do this.
That seems natural.
IMO "behave" is doing a lot heavy lifting, it's not like they are returning a different value, one is throwing an error and the other is returning a value. Does the spec say anything about errors or exceptions?
The PR to fix the bug was rejected as being a bad user experience. This improves the user experience.
Short circuiting is very common for anything that implements Boolean logic. I don't imagine it difficult to explain it to users. |
|
Right, it's not arcane. Rather the opposite, with a half century of prior art. Am surprised it never worked that way, but better late than never. It is true that this pull request is only one part of a complete fix. There are at least two bugs here, and the pull request fixes one. On the plus side, the two issues are not directly connected and one fix need not hold back the other. |
Well, isn't it? The docstring does say "there is no such thing as an invalid version number under this scheme", does anyone have a counter-example to offer? |
It does, yeah. Ctrl+f for "an error" in PEP 508 finds them. The relevant considerations for this issue are the operators which aren't supported in Python, i.e.
I'm not sure I agree on this. It complicates things.
It does seem that way, yes. |
That's a net improvement to the user experience, and something that can be recommended on how to write metadata.
That’s an implementation issue, not a shortcoming of short-circuiting, a pass for illegal syntax could be done before evaluating the operations. Why not leave a PR review?
That's a syntax error, not a runtime comparison error, you've constructed a scenario related to illegal syntax that no one has asked to work, the ask is for short-circuiting value evaluation which would look far more like this in Python: def error():
raise RuntimeError
result = False and error()
if not result:
print("No error")In this example even though
Another suggestion for users who want their tooling to work, and don't need this metadata to work for other users, they could try uv, which doesn't rely on packaging. |
|
I've amended my PR to make it work irrespective of the ordering of marker clauses. Ultimately the call that needs to be made is, if a package
Should packaging/pip fail on it? If "no" then some kind of mitigating fix should be made given that other "proper" change attempts have stalled out. |
|
That looks like an improvement and resolves some of my concerns (I'll strike them out above). I still have some reservations, but it's a more convincing PR now. Maybe you can also add these test cases: Also, as a minor point I think that the try/except used within the test body isn't ideal - suppose the "expected" result is a boolean, but the marker evaluation raises. In that case we'll go into the except branch and do something like |
|
For what it's worth, it's been on my radar to try to start a discussion about changing the spec when I have time to properly follow that through. As I'm sure everyone understands, it's not a simple problem and I wouldn't want to give it a half-hearted attempt at a solution. The recent discussion here is quite helpful. This is not to say that anyone else couldn't propose a change to the spec if they want to - I'm not claiming dibs on that approach. Just reassuring people that it hasn't been forgotten. |
This comment was marked as off-topic.
This comment was marked as off-topic.
In a conversation on Mastodon, @kevinbowen777 reported an error with a failing version comparison when installing py5 with pdm. With @py5coding and myself, we tracked the cause to markers like
platform_release >= "20.0" and sys_platform == "darwin"that appear in the pdm lock file. Specifically, on Linux,platform_releaseevaluates to the full Linux kernel version as returned byuname -r, which might be something like6.1.0-17-amd64(on Kevin's system) or6.7.0-gentoo(on mine), and so when packaging applies PEP 440 version comparison logic to that version number, it raises anInvalidVersionerror.Here's a simple reproduction on my system:
I know it's not terribly common for packages to use the
platform_releasemarker, but evidently it does happen; in this case when installing py5, something is conditionally depending on pyobjc and is usingplatform_releaseto express that condition. And I didn't find anything in the documentation indicating that this sort of thing shouldn't be allowed. So it certainly at least looks like an issue that needs to be worked around.This is pretty similar to #678, but that issue was about the Python runtime version, where there's a bit of an argument to be made that CPython should stick to PEP 440-compatible version numbers. I'm pretty sure that argument isn't going to fly for distribution-packaged Linux kernel versions. I wasn't sure if you'd rather have a separate issue or just a comment on #678, but I figured I'd start with this and you can absorb it into that other issue if you want.
Original example with pdm and py5
It's worth noting that we haven't figured out how to reproduce this with any other package installer besides pdm. We haven't figured out why exactly that is; evidently there's something that pdm is doing and others don't which is involved in triggering the error. But, considering the simple example I posted above, this clearly doesn't need pdm to be reproducible, and it didn't seem like pdm is using
Marker.evaluate()incorrectly, which is why I'm reporting it here rather than to pdm.The text was updated successfully, but these errors were encountered: