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

Build with Qt6 and optionally with QML #11608

Merged
merged 7 commits into from
Jun 3, 2023

Conversation

m0dB
Copy link
Contributor

@m0dB m0dB commented May 29, 2023

With this PR mixxx with its current skin-based UI can be built with Qt 6. Building with Qt 6 + QML is still possible, but the cmake option QT6 now builds the skin-based UI with Qt6, while the combination of cmake options QT6 and QML build the QML-based UI.

I have been using the official Qt 6.5 binary installer:

cmake .. -DCMAKE_PREFIX_PATH=~/Qt6/6.5.0/macos/ -DCMAKE_BUILD_TYPE=Release -DQT6=ON -DQTKEYCHAIN=OFF

As far as I see, after some smoke testing on macOS, everything works as expected except for the following:

  • The splitter below the waveforms doesn't work. I have done some debugging and found that the size policy of the child widgets changes somewhere between when the skin is read in the legacy skin parser and when I try to move the splitter. If I force the size policy back to it's original value it works.

  • The Latenight knobs look slightly different (more pronounced outline?), but nothing major.

  • I get a lot of the following warning: warning [Main] skipping QEventPoint(id=1 ts=0 pos=0,0 scn=599.994,259.461 gbl=599.994,259.461 Released ellipse=(1x1 ∡ 0) vel=0,0 press=-599.994,-259.461 last=-599.994,-259.461 Δ 599.994,259.461) : no target window

Also during compilation there are still a bunch of "deprecated" warnings for the Qt API. These should be easy to fix.

@@ -1907,7 +1907,7 @@
</ul>
</description>
</release>
<release version="2.3.6" type="development" date="2023-05-29" timestamp="1685366841">
<release version="2.3.6" type="development" date="2023-05-19" timestamp="1684533240">
Copy link
Member

Choose a reason for hiding this comment

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

The changes in this file are clearly unrelated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, pre-commit made this change.

Copy link
Member

@daschuer daschuer May 29, 2023

Choose a reason for hiding this comment

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

This was a failed merge conflict resolve. I have just pushed the fix.
In this case it is reasonable to remove this change via rebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Will do this tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -2038,6 +2047,8 @@ if(ENGINEPRIME)
message(STATUS "Using existing system installation of libdjinterop")
target_include_directories(mixxx-lib PUBLIC ${DjInterop_INCLUDE_DIRS})
target_link_libraries(mixxx-lib PRIVATE DjInterop::DjInterop)
find_package(ZLIB 1.2.8 REQUIRED)
target_link_libraries(mixxx-lib PRIVATE ${ZLIB_LIBRARIES})
Copy link
Member

Choose a reason for hiding this comment

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

Does this change relate to Qt6 or is it something we need in the 2.4 branch anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Qt6 only, and possibly because I am using the official Qt6 binary install. I suppose we should use a vcpkg buildenv with Qt6 instead?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. I will take a look to provide such a package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, with the current buildenv I get this warning for all Qt libraries when linking: warning: dylib (/Users/m0dB/Qt6/6.5.0/macos/lib/QtPrintSupport.framework/Versions/A/QtPrintSupport) was built for newer macOS version (11.0) than being linked (10.15)

Copy link
Member

Choose a reason for hiding this comment

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

You use the upstream distribution of QT, right? Since Qt 6.5 they build by default with a minimum of macOS 11.00
without a technical reason qt/qtbase@f2415df, while hey still allow to build for 10.15
https://github.com/qt/qtbase/blob/38c8eb8564495b0fe345eaca700966889f1f8aa3/cmake/QtBuild.cmake#L235

If we we do not use 11.0 only API, I think we should not drop 10.15 in our own build artificially.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I just noticed that it the minimum requirement is macOS 10.14
qt/qtbase@4933a5f

My build confirms it:
https://github.com/daschuer/vcpkg/actions/runs/5125452456

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree and it's a good reason to use Qt6 from our own buildenv.

@m0dB m0dB force-pushed the build-with-qt6-optional-qml branch from 5d326f8 to d5c96f4 Compare May 30, 2023 12:41
@github-actions github-actions bot added the skins label May 30, 2023
@m0dB
Copy link
Contributor Author

m0dB commented May 30, 2023

I have rebased on main to remove the irrelevant commit and I have also found and fixed the splitter issue. This was in fact a bug in the Latenight skin but apparently Qt 5 deals with conflicting size policies differently than Qt 6.

