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

Fix issue where python_version not correctly handled in converted package #97

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

BryceGattis
Copy link

Not exactly sure how to fix this yet. But I've added a test that fails as expected with my use case.

Fixes #96.

@BryceGattis
Copy link
Author

BryceGattis commented Apr 30, 2024

I'm getting pretty stumped with a solve for the issue. Effectively I'm trying to implement something that can more accurately represent the "environment markers" present in python package requirements.
I came up with an example that I think would be impossible to replicate in Rez currently:

[
     'importlib-metadata ; python_version < "3.8"',
     'click ; python_version > "3.6"'
]

We can't represent these 2 package requirements as variants like this:

variants = [
    ["importlib-metadata", "python<3.8"],
    ["click", "python>3.6"]
]

because Rez will only select one of these variants at resolve time, when we may need both (ex. we request Python 3.7)
It almost seems like we need to replicate the package markers functionality in Rez packages. Unless there's some other Rez functionality I don't know about yet.
That being said, maybe we could do something where if there 2 or more non mutually exclusive requirements like there is above, we just do the old rez-pip1 solve where we lock each variant down to a specific <major>.<minor> python version? Ex.

variants = [
    ["importlib-metadata", "click", "python-3.7+<3.8"],
    ["click", "python-3.8+<3.9"]
]

Any new <major>.<minor> versions would not be supported here though and you'd have to do a new rez-pip for each new Python version as you need support for it.

@JeanChristopheMorinPerso
Copy link
Owner

I see, I don't know how to cleanly and easily solve this. The only way I can think of right now would be by "exploding" the matrix:

variants = [
    ["importlib-metadata", "python<3.8"],
    ["click", "python>3.6"],
    ["click", "importlib-metadata", "python>=3.6|<3.8"]
]

Alternatively, we could create a late bound variants functions... Both options would be hard to implement and both options wouldn't be reliable I think.

The last option is to implement markers in rez to have a more declarative option than @late. AcademySoftwareFoundation/rez#1503 would be of interest I think. One of my arguments in that discussion was that by making the new variants generic, we could introduce new features. For example, we were thinking about:

variants = [
     Variant("maya", requires=["foo", "spam", "maya"], private_build_requires=["maya_devkit"]),
     Variant("houdini", requires=["foo", "houdini"],
     ...
]

If we take this and go even further, we could have

requires = [
    Dep("python"),
    Dep("click", when="python>=3.6"),
    Dep("importlib-metadata", when="python<3.8"),
]

This would be a major change and it would require major changes to the rez codebase though...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rez-pip can't handle python_version requirements in packages
2 participants