Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Replace PyQt5 with pyside2 #1676

Closed
mkolar opened this issue Jun 10, 2021 · 8 comments · Fixed by #1744
Closed

Replace PyQt5 with pyside2 #1676

mkolar opened this issue Jun 10, 2021 · 8 comments · Fixed by #1744
Assignees
Labels
type: enhancement Enhancements to existing functionality

Comments

@mkolar
Copy link
Member

mkolar commented Jun 10, 2021

PyQt5 license is more restrictive than PySide2, so if at all possible we should replace it. We're not currently providing pre-built binaries, but if we decide to do it, this might be a potential blocker on that front.

[cuID:PYPE-1334]

@mkolar mkolar added the type: enhancement Enhancements to existing functionality label Jun 10, 2021
@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Jun 10, 2021

This require to have a folder in OpenPype build (and code) that is added to sys.path on OpenPype launch but is not added to PYTHONPATH (currently we're lucky that none of hosts is using PyQt5). @antirotor would that be possible?

EDIT:
That may be an issue in code right? As pyenv/poetry can install python modules only to one directory...

@iLLiCiTiT
Copy link
Member

When this is done, all we need is to replace PyQt5 with PySide2 in modules... (and maybe fix few smaller issues here and there)

@antirotor
Copy link
Member

This require to have a folder in OpenPype build (and code) that is added to sys.path on OpenPype launch but is not added to PYTHONPATH (currently we're lucky that none of hosts is using PyQt5). @antirotor would that be possible?

EDIT:
That may be an issue in code right? As pyenv/poetry can install python modules only to one directory...

Please remind me - this is because we must not mix pyside2 with pyqt5 in the host, right?

@mkolar
Copy link
Member Author

mkolar commented Jun 10, 2021

Please remind me - this is because we must not mix pyside2 with pyqt5 in the host, right?

not entirely. that is doable.

The main issue is their licensing. PyQt5 is available under a GPL or commercial license, and PySide2 under a LGPL license.

Effectivelly meaning that you ran provide pre-built binaries with pyside, but not with PyQt5. Even though we are providing source code even if there are binaries. It would still be a lot simpler to work with various distribution situations with LGPL license. That's actually the whole reason for existence of PySide

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Jun 10, 2021

this is because we must not mix pyside2 with pyqt5 in the host, right?

It is because host may not use it's python modules as first but use what is in PYTHONPATH as first and since Qt bindings are build for specific version of python it will crash.

2 issues

  • Maya will crash on start if you have different than maya's PySide2 in PYTHONPATH because will try to use it before it's own PySide2
  • The same for Blender where we install PySide2 inside it's python but then we would pass PySide2 from OpenPype with PYTHONPATH so it will crash because of same reason

EDIT:
It didn't crash because we passed PyQt5 so PySide2 imports still work. So it's actually opposite issue that we should not mix openpype's pyside2 with host's pyside2.

@iLLiCiTiT
Copy link
Member

And since we have to pass our PYTHONPATH with our python packages we need to have 2 separated directories.

I think the best would be to have 2 venvs one for python modules that should not be passed to any host (Python modules, Qt binding,...) and other we want to pass...

@iLLiCiTiT
Copy link
Member

Could we install PySide2 and shiboken2 using wheels into different directory? We will probably loose auto version control but I don't see other option...

@antirotor
Copy link
Member

Could we install PySide2 and shiboken2 using wheels into different directory? We will probably loose auto version control but I don't see other option...

yeah, me to. I would add function to lib, something like get_qt_bindings() that will then get it on demand from that different directory. create_env can install it via Poetry to .venv but then move it out to that other dir - thus we will retain management via Poetry.

@antirotor antirotor mentioned this issue Jun 22, 2021
17 tasks
@mkolar mkolar added cu and removed cu labels Aug 24, 2021
@ynbot ynbot assigned antirotor and unassigned iLLiCiTiT Sep 27, 2021
@gitkraken-boards gitkraken-boards bot assigned antirotor and unassigned antirotor Sep 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: enhancement Enhancements to existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants