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

Is handling of python_version correct? #5717

Closed
dimbleby opened this issue May 29, 2022 · 8 comments
Closed

Is handling of python_version correct? #5717

dimbleby opened this issue May 29, 2022 · 8 comments
Labels
kind/bug Something isn't working as expected

Comments

@dimbleby
Copy link
Contributor

Currently poetry mostly treats python_version x.y as equivalent to python_full_version x.y.0, but is it necessarily so?

#!/usr/bin/env python3

from poetry.core.version.markers import parse_marker

m1 = parse_marker('python_version > "3.6"')
m2 = parse_marker('python_full_version == "3.6.2"')
allowed = m1.constraint.allows(m2.constraint)
print(allowed)

This prints True. But surely PEP508 says that the python_version at 3.6.2 is exactly 3.6, which does not satisfy m1.

related #2480.

@dimbleby dimbleby added kind/bug Something isn't working as expected status/triage This issue needs to be triaged labels May 29, 2022
@radoering
Copy link
Member

Seems to be wrong. Probably, it is only correct for >= and <, but fails for > and <=. In get_python_constraint_from_marker(), there is even a special handling for these cases: https://github.com/python-poetry/poetry-core/blob/dfd1b39ffec8bfad027a6dda0c31bab24b4c6019/src/poetry/core/packages/utils/utils.py#L310-L325 I assume that is required in other places, too.

@dimbleby
Copy link
Contributor Author

For a start, we should roll back the marker simplifications that treat these two the same.

That might be enough, if we never compare python_version and python_full_version directly perhaps we're ok

@dimbleby
Copy link
Contributor Author

https://github.com/python-poetry/poetry-core/blob/63322f630b40a261c26644d4b82324a44b6aafe0/src/poetry/core/packages/utils/utils.py#L162-L165 looks highly suspicious

        # python_full_version is equivalent to python_version
        # for Poetry so we merge them
        if marker_name == "python_full_version":
            marker_name = "python_version"

@radoering
Copy link
Member

Suspicious yes, but might not wrong per se. I think it depends on how its callers process the result of convert_markers(). At least, get_python_constraint_from_marker() seems to take care of it.

By the way, do you have plans to work on a correct merging of python_version and python_full_version in marker unions and intersections within the next few days? (If not, I would take a look into it.)

@dimbleby
Copy link
Contributor Author

You've probably seen that I've reverted the naive merging. No, I don't have plans to implement a smarter version.

@dimbleby
Copy link
Contributor Author

dimbleby commented May 30, 2022

The marker setter on a Dependency only cares about python_version: so if I set the marker python_full_version >= 3.6 (for example), the dependency's python constraint is *.

Edit: that's not true, but it looks as though it really ought to share code with get_python_constraint_from_marker()

@dimbleby
Copy link
Contributor Author

dimbleby commented Jun 5, 2022

Gonna call this done, it has triggered some good fixes, if there are more bugs then they can be tracked as they are found

Copy link

github-actions bot commented Mar 1, 2024

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Something isn't working as expected
Projects
None yet
Development

No branches or pull requests

3 participants