-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Upgrade to Qt6 #8118
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
base: master
Are you sure you want to change the base?
Upgrade to Qt6 #8118
Conversation
|
@tresf The Linux builds are failing in the packaging step, and the macOS x86_64 build is failing due to the macOS 10.13 target not supporting |
The SUIL dependency (LV2) links against Qt5 still. I'm unsure if we should bundle this or not (and until #7201 is merged, I'm not sure how easy this is to test). I would expect this dependency to be installed with # simplified for readiblity
linuxdeploy LMMS.AppDir
--desktop-file lmms.desktop
--plugin qt
--deploy-deps-only usr/lib/lmms
--deploy-deps-only usr/lib/lmms/ladspa
--deploy-deps-only usr/lib/jack/libjack.so.0
--deploy-deps-only /usr/lib/suil-0
--exclude-library *libgallium*
--verbosity 2
ERROR: Could not find dependency: libQt5Widgets.so.5 Required by: Links against the following: ldd suil-0/libsuil_x11_in_qt5.so
# libQt5Widgets.so.5 => libQt5Widgets.so.5 (0x0000fae163bd0000)
# libQt5Core.so.5 => libQt5Core.so.5 (0x0000fae163620000)
# libQt5X11Extras.so.5 => libQt5X11Extras.so.5 (0x0000fae1635f0000)... that's my best guess at least. Attempted via d95f598. If we choose NOT to bundle this, we may tell I also noticed under mixed Qt environments (ones that contain both Qt5 and Qt6) testing locally that our |
|
Looks like Qt6 support for X11 was not added to suil until this commit from last year: lv2/suil@35804e3 This is a part of version 0.10.22 which was released in January. To fix this, we may need to build suil from source like we do for fltk. Fortunately it's a tiny library, so it should not take much time to build it. |
Agreed however I think we should have unit tests to base this decision off of (the library is currently unused). If the UIs in #7201 crash when Qt versions are mixed, that helps drive this decision. Pinging @JohannesLorenz as an FYI. |
|
Also, somewhat related, but #8105 will drastically change the landscape of dependency decisions if we change our AppImages to use that. At time of writing this, AppImages produced by |
Patched via 80ab858, however this will raise the minimum target significantly.
Related: Quoting:
@messmerd I'm also fine tossing-out the script and just hard-coding this. |
That's definitely an issue.
That's fine with me. It could be set by variables in the matrix section like #8004 did originally. If needed, we could put comments there justifying the choice. I still think the script is worth keeping if we could repurpose it as a sanity check for our manually chosen |
Well, when I originally fought this proposal I was under the false assumption that deployment targets were hard-limited. Since we know now that it's more nuanced, I'm just fine using that technique. Thanks for the reminder, it makes writing the yaml quicker. :D
I'd love to keep it, but in my experience it's too unreliable as proven by the 10.13 -> 14.0 leap just by switching qt versions. Let's just ditch it and keep it hard-coded until we learn a better way (if a better way even exists). In hindsight, I find this easier to read as well. The one thing we lose is the logic telling us the lowest that the SDK supports, but that's so arbitrarily low, that I have a feeling we're more likely to remain at the mercy of which APIs us and our deps expect and never hit that lower boundary. |
|
Builds are fixed. We should probably download and test the various platforms before switching. I'm fine back-burner-ing SUIL task (although I would love feedback from @JohannesLorenz if possible) as well as shelving the x11embed task. |
I can surely test, but what exactly do you mean by "mixed"? I currently run CMake with this PR and get Edit: I think I got it. My suil links to Qt5: So, you ask, what happens if I rebase #7201 on this PR and compile with |
Correction... if you rebase against this AND use the appimage provided by the CI (which will use the mismatched Qt versions)
In short, Ubuntu doesn't offer SUIL libs compiled against Qt6 yet (your distro likely does), so if we continue to use Ubuntu to build our AppImages, we're at the mercy of whatever Ubuntu has (or as @messmerd has stated, may require using CI to compile SUIL libs from source). I'd like to know if SUIL compiled against Qt5 (when LMMS is compiled against Qt6) is a breaking change. If so, we'll have to leverage one of the above solutions. Note that our reliance on Ubuntu dependencies may change soon thanks to #8105, but I don't want to hold up Qt6 support for that effort. |
|
Thanks for explaining. I just rebased #7201 on master, and the commit of #7339 causes segfaults for the Lv2 UIs when I start them (at the point where it chooses a suiting UI type) - assuming I configure with |
|
Here is a backtrace of mentioned crash: Note the different Qt versions between frames 12 and 13. It looks like this for all plugins. |
|
Nonetheless I now tried running the Linux AppImage on my Arch PC. The same crashes, the backtrace looks almost the same: Once again, we see the Qt difference, now between frames 10 and 9. In both backtraces, it looks like libQt5Gui is linked against libQt6Core (in the first case, both libs are from my system, in the second case, both are inside the AppImage). That makes 0 sense though, and using ldd shows in both cases that this is not true. I interpret the line that libsuil tries to dynamically load libQt5Gui, while we use it in Qt6 context. In short, I think the problem is indeed that suil is compiled fixed, and we need to add it to our source tree to compile it with the correct Qt version. Note: I do use suil 0.10.22 on my Arch Linux, so in theory, this should support Qt6. |
@JohannesLorenz In #7201, |
Thanks for the hint! I added So I compiled suil from upstream ( For the local version, this entirely fixed it. For the AppImage version, however, I got This is indeed true, there is no such Concluding: Many distros will still ship an old suil, which will make Lv2 not work anyways. Even for my Arch (which is pretty much up to date) I had to compile for myself. Since compiling suil only takes 5-6 seconds, I guess it's fair to add it as a submodule. |
Btw, I can offer to do this myself - add a submodule to Let me know if I should get involved here. |
@PhysSong good catch! I had a feeling there was a lingering Qt5 reference somewhere!
Hmm... that's odd... I would expect all dependencies to deploy, since we call lmms/cmake/linux/LinuxDeploy.cmake Lines 108 to 114 in c79d6af
Note that the one line from this PR that was needed with mixed Qt versions is here. The rest of this PR only really pertains to the CI.
Hmm... I don't understand... why would suil want a lib that doesn't exist? If x11 support for Qt6 is somehow missing or disabled, we need to gracefully handle this. @JohannesLorenz what I was really curious about, was whether or not AppImages created from Ubuntu 22.04 were capable of running Qt5 SUIL alongside Qt6 LMMS. That's most easily done by making a branch and testing the artifacts created by the CI, |
It is disabled, because I didn't want to work on the x11embed submodule (I could if i wanted to, but I felt too lazy). Also, every single embedding method seems kinda broken lately. We should try to fix it but for now, I felt having no embed by default is fine. If embedding is really needed, I would prefer to use qt embedding, but that's my preference. But unfortunately, qt embedding is broken too the last time I tested. |
This was what I did before PhysSong's comment, right? Which leaded to the gdb logs showing the mismatching Qt versions and the crashes. Then PhysSong said I should run Qt6 SUIL alongside Qt6 LMMS - which I then did. If you want to check, my branch is here - though this is with PhysSong's suggestion. |
Thanks! (yes, the builds from CI are broken; LV2 UIs segfault for me too, Ubuntu 24.04, ARM64)
First, I think #cmakedefine QT_VERSION_MAJOR @QT_VERSION_MAJOR@However I don't think this is correct in the context of SUIL since this assumes the LMMS' Qt version is always the same as the SUIL's Qt version...
... Apparently I authored |
|
I'm fine with adding This option is simple and will always work regardless of Qt version. It's also easy on devs since no one needs to bother with providing a compatible The only downside is If for some reason we instead want devs to provide their own pre-built If we have the strings /usr/lib/x86_64-linux-gnu/libsuil-0.so | grep suil_x11_in_qtFor me, this just prints "suil_x11_in_qt5" which confirms the EDIT: Or checking that @JohannesLorenz We don't need to set a #if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)
// Using Qt6
#else
// Using Qt5
#endifOr: #define QT_VERSION_MAJOR (QT_VERSION >> 24) |
Hmm... this may very well be true, but so far, we have no evidence to support this claim.
That's the crude solution I'd use.
Good catch. |
Mockup here: da01ed2 include(FindSuilModules.cmake)
message("SUIL Module wants Qt${Suil_QT_VERSION_MAJOR}")Output: |
I agree with this approach, especially if building suil doesn't take long. Sounds better than fiddling around packaging and build tools. |
It seems clear to me that |
|
FYI, a suil installation may provide both Qt 5 and Qt 6 modules, like in Arch Linux: |
AFAIU,
Good point, that's a fall through condition in the cmake code if we're to use it. |
|
FYI, Homebrew seems to be a bit behind in this regard too and still only provides a qt5 version... ... I'm testing out mixed versions (Linux) on a separate branch to see if they're even possible, just waiting for the CI to finish. |
The builds from https://github.com/tresf/lmms/actions/runs/19385835739#artifacts fail to load the LV2 UIs, but also don't segfault like the previous "Qt6UI" builds did. I'm not sure this matters though, if I force the AppImage to use So I concede @messmerd 😅 let's just build them in. Even if we can mix them, I'm not sure it's worth our time to figure out how. |
Unfortunately, suil supports Qt 6 only on Linux + X11 for now. Even worse, there's no Qt support on Windows(gtk2 only). |
|
For Windows, Qt 6.2+ require Win 10 V1809. |
It only took 5 seconds compiling, 5 seconds linking for me. Also, we may elide creating most libraries, we only need one (Qt5 OR Qt6). Which might be a reason to bypass the meson buildfile and use our own CMakeLists as usual. This also avoids the meson dependency.
For me, it's still not available because I'm on "stable" branch :(
I think that's even vital, given how slow distros adapt.
Do we want to support this, too? Like All in all, I would be ready to make a PR against this PR, but I am also fine if anyone else wants to try... |
Probably overkill.
Why not Gtk? |
If you look at this, the Here is the code which proves that ( |
So are you saying that LMMS is incapable of loading Gtk LV2 plugins at all? Is this intended behavior? |
For clarification, AFAIU, SUIL's entire point is to NOT need to be using the framework that the plugin was built against, quoting (again)
... this should be the same for Gtk, no? Am I misunderstanding this framework? |
|
To clarify, for example, |
Ok, that explains why mixing Qt versions would never work. if that's the case, we need to omit it from packaging, the Gtks and Qt5s are currently being processed by the AppImage and DMG scripts. |
Upgrades all builds (except Windows MinGW) to Qt6.
TODO:
MACOSX_DEPLOYMENT_TARGET=14.0is wanted