Use current and requested Python versions in requires-python incompatibility errors
#986
Conversation
| Because there are no versions of Python that satisfy Python>=3.11,<3.12 and there are no versions of Python that satisfy Python>=3.12, we can conclude that Python>=3.11 are incompatible. | ||
| Because Python 3.9 does not satisfy Python>=3.11,<3.12 and Python 3.9 does not satisfy Python>=3.12, we can conclude that Python>=3.11 are incompatible. |
There was a problem hiding this comment.
Well this message didn't get worse but what the heck is it saying? TODO create an issue tracking this bug
| // We assume there is _only_ one available Python version as Puffin only supports | ||
| // resolution for a single Python version; if this is not the case for some reason | ||
| // we'll just fall back to the usual verbose message in production but panic in | ||
| // debug builds |
There was a problem hiding this comment.
I think this actually isn't true because if you use --python-version, we'll have both the installed and the target version in the tree, and either one could be incompatible (or both). So we might want to make it explicit that this only works if --python-version isn't provided (and remove the debug assert), or find a way to accommodate that.
There was a problem hiding this comment.
Ah gee... we should accommodate that somehow thanks for catching it.
# Conflicts: # crates/puffin/tests/pip_compile.rs # crates/puffin/tests/pip_install_scenarios.rs
zanieb
force-pushed
the
zb/python
branch
3 times, most recently
from
e0c5f63
to
462f9a8
Compare
# Conflicts: # crates/puffin/tests/pip_compile.rs # crates/puffin/tests/pip_install_scenarios.rs
zanieb
changed the title
requires-python incompatibility errorsrequires-python incompatibility errors
| { | ||
| // Simple case, the installed version is the same as the target version | ||
| // N.B. Usually the target version does not include anything past the | ||
| // minor version mumber so we only compare to part of the installed |
There was a problem hiding this comment.
Are they not exactly the same when not provided? I think I would've expected --python-version 3.10 to be treated as --python-version 3.10.0. I'm just wondering if this will be correct if the user uses a different patch version in the resolution.
There was a problem hiding this comment.
If a target Python version is provided e.g. --python-version 3.11
DEBUG puffin_resolver::resolver Solving with target Python 3.11.6 and installed Python 3.12.0
If the target Python version is inferred from the current version
DEBUG puffin_resolver::resolver Solving with target Python 3.12 and installed Python 3.12.0
If a target Python version is provided with patch version e.g. --python-version 3.11.3
DEBUG puffin_resolver::resolver Solving with target Python 3.11.3 and installed Python 3.12.0
If a target Python version is provided but not installed e.g. --python-version 3.13
DEBUG puffin_resolver::resolver Solving with target Python 3.13 and installed Python 3.12.0
There was a problem hiding this comment.
We compare the full target to the first two numbers of the installed so we should only have equality here when --python-version is not provided.
Let me know if I can make the this clearer in my comment.
There was a problem hiding this comment.
These are helpful...
(So first, I am assuming that 3.12 and 3.12.0 are considered equal when you use == with Version. I'm pretty sure this is true and per spec. In that case, we shouldn't need to do the .take(2) thing.)
If a target Python version is provided e.g.
--python-version 3.11
I initially thought this one was a bug, because 3.11 should've resolved to 3.11.0. But then I realized we do a thing whereby we assume the highest known minor. That behavior kind of feels wrong in hindsight... I think it exists because --python-version 3.7 would otherwise use 3.7.0, and a lot of things aren't compatible with the first few patch releases in 3.7, and that was a confusing user experience in my testing. But it might be wrong...
If the target Python version is inferred from the current version
So these should match based on Version equality, I think.
If a target Python version is provided with patch version e.g.
--python-version 3.11.3
These should be considered unequal based on Version equality.
If a target Python version is provided but not installed e.g.
--python-version 3.13
This seems right to me, they'd be considered unequal based on version equality. But it shouldn't have to do with whether the target is installed.
There was a problem hiding this comment.
Which case specifically would be wrong if we used == instead of take(2)?
There was a problem hiding this comment.
Why would we see 3.12.1 != 3.12 though? Why is one of the versions not inclusive of the patch, when you don't provide a --python-version? Isn't that wrong?
There was a problem hiding this comment.
I have no idea why it's implemented that way, but yes as noted above if the target version is inferred from the current Python version it will not include the patch version. I'm all for changing that behavior if it was not intentional, but it doesn't seem like we should do it here.
Here's another commit showing the installed version alongside the target one: 43910a8e
There was a problem hiding this comment.
Personally, I think the right order of operations is to fix the missing patch version (in a separate PR) and then rebase this on top of that. Otherwise, we're adding a workaround for a bug that we need to fix immediately. But feel free to merge, I see the issue and can fix now.
There was a problem hiding this comment.
Tossed something up here 🤞 #1033
In #986 there was some confusion about what these values are set to and I noticed that we never actually display the target version being used for a resolution. - Consistently display the Python interpreter being used, i.e. make it clear that we are referring the the interpreter/installed Python version and always show the version number - Display the target Python version during solving
Closes #806