@ywwg
Copy link
Member

ywwg commented May 30, 2023

Build error:

/home/owen/src/github/mixxx/src/mixxxmainwindow.cpp:81:10: fatal error: QtX11Extras/QX11Info: No such file or directory
   81 | #include <QtX11Extras/QX11Info>

seems to be Zren/material-decoration#60

@ywwg
Copy link
Member

ywwg commented May 30, 2023

m0dB#5 ?

@m0dB
Copy link
Contributor Author

m0dB commented May 30, 2023

Yes, I have only compiled on macOS. Hopefully Windows and Linux developers can fix the compilation there.

The changes in the X11 module are documented here: https://doc.qt.io/qt-6/extras-changes-qt6.html (Changes to Qt X11 Extras, halfway the page)

@m0dB
Copy link
Contributor Author

m0dB commented May 30, 2023

Sorry, I didn't see your PR. Thanks. Merged!

@ywwg
Copy link
Member

ywwg commented May 30, 2023

the ubuntu 22.04 build error just looks like complaints about float/double conversion. it should be easy to add a static_cast to fix that

@ywwg
Copy link
Member

ywwg commented May 30, 2023

silencing the conversion warnings / errors: m0dB#6

@daschuer
Copy link
Member

Here is a QT6 build of the vcpkg environment:
https://github.com/daschuer/vcpkg/actions/runs/5125452456

@daschuer
Copy link
Member

I can confirm that this runs flawlessly even on my ages Ubuntu Focal using Qt 6.2.3 from:
https://launchpad.net/~daschuer/+archive/ubuntu/qt6-backports

@@ -2520,7 +2533,11 @@ endif()
if(APPLE OR WIN32)
# qt_de.qm is just one arbitrary file in the directory that needs to be located;
# there is no particular reason to look for this file versus any other one in the directory.
find_file(QT_TRANSLATION_FILE qt_de.qm PATHS "${Qt5_DIR}/../../../translations" "${Qt5_DIR}/../../qt5/translations" REQUIRED NO_DEFAULT_PATH)
if(QT6)
find_file(QT_TRANSLATION_FILE qt_de.qm PATHS "${Qt6_DIR}/../../../translations" "${Qt6_DIR}/../../qt5/translations" REQUIRED NO_DEFAULT_PATH)
Copy link
Member

Choose a reason for hiding this comment

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

This works for me:

Suggested change
find_file(QT_TRANSLATION_FILE qt_de.qm PATHS "${Qt6_DIR}/../../../translations" "${Qt6_DIR}/../../qt5/translations" REQUIRED NO_DEFAULT_PATH)
find_file(QT_TRANSLATION_FILE qt_de.qm PATHS "${Qt6_DIR}/../../translations/Qt6" REQUIRED NO_DEFAULT_PATH)

@uklotzde
Copy link
Contributor

It would be beneficial for early build testing if we could at least compile 2.4 with Qt6.

Is it mandatory to base these changes on main? Since the changes are hidden behind a build flag rebasing them should not cause any issues.

@daschuer
Copy link
Member

@uklotzde Please use the main branch for Qt 6 testing, this will be soon our new 2.5-beta after August 08, 2023 the 2.4 release date and we need a good amount of developer testing until than. For now it sound to me that 2.5 will be our first Qt 6 release. Let's make it real.
I have no interest that distros starting to enable Qt6 for 2.4 creating basically untested builds, as it has happened when switching to Qt 5.

@ywwg
Copy link
Member

ywwg commented May 31, 2023

finding a couple more build issues, updating my pr

@m0dB
Copy link
Contributor Author

m0dB commented May 31, 2023

Thanks, @ywwg , LGTM but before adding your PR to mine, I'd like to ask @daschuer if he prefers one larger PR to merge to main that contains all these changes or to do it in smaller steps.

@daschuer
Copy link
Member

Just merge m0dB#6 and than I will merge this one.

@ywwg
Copy link
Member

ywwg commented May 31, 2023

all builds pass so my PR is ready to merge into this one

Silence some float conversion warnings by making conversion explicit
@ywwg
Copy link
Member

ywwg commented Jun 2, 2023

@daschuer #6 is merged

@daschuer
Copy link
Member

daschuer commented Jun 3, 2023

Thank you. LGTM.
The next one is merging this: mixxxdj/vcpkg#71 before we can finally switch the 2.5 workflows to Qt6. Who can have a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants