-
-
Notifications
You must be signed in to change notification settings - Fork 153
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
PR: Add QT_VERSION
environment variable
#478
base: master
Are you sure you want to change the base?
PR: Add QT_VERSION
environment variable
#478
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Bzero for your work here! Checked the CI failures and I think those were unrelated to the work being done here (trigger a new run and seems like checks are passing now). Left some initial comments but from a quick review I would say the changes make sense 👍
def test_qt_version(): | ||
""" | ||
If QT_VERSION is specified, check that one of the correct Qt wrappers was used | ||
""" | ||
|
||
QT_VERSION = os.environ.get("QT_VERSION", "").lower() | ||
|
||
if QT_VERSION == "qt5": | ||
assert_qt5() | ||
elif QT_VERSION == "qt6": | ||
assert_qt6() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not totally sure but, I think we are not setting the QT_VERSION
env var over the CI so for the moment this test is passing without doing any assert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I don't think QT_VERSION
is set in CI, ideally both possibilities should be checked. Do you happen to know how this works for the similar test test_qt_api
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think currently the test_qt_api
is going through the else
branch 😅 Checking the script that does the test setup (test.sh) I'm not seeing us setting up QT_API
🤔 Maybe we should try to check a way to setup QT_API
(and QT_VERSION
) for the tests (I think there is a USE_QT_API
that can be set for test but I think we are not using it here either). What do you think @ccordoba12 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think that's a good idea but I don't understand very well how to set up those env vars in our CIs.
Co-authored-by: Daniel Althviz Moré <[email protected]>
QT_VERSION
environment variableQT_VERSION
environment variable
This PR adds the option to specify the Qt version qtpy will use via the
QT_VERSION
environment variable and should resolve #476.If the
QT_VERSTION
but not theQT_API
environment variable is set, theAPI
is selected based onQT_VERSTION
(and follows the usual orderPyQt5
->PySide2
->PyQt6
->PySide6
if a binding is not installed). If both environment variables are set,QT_API
takes precedence. If the version cannot be satisfied or the environment variables are incompatible warnings are triggered.Please have an extra eye on the test when reviewing as I am not perfectly sure it will behave correctly in the CI environment.