CMake: add option to build with Qt6 #4051

Be-ing · 2021-07-03T03:22:28Z

No description provided.

Holzhaus · 2021-07-03T07:48:14Z

CMakeLists.txt

-    install_qt5_plugin(Qt5::QSvgIconPlugin BUNDLE_LIBS "${MIXXX_INSTALL_PREFIX}")
-    install_qt5_plugin(Qt5::QJpegPlugin BUNDLE_LIBS "${MIXXX_INSTALL_PREFIX}")
-    install_qt5_plugin(Qt5::QGifPlugin BUNDLE_LIBS "${MIXXX_INSTALL_PREFIX}")
+    install_qt5_plugin(Qt${QT_VERSION_MAJOR}::QCocoaIntegrationPlugin BUNDLE_LIBS "${MIXXX_INSTALL_PREFIX}")


Install Qt5 plugin?

I'm going to defer working on this macOS packaging for now since it breaks the Qt5 build.

Holzhaus · 2021-07-03T07:49:47Z

CMakeLists.txt

 )
+if(NOT QT6)


Do we really need to remove all these files? Can't we keep everything except QGLWidget?

I would also suggest to keep everything that builds fine with Qt5 and only remove files conditionally if incompatibilities arise or if they become obsolete.

No, there is no point trying to get the legacy skin system to build with Qt6 because essential features cannot work with Qt6. That would probably require lots more preprocessor Qt version checks scattered around the legacy code.

Which ones won't work? I though only QOpenGLWidget?

The legacy skin system uses QGLWidget, not QOpenGLWidget, which is the reason it needs to be replaced.

Do you consider it feasible to start with this radical switch and then gradually try to re-enable the legacy skin components for Qt6?

Yes, I advocated for this approach with the library but @Holzhaus was against it. Since @Holzhaus is doing the work on that and I am minimally familiar with the library code, I trust that his approach is a good way to proceed. I ask that trust be reciprocated here. From my experiences redesigning Deere, implementing the effects system, following #1974, and struggling for months with every idea I could think of to improve macOS performance with the legacy skin system, I judge that the legacy skin system is a dead end that we should abandon as soon as possible. macOS performance is still only kinda passable now and we have no idea how to fix it without starting from scratch with Qt Quick. I want to implement a new feature on top of #2618 for one big effect chain with more effects, but just the thought of adding more complexity with the legacy skin system -- and repeating that four times! -- is making me reconsider whether that is worthwhile at this time. I will not put any work into backtracking by putting QQuickWidgets into legacy skins. We know from the Qt documentation such an approach would have major performance limitations and it might not even solve our persistent macOS troubles.

Mixxx's CPU usage at baseline, which is due to the hugely inefficient GUI, is terrible. I have never gotten Mixxx to run without my computer's fans turning on. This is should not happen. Mixxx is not a fancy schmancy 3D game. Our fanciest graphics are just drawing a waveform. This should not be difficult at all for my fancy schmancy laptop, yet it is. Mixxx should be able to run easily on hardware as weak as a PinePhone, but we won't get there if we cling to very inefficient legacy GUI technology. The skin system dates from before Qt had any comparable solution. It essentially does what Qt did with the XML GUI layouts for Qt Designer, but badly. Qt's current solution with Qt Quick is much, much better both for performance and maintainability. Let's embrace it without hesitation and drop the legacy code ASAP.

We cannot drop legacy code until the new code works fine. That's my whole point. Otherwise we'll have a broken mixxx main branch and cannot do a mixxx release in years.

I know this is just for Qt6 support and I'm okay with merging initial Qt6 support without the legacy skin system, but we should not burn bridges unless we need to.

I want to implement a new feature on top of #2618 for one big effect chain with more effects, but just the thought of adding more complexity with the legacy skin system -- and repeating that four times! -- is making me reconsider whether that is worthwhile at this time.

I have no objections against a new hidden feature that is not accessible from the legacy skins, just from QML. But if that feature breaks legacy skins, we cannot merge at this time. QML skin feature parity is probably still years away, and I'd like to do more releases until then.

We need to ensure that we are all in a position to make progress. No one is forced to work on features that they do not support. Nevertheless we need to coordinate our work and be aware of the differing goals to prevent conflicts and frustration.

I guess we all agree that we want and need to replace the legacy skins with QML asap. If it turns out that keeping the legacy skins alive imposes too much work or would pollute the code base unacceptably we can decide to stop this maintenance effort at any time and shift focus. If we could agree to proceed with the intent to defer this decision until it becomes inevitable instead of giving up recklessly this would help.

How about extracting Qt6-only components/modules into separate files if needed, e.g. the new QML UI based on #2618? This will hopefully prevent to mix conflicting Qt5/Qt6 code in the same file when managing the differences becomes inconvenient. Not sure how to deal with the split at the QML-level where we also have backwards compatible components that are not even using all the features available in Qt 5.15, mainly due to outdated Ubuntu dependencies.

We cannot drop legacy code until the new code works fine.

I fully agree. But wasting effort on maintaining legacy code with Qt6 does nothing to help us get there, it only puts obstacles in the way.

I know this is just for Qt6 support and I'm okay with merging initial Qt6 support without the legacy skin system, but we should not burn bridges unless we need to.

The point of this approach is that it does not touch the legacy code at all. It just doesn't build the old code with Qt6. No bridges were harmed in the making of this PR.

Not sure how to deal with the split at the QML-level where we also have backwards compatible components that are not even using all the features available in Qt 5.15, mainly due to outdated Ubuntu dependencies.

When we get CI setup for Qt6 we could make QML build for only Qt6 and not worry about old Qt5 versions. Here is a PR to start that:
mixxxdj/vcpkg#19

After we get vcpkg working on macOS, it should be easy to use vcpkg for Linux CI as well.

uklotzde · 2021-07-04T18:05:06Z

Closed? I was considering this approach as feasible and a valid starting point if we could agree on how to continue from here.

Be-ing · 2021-07-04T20:58:36Z

I'm not continuing if others insist on putting huge unnecessary obstacles in my way for no benefit.

Holzhaus · 2021-07-04T21:28:26Z

I'm not continuing if others insist on putting huge unnecessary obstacles in my way for no benefit.

What obstacles? I already said above:

I know this is just for Qt6 support and I'm okay with merging initial Qt6 support without the legacy skin system.

I think it's reasonable to ask if it wouldn't suffice to disable QGLWidget. You didn't really answer that and just said that there is no point. The discussion then morphed into a debate about development philosophies (incremental changes and improvements vs. throwing everything away and starting from scratch).

I have a different opinion on that question, but that doesn't need to hold up this PR. I'd prefer if we just disabled QGLWidget, but if you don't want to do this, I'm open to merging anyway. Someone else might do this later on, if deemed necessary.

uklotzde · 2021-07-04T22:04:03Z

@Be-ing Thanks ;) I guess we all have the same goals, even if the ideas about how to reach them are discussed controversially.

Be-ing · 2021-09-16T14:27:44Z

There are lots and lots of changes throughout the code base that will be needed to get Mixxx compiling with Qt6. Trying to take care of them all in one giant PR would likely create a bunch of merge conflicts and take a long time. Let's merge this as it is simply adding a CMake option to build with Qt6. The code changes to get it to build can be done in small PRs one by one.

uklotzde

The legacy files are only excluded when building for Qt6. I consider this approach legit.

Holzhaus

Just a little suggestion to make the code easier. Otherwise it looks good to me (although I cannot test this, just reviewed on my phone).

CMakeLists.txt

Be-ing · 2021-09-17T21:11:13Z

Merge?

QT6=ON does not build yet.

Be-ing · 2021-09-18T22:48:58Z

@uklotzde merge? It is cumbersome to work towards Qt6 support without this merged.

uklotzde

I was waiting for Jan's final approval. Let's merge and improve it afterwards if needed.

Thank you! LGTM

Holzhaus · 2021-09-19T21:15:54Z

Yes, sorry. I'm still traveling. Don't wait for my approval right now and just merge if you reviewed and LGTM'ed a PR. I'll let you know when I'm available for reviews again.

Be-ing marked this pull request as draft July 3, 2021 03:22

github-actions bot added the build label Jul 3, 2021

Be-ing force-pushed the qt6 branch 6 times, most recently from ef92869 to c628019 Compare July 3, 2021 04:25

Be-ing mentioned this pull request Jul 3, 2021

Qt6 build system prerequisites #4052

Merged

Holzhaus reviewed Jul 3, 2021

View reviewed changes

Be-ing closed this Jul 4, 2021

Be-ing reopened this Jul 4, 2021

Be-ing force-pushed the qt6 branch 5 times, most recently from 084b0fb to b2c2218 Compare September 16, 2021 14:23

Be-ing marked this pull request as ready for review September 16, 2021 14:24

Be-ing changed the title ~~add option to build with Qt6~~ add CMake option to build with Qt6 Sep 16, 2021

Be-ing changed the title ~~add CMake option to build with Qt6~~ CMake: add option to build with Qt6 Sep 16, 2021

uklotzde approved these changes Sep 16, 2021

View reviewed changes

Be-ing requested a review from Holzhaus September 17, 2021 05:59

Holzhaus reviewed Sep 17, 2021

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

CMake: add option to build with Qt6

fa8dae5

QT6=ON does not build yet.

Be-ing force-pushed the qt6 branch from b2c2218 to fa8dae5 Compare September 18, 2021 18:45

uklotzde approved these changes Sep 19, 2021

View reviewed changes

uklotzde merged commit 8dcd528 into mixxxdj:main Sep 19, 2021

Be-ing deleted the qt6 branch September 19, 2021 11:20

daschuer mentioned this pull request Dec 12, 2021

Updated CHANGELOG.md for 2.4 beta #4564

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake: add option to build with Qt6 #4051

CMake: add option to build with Qt6 #4051

Be-ing commented Jul 3, 2021

Holzhaus Jul 3, 2021

Be-ing Jul 3, 2021

Holzhaus Jul 3, 2021

uklotzde Jul 3, 2021

Be-ing Jul 3, 2021

Holzhaus Jul 3, 2021

Be-ing Jul 3, 2021

Be-ing Jul 4, 2021 •

edited

Loading

Holzhaus Jul 4, 2021 •

edited

Loading

uklotzde Jul 4, 2021

Be-ing Jul 4, 2021

Be-ing Jul 4, 2021

uklotzde commented Jul 4, 2021

Be-ing commented Jul 4, 2021

Holzhaus commented Jul 4, 2021 •

edited

Loading

uklotzde commented Jul 4, 2021

Be-ing commented Sep 16, 2021

uklotzde left a comment

Holzhaus left a comment

Be-ing commented Sep 17, 2021

Be-ing commented Sep 18, 2021

uklotzde left a comment

Holzhaus commented Sep 19, 2021

		)
		if(NOT QT6)

CMake: add option to build with Qt6 #4051

CMake: add option to build with Qt6 #4051

Conversation

Be-ing commented Jul 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing Jul 4, 2021 • edited Loading

Choose a reason for hiding this comment

Holzhaus Jul 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Jul 4, 2021

Be-ing commented Jul 4, 2021

Holzhaus commented Jul 4, 2021 • edited Loading

uklotzde commented Jul 4, 2021

Be-ing commented Sep 16, 2021

uklotzde left a comment

Choose a reason for hiding this comment

Holzhaus left a comment

Choose a reason for hiding this comment

Be-ing commented Sep 17, 2021

Be-ing commented Sep 18, 2021

uklotzde left a comment

Choose a reason for hiding this comment

Holzhaus commented Sep 19, 2021

Be-ing Jul 4, 2021 •

edited

Loading

Holzhaus Jul 4, 2021 •

edited

Loading

Holzhaus commented Jul 4, 2021 •

edited

Loading