Skip to content
This repository has been archived by the owner on May 12, 2023. It is now read-only.

4.2.4 + Qt6 #30

Merged
merged 2 commits into from
Sep 5, 2022
Merged

4.2.4 + Qt6 #30

merged 2 commits into from
Sep 5, 2022

Conversation

guihkx
Copy link
Contributor

@guihkx guihkx commented Aug 27, 2022

I just want to test if this fixes martinrotter/rssguard#565, once and for all.

My current setup:

  • Arch Linux
  • GNOME 42.4
  • RSS Guard 4.2.3 (built with Qt 5.15.4)

Currently, there doesn't seem to leak file descriptors as badly as before (presumably caused by just calling QSystemTrayIcon::isSystemTrayAvailable()), so that's good.

However, there is this other horrible leak triggered by just hovering links in the WebView (caused by QWebEngineView::setHtml()?):

hovering-fd-leak.mp4

The commands I used in the video:

  • flatpak run com.github.rssguard -w (to launch RSS Guard with "basic" WebView support)
  • watch -n.1 'bash -c "echo fd count: ±$(ls -l /proc/$(pidof rssguard)/fd | wc -l)"' (to monitor fd count)

However, I've been experimenting with Arch Linux's official build of RSS Guard, which only recently switched to building with Qt 6. From my testing, the same fd leak above does not happen in the Qt 6 build, so I want to see if this hopefully fixes the Flatpak package as well!

Closes #31

@flathubbot
Copy link
Contributor

Started test build 106746

@flathubbot
Copy link
Contributor

Started test build 106747

@guihkx
Copy link
Contributor Author

guihkx commented Aug 27, 2022

-- Could NOT find Qt6Core5Compat (missing: Qt6Core5Compat_DIR)
CMake Error at CMakeLists.txt:148 (find_package):
  Found package configuration file:
    /usr/lib/x86_64-linux-gnu/cmake/Qt6/Qt6Config.cmake
  but it set Qt6_FOUND to FALSE so package "Qt6" is considered to be NOT
  FOUND.  Reason given by package:
  Failed to find Qt component "Core5Compat".
  Expected Config file at
  "/usr/lib/x86_64-linux-gnu/cmake/Qt6Core5Compat/Qt6Core5CompatConfig.cmake"
  does NOT exist

Will have to investigate this one...

[EDIT] Update

Qt6Core5Compat is missing from the KDE runtime: https://invent.kde.org/packaging/flatpak-kde-runtime/-/issues/26

Workaround for now is to just build it ourselves.

@flathubbot
Copy link
Contributor

Started test build 106751

@flathubbot
Copy link
Contributor

Started test build 106753

@flathubbot
Copy link
Contributor

Build 106753 failed

@flathubbot
Copy link
Contributor

Started test build 106754

@flathubbot
Copy link
Contributor

Build 106746 failed

@flathubbot
Copy link
Contributor

Build 106747 failed

@flathubbot
Copy link
Contributor

Build 106751 failed

@flathubbot
Copy link
Contributor

Started test build 106756

@flathubbot
Copy link
Contributor

Build 106756 failed

@guihkx
Copy link
Contributor Author

guihkx commented Aug 27, 2022

Okay, so now it at least builds fine, but it fails to validate the dimensions of the RSS Guard icon for not being a perfect square. 🤡

@martinrotter I'll add the icon here for now just to test this Qt 6 build, but for the next release, can you please fix this ~REALLY IMPORTANT~ icon validation issue? Thanks!

EDIT: I've sent a pull request here.

@flathubbot
Copy link
Contributor

Build 106754 failed

@flathubbot
Copy link
Contributor

Started test build 106765

@flathubbot
Copy link
Contributor

Started test build 106766

@flathubbot
Copy link
Contributor

Build 106766 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/104443/com.github.rssguard.flatpakref

@flathubbot
Copy link
Contributor

Started test build 106768

@flathubbot
Copy link
Contributor

Build 106768 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/104445/com.github.rssguard.flatpakref

@guihkx
Copy link
Contributor Author

guihkx commented Aug 27, 2022

Yup, it definitely fixes it:

qt6-hovering-fd-leak-fixed.mp4

Nevertheless, I'll be using this Qt 6 build for a while just to make sure there are no obvious bugs.

@flathubbot
Copy link
Contributor

Build 106765 failed

@guihkx
Copy link
Contributor Author

guihkx commented Aug 31, 2022

Okay, so after leaving RSS Guard running for about 3 days straight, I can confirm that the file descriptor leak issue is gone for good!

The switch to Qt 6 causes only two minimal differences, but they should only affect GNOME users with a very specific setup.

The first issue is simply the placeholder text in input fields being hard to read (reported to upstream here), but it only affects people using QGnomePlatform/adwaita-qt, which are components used to better integrate Qt apps running on GNOME.

The second issue is related to system tray notifications. On the Qt 5 build, notifications were sent directly to GNOME's "notification center", also thanks to QGnomePlatform, but now, they are displayed as a balloon like this (which I think it's how it is on KDE?):

Screenshot from 2022-08-30 05-12-04

For me, this is not a big deal and it's actually better to read than GNOME's notification center. Nevertheless, this has been already fixed, but there's no release with the fix just yet.

To sum up, if you are okay with the changes, I think this PR could be postponed to the next RSS Guard release, so I can drop the patches and make the PR cleaner.

@martinrotter
Copy link
Collaborator

In fact, I believe that this could/should be merged right now. Feel free to rearrange the PR to make it cleaner and then let me know and I will merge.

@guihkx
Copy link
Contributor Author

guihkx commented Aug 31, 2022

Awesome! Well, the PR is pretty much ready at this point. Just keep in mind that if you want to merge this right now, on the next RSS Guard release you'll have to modify the manifest a bit and remove the left over files, because the two fixes that I'm using here have already been upstreamed by you here and here.

So for the next release you'd just delete lines 56 through 65 and then delete these two files: qt6-build-fix.patch and rssguard-perfect-square.png.

@guihkx guihkx marked this pull request as ready for review August 31, 2022 14:04
@flathubbot
Copy link
Contributor

Started test build 108278

@guihkx guihkx changed the title build: use Qt6 4.2.4 + Qt6 Sep 5, 2022
@flathubbot
Copy link
Contributor

Build 108278 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/105944/com.github.rssguard.flatpakref

@guihkx
Copy link
Contributor Author

guihkx commented Sep 5, 2022

@martinrotter Please merge this one instead of #31 (which I also included here). Thank you!

@martinrotter martinrotter merged commit fa2cdff into flathub:master Sep 5, 2022
@guihkx guihkx deleted the qt6 branch September 5, 2022 19:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: socket descriptor leakage on Linux
3 participants