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

[WIP] Upgrade to Qt 6 #164

Closed
wants to merge 9 commits into from
Closed

Conversation

gmarmstrong
Copy link
Contributor

@gmarmstrong gmarmstrong commented May 31, 2022

This draft PR is a work in progress.

Commits are based on #161 but could be rebased onto main if preferred.

Switch from PySide2 to PySide6, which upgrades Dangerzone from Qt 5 to Qt 6. My immediate motivation was that PySide6 prebuilt binaries support a wider range of CPU architectures (namely Apple Silicon/M1).

Introduces the following issues, to be resolved before continuing:

  • Build instructions and CI/CD configurations need to be updated to address the dependency change. The apt and rpm repositories don't have python3-pyside6 packages, so they'll need to be installed through pip or poetry (or for packaging, through stdeb.cfg and pyp2rpm?)
  • Upstream bug re-introduced by Qt 6.2, causes a minor performance issue on startup and prints a warning like: qt.qpa.fonts: Populating font family aliases took 76 ms. Replace uses of missing font family "Menlo" with one that exists to avoid this cost. (font and delay vary by machine)
  • dangerzone.gui.main_window.ConvertThread causes a TypeError upon finishing (but this doesn't cause any actual issues for an otherwise successful document conversion):
    [...]
    100% Safe PDF created
    Traceback (most recent call last):
      File "/Users/guthrie/Projects/dangerzone/dangerzone/gui/main_window.py", line 512, in run
        self.finished.emit(self.error)
    TypeError: finished() only accepts 0 argument(s), 1 given!
    

PySide2 5.15.2.1 added support for Python 3.10
PySide6 prebuilt binaries have support a wider range
of CPU architectures (namely Apple Silicon/M1).
ConvertThread.finished conflicted with QThread.finished
With Qt 6, subclassing QApplication no longer seems to segfault
Was indicative of something that needs to be refactored, but this
change resolves the ImportError
Object needed to be bound to some variable
Copy link
Contributor Author

@gmarmstrong gmarmstrong left a comment

Choose a reason for hiding this comment

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

[...]

EDIT: can't review per-commit, apparently.

@deeplow
Copy link
Contributor

deeplow commented Jul 11, 2022

  • Upstream bug re-introduced by Qt 6.2, causes a minor performance issue on startup and prints a warning like: qt.qpa.fonts: Populating font family aliases took 76 ms. Replace uses of missing font family "Menlo" with one that exists to avoid this cost. (font and delay vary by machine)

I haven't been able to reproduce this in either a windows machine or a linux one (running fedora). In which machine did this happen?

Copy link
Contributor

@deeplow deeplow 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 yet another PR.

pyproject.toml Outdated Show resolved Hide resolved
dangerzone/gui/main_window.py Outdated Show resolved Hide resolved
dangerzone/gui/main_window.py Outdated Show resolved Hide resolved
QThread already sends a "finished" signal when it ends. This also
solves a previously introduced bug where the signal had a signature
of one bool argument representing if there was an error or not.

This arg's inclusion was introduced in commit ea47a2e
but seems it was added by mistake as the slot that catches the
signal (ConvertWidget.all_done) doesn't have any arguments.

This last part in particular was introducing an error in Qt6
since the signal became a private signal, whereas Qt 5 had
defined it as a just a signal [1].

[1]: freedomofpress#164 (comment)
@gmarmstrong
Copy link
Contributor Author

  • Upstream bug re-introduced by Qt 6.2, causes a minor performance issue on startup and prints a warning like: qt.qpa.fonts: Populating font family aliases took 76 ms. Replace uses of missing font family "Menlo" with one that exists to avoid this cost. (font and delay vary by machine)

I haven't been able to reproduce this in either a windows machine or a linux one (running fedora). In which machine did this happen?

On macOS Monterey 12.4 (M1 chip). The comments on the Qt bug report suggest that the issue might have only returned for macOS.

Remove unneeded "finished" signal in QThread
@deeplow
Copy link
Contributor

deeplow commented Jul 13, 2022

  • The apt and rpm repositories don't have python3-pyside6 packages, so they'll need to be installed through pip or poetry (or for packaging, through stdeb.cfg and pyp2rpm?)

I've been looking into this a bit. The issue is that even if we are able to install it with poetry, when building RPMs and DEBs won't be able to include these depetencies. Perhaps it can be installed via poetry at install time, but it feels like we'd be stepping over the distro's package manager and that could lead to issues in the future and definitely feels to "hacky" to be stable.

To me this seems like a blocker for this PR, unfortunately. But if I find a viable solution, I'll post it here.

@deeplow deeplow mentioned this pull request Aug 4, 2022
@gmarmstrong
Copy link
Contributor Author

  • The apt and rpm repositories don't have python3-pyside6 packages, so they'll need to be installed through pip or poetry (or for packaging, through stdeb.cfg and pyp2rpm?)

I've been looking into this a bit. The issue is that even if we are able to install it with poetry, when building RPMs and DEBs won't be able to include these depetencies. Perhaps it can be installed via poetry at install time, but it feels like we'd be stepping over the distro's package manager and that could lead to issues in the future and definitely feels to "hacky" to be stable.

To me this seems like a blocker for this PR, unfortunately. But if I find a viable solution, I'll post it here.

Are there really no other packages in the APT and RPM repositories that depend on Qt6?

@deeplow
Copy link
Contributor

deeplow commented Aug 5, 2022

Are there really no other packages in the APT and RPM repositories that depend on Qt6?

I think this is exactly the case! The closest I've been able to find is the persepolis package, which supports qt5 (via PyQt5) and PySide6. In their Debian package they ship the PyQt5 as a dependency instead of PySide6.

And this approach of supporting is not super elegant, let's say:

try:
    from PySide6.QtCore import Qt, QSize, QPoint, QFile, QIODevice, QTextStream
    from PySide6.QtWidgets import QWidget
    from PySide6.QtGui import QIcon
except:
    from PyQt5.QtCore import Qt, QSize, QPoint, QFile, QIODevice, QTextStream
    from PyQt5.QtWidgets import QWidget
    from PyQt5.QtGui import QIcon

In our case, it would probably look the same but with PySide2 instead of PyQt5.

@gmarmstrong
Copy link
Contributor Author

Well, that's pretty disappointing, especially since #166 and #168 depend on this. This is what I get for writing PRs that depend upon other PRs. 😅

I see several options here:

  1. Review the dependent PRs and, if you want to add those changes, then keep the whole commit history and downgrade to Qt 5 on top of it all.

  2. Rebase the dependent PRs onto main (or onto Support Python 3.10 #161).

  3. Wait some time, hoping that 9 months from now, when Qt 5 reaches EOL on 26 May 2023, the APT and RPM repositories will by then include a python3-pyside6 package (or an equivalent).


I will note that #161, #167, and #170 are all standalone changes that should neither depend upon nor conflict with this chain of PRs.

@deeplow
Copy link
Contributor

deeplow commented Aug 16, 2022

Yes, the review complexity is quite high because of that. I'd go for 2. at least for #168. Is this something you can do? Or shall I? Unit tests are probably needed soon and since you already made a base for it...

@deeplow deeplow mentioned this pull request Aug 16, 2022
@eloquence
Copy link
Member

We're not adding this to the next release milestone, as it's not strictly necessary to support M1/M2 Macs, but should pick this up again soon given the looming EOL for Qt5.

@deeplow deeplow mentioned this pull request Sep 21, 2022
@apyrgio apyrgio mentioned this pull request Nov 30, 2022
33 tasks
@deeplow
Copy link
Contributor

deeplow commented Dec 26, 2022

We won't be adding support for PySide6 on Linux at this time, but we will add it to Windows and MacOS.

Closing this in favor #296. Thanks @gmarmstrong for starting this discussion and suggesting a change. The new PR includes some of your suggestions.

@deeplow deeplow closed this Dec 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants