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

Support Python versions with trailing + #1357

Closed
charliermarsh opened this issue Feb 15, 2024 · 8 comments · Fixed by #1771
Closed

Support Python versions with trailing + #1357

charliermarsh opened this issue Feb 15, 2024 · 8 comments · Fixed by #1771
Assignees
Labels
bug Something isn't working

Comments

@charliermarsh
Copy link
Member

It was reported that a locally-built Python with 3.11.4+ errors out in our version parser:

https://gist.github.com/jsbueno/be1797cfe654c69245ddbea78e0360ff

@charliermarsh charliermarsh added the bug Something isn't working label Feb 15, 2024
@BurntSushi
Copy link
Member

The Local Version Identifiers section of the Version Specifiers spec seems to suggest that a local label needs to be non-empty, but it doesn't seem to come out and say it explicitly. With that said, the regex for parsing version numbers does unambiguously require a non-empty local version segment if a + is present.

So at least with respect to the spec, it seems like the version number here is invalid. Is there any particular reason why 3.11.4+ is being used? If it's just a mistake, then perhaps that should be fixed instead.

But if this is a widespread or common practice, then perhaps there isn't too much harm in making our version parser slightly more flexible. I think the point here though would be to decide what the semantics of 3.11.4+ are. Is the version identical to 3.11.4? And in particular, if someone gave us a 3.11.4+ version and we spit 3.11.4 back out at some point, would that be an issue? The question might seem strange, but it's the difference between a "version without a local segment" and a "version with an empty local segment." (The current representation of a Version doesn't distinguish between those cases.)

@hauntsaninja
Copy link
Contributor

I think the Python version isn't governed by PEP 440.

3.11.4+ is if you build off of random CPython commit. I think it comes from e.g. https://github.com/python/cpython/blob/c08c0679055d96c0397cf128bf7cc8134538b36a/Include/patchlevel.h#L26

Should be fine to treat it as 3.11.4

@zanieb
Copy link
Member

zanieb commented Feb 16, 2024

They seem semantically different; I would treat 3.11.4+ as fulfilling versions that are not pinned. It's rare that we request e.g. ==3.11 or ==3.11.4

@charliermarsh
Copy link
Member Author

What's the issue with treating this as equivalent to parsing 3.11.4?

@edgarrmondragon
Copy link
Contributor

edgarrmondragon commented Feb 16, 2024

I second @hauntsaninja. The + is usually added to Python builds that are built off the main branch rather than a tag:

>>> import pprint
>>> from packaging.markers import default_environment
>>> pprint.pprint(default_environment())
{'implementation_name': 'cpython',
 'implementation_version': '3.13.0a3',
 'os_name': 'posix',
 'platform_machine': 'arm64',
 'platform_python_implementation': 'CPython',
 'platform_release': '23.4.0',
 'platform_system': 'Darwin',
 'platform_version': 'Darwin Kernel Version 23.4.0: Wed Feb  7 23:21:07 PST '
                     '2024; root:xnu-10063.100.637.501.2~2/RELEASE_ARM64_T8112',
 'python_full_version': '3.13.0a3+',
 'python_version': '3.13',
 'sys_platform': 'darwin'}

@charliermarsh
Copy link
Member Author

Makes sense. I'd also be open to just stripping a trailing + for those fields specifically... @BurntSushi, what do you think?

@zanieb
Copy link
Member

zanieb commented Feb 16, 2024

They're different versions — the + has meaning. Similar to 1.0.0rcbeing different from 1.0.0. I don't think treating them as distinct versions would cause any problems for our usages of Python versions though. If we treat them as different you could e.g. require a local version by requesting 3.13.0+, but if you request 3.13 it should work fine even if 3.13.0+ is what we find.

@konstin
Copy link
Member

konstin commented Feb 20, 2024

python_full_version needs to be a PEP 440 compliant version, otherwise environment markers break (PEP 508 environment marker comparisons). Depending on your interpretation of PEP 508, we would fall back to stringly comparisons for python_full_version where '3.9.0' is lower than '3.11.4+'.

There's an upstream cpython bug: python/cpython#99968

BurntSushi added a commit that referenced this issue Feb 20, 2024
(This PR message is mostly copied from the comment in the code.)

For local builds of Python, at time of writing, the version numbers end with
a `+`. This makes the version non-PEP-440 compatible since a `+` indicates
the start of a local segment which must be non-empty. Thus, `uv` chokes on it
and [spits out an error][1] when trying to create a venv using a "local" build
of Python. Arguably, the right fix for this is for [CPython to use a PEP-440
compatible version number][2].

However, as a work-around for now, [as suggested by pradyunsg][3] as one
possible direction forward, we strip the `+`.

This fix does unfortunately mean that one [cannot specify a Python version
constraint that specifically selects a local version][4]. But at the time of
writing, it seems reasonable to block such functionality on this being fixed
upstream (in some way).

Another alternative would be to treat such invalid versions as strings (which
is what PEP-508 suggests), but this leads to undesirable behavior in this
case. For example, let's say you have a Python constraint of `>=3.9.1` and
a local build of Python with a version `3.11.1+`. Using string comparisons
would mean the constraint wouldn't be satisfied:

    >>> "3.9.1" < "3.11.1+"
    False

So in the end, we just strip the trailing `+`, as was done in the days of old
for [legacy version numbers][5].

I tested this fix by manually confirming that

    uv venv --python local/python

failed before it and succeeded after it.

Fixes #1357

[1]: #1357
[2]: python/cpython#99968
[3]: pypa/packaging#678 (comment)
[4]: #1357 (comment)
[5]: https://github.com/pypa/packaging/blob/085ff41692b687ae5b0772a55615b69a5b677be9/packaging/version.py#L168-L193
BurntSushi added a commit that referenced this issue Feb 20, 2024
(This PR message is mostly copied from the comment in the code.)

For local builds of Python, at time of writing, the version numbers end
with
a `+`. This makes the version non-PEP-440 compatible since a `+`
indicates
the start of a local segment which must be non-empty. Thus, `uv` chokes
on it
and [spits out an error][1] when trying to create a venv using a "local"
build
of Python. Arguably, the right fix for this is for [CPython to use a
PEP-440
compatible version number][2].

However, as a work-around for now, [as suggested by pradyunsg][3] as one
possible direction forward, we strip the `+`.

This fix does unfortunately mean that one [cannot specify a Python
version
constraint that specifically selects a local version][4]. But at the
time of
writing, it seems reasonable to block such functionality on this being
fixed
upstream (in some way).

Another alternative would be to treat such invalid versions as strings
(which
is what PEP-508 suggests), but this leads to undesirable behavior in
this
case. For example, let's say you have a Python constraint of `>=3.9.1`
and
a local build of Python with a version `3.11.1+`. Using string
comparisons
would mean the constraint wouldn't be satisfied:

    >>> "3.9.1" < "3.11.1+"
    False

So in the end, we just strip the trailing `+`, as was done in the days
of old
for [legacy version numbers][5].

I tested this fix by manually confirming that

    uv venv --python local/python

failed before it and succeeded after it.

Fixes #1357

[1]: #1357
[2]: python/cpython#99968
[3]:
pypa/packaging#678 (comment)
[4]: #1357 (comment)
[5]:
https://github.com/pypa/packaging/blob/085ff41692b687ae5b0772a55615b69a5b677be9/packaging/version.py#L168-L193
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants