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: Choose Qt binding to install from SPYDER_QT_BINDING environment variable #23055

Merged
merged 4 commits into from
Dec 2, 2024

Conversation

dpizetta
Copy link
Contributor

@dpizetta dpizetta commented Nov 22, 2024

Description of Changes

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot or animation (if affecting the UI, see Licecap)

This is a proposal to allow the user to choose the Qt binding to be installed when installing with PIP.

for example using PyQt6 (or PySide6 in the future).

Since the we could not rely on having other imports other thanstdlibavailable on setup.py when building and installing, we could not test if there is a binding already installed. For example, we could use QtPy to identify which one was installed, including it to setup_requires, and asking for qtpy.QT_API. But, also without success.
After testing some other ways to do that, I have implemented this version that checks the SPYDER_QT_BINDING environment variable to proper set the requirement during installation.

We can use like this:

SPYDER_QT_BINDING='pyqt6' pip install spyder

This is also compatible with test environments, where you can set the variable.

Not sure if this useful for conda also, I'm not familiar with it.

I want to hear from you guys, let me know your thoughts.

Links related to setup imports:

Issue(s) Resolved

Part of #20201

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: dpizetta

@pep8speaks
Copy link

pep8speaks commented Nov 22, 2024

Hello @dpizetta! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 150:27: E221 multiple spaces before operator

Comment last updated at 2024-12-02 16:48:07 UTC

@rear1019
Copy link
Contributor

This is a proposal to allow the user to choose the Qt binding to be installed when installing with PIP.

This is something I thought about as well while working on PR #23118. I really like the proposal: Things work by default, users have the option to chose a binding and only a single binding is installed. Other options I came up with have drawbacks:

  • Adding a pyside2/pyqt6/pyside6 section to extra_require while leaving pyqt5 in install_requires: Two bindings are installed.
  • Remove Qt bindings from install_requires and let the packager/end-user handle the installation of a binding (as mentioned here [1]): Not user-friendly.

[1] #20201 (comment)

setup.py Show resolved Hide resolved
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this @dpizetta! Some small suggestions for you, the rest looks good to me.

Also, do you need help to fix the failing test?

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@ccordoba12 ccordoba12 added this to the v6.0.3 milestone Nov 29, 2024
@ccordoba12 ccordoba12 changed the title Choose Qt binding from SPYDER_QT_BINDING environment variable PR: Choose Qt binding to install from SPYDER_QT_BINDING environment variable Nov 29, 2024
@dpizetta
Copy link
Contributor Author

dpizetta commented Dec 2, 2024

Thank you for taking a look! It should be OK after these changes. I may need some help with the AST errors. Thanks

Also, fix small style issue in setup.py
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now, thanks @dpizetta!

@ccordoba12 ccordoba12 merged commit 42be1ff into spyder-ide:master Dec 2, 2024
17 checks passed
@ccordoba12
Copy link
Member

@meeseeksdev please backport to 6.x

meeseeksmachine pushed a commit to meeseeksmachine/spyder that referenced this pull request Dec 2, 2024
ccordoba12 pushed a commit that referenced this pull request Dec 2, 2024
…om `SPYDER_QT_BINDING` environment variable) (#23142)
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.

4 participants