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

PR: Add QT_VERSION environment variable #478

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Bzero
Copy link

@Bzero Bzero commented Apr 13, 2024

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 the QT_API environment variable is set, the API is selected based on QT_VERSTION (and follows the usual order PyQt5 -> 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.

@coveralls
Copy link

coveralls commented Apr 13, 2024

Coverage Status

coverage: 96.781%. remained the same
when pulling b8c9d20 on Bzero:add_QT_VERSION_environment_variable
into 3238de7 on spyder-ide:master.

@ccordoba12 ccordoba12 added this to the v2.5.0 milestone Apr 13, 2024
@dalthviz dalthviz assigned Bzero and unassigned dalthviz Apr 16, 2024
@dalthviz dalthviz self-requested a review April 16, 2024 19:41
Copy link
Member

@dalthviz dalthviz left a 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 👍

qtpy/__init__.py Outdated Show resolved Hide resolved
Comment on lines +173 to +183
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()
Copy link
Member

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?

Copy link
Author

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?

Copy link
Member

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 ?

Copy link
Member

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é <16781833+dalthviz@users.noreply.github.com>
@dalthviz dalthviz changed the title Add QT_VERSION environment variable PR: Add QT_VERSION environment variable Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to specify Qt version via an environment variable (QT_VERSION)
4 participants