Update VCPKG by acolombier · Pull Request #16095 · mixxxdj/mixxx

acolombier · 2026-03-04T01:29:06Z

Add latest version of all Mixxx dependencies

acolombier · 2026-03-04T01:32:34Z

+# Move custom module path at the end, so system and VCPKG ones take precedence (particularly findFFmpeg)
+list(POP_FRONT CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules")
+list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules")
+


This is required because QtMultimedia internally include findFFmpeg but because of Windows and MacOS FS case insensitivity, it defaults to our local module findFFMPEG leading to wrong variable casing: both find the same libraries, but one will set FFMPEG_FOUND, the other one FFmpeg_FOUND

The package name passed to `find_package_handle_standard_args` (FFMPEG) does not match the name of the calling package (FFmpeg). This can lead to problems in calling code that expects `find_package` result variables (e.g., `_FOUND`) to follow a certain pattern.

One other fix would be to rename our moduel to match Qt's cases. Toughs?

Having two files doing the same seems not to be good anyway. With the different naming, we can at least detect, which one was used.
@daschuer What do you think? Should we rename ours to the name Qt expects?

Solution like that change the standard behavior of cmake and should be avoided IMHO.

The file in question is this one:
https://github.com/qt/qtmultimedia/blob/0c08313895ce2f814281a993c83165c3282f9391/cmake/FindFFmpeg.cmake

There is another file in VCPKG with the upper case spelling:
https://github.com/microsoft/vcpkg/blob/master/ports/ffmpeg/FindFFMPEG.cmake.in

The spelling change has been introduced probably exactly for the same issue, see:
2674fa2

We had similar issue for project where files where provided via cmake directly or from upstream. We solved this by aligning our naming to them. I think it was libsqlite2 and sord, but unsure.

At least after looking around, our upercase spelling seems to be exceptional.
It is common with these find modules to have local and upstream versions. So I don't see an issue with the renaming. But we shall file an upstream issue for VCPKG.

Solution [that] change the standard behavior of cmake and should be avoided IMHO.

Just a heads up that this not changing the standard behaviour of CMake:

We add "${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules" early on in the CMAKE_MODULE_PATH, before we import the VCPKG toolchain, so we can import custom behaviour (namely default variable), which are needed before we can handle the VCPKG config and thus the toolchain setup.

If we think there is a strong need not to change the standard behavior of cmake, then I can remove our default variable, and so I will be able to set the path correctly, as we are already doing but after setting up the toolchain.

Also, just to clarify on "the standard behavior of cmake" - here is the documentation for CMAKE_MODULE_PATH

By default it is empty. It is intended to be set by the project.

Please justify why this solution circumvent the standard behavior of cmake

I went away and rename our module - didn't work. I copied Qt's one to use as our module - didn't work.

It feels like the module path approach is the simplest and most functional.

We do not follow a aaa-style (almost-always-auto).

This means when in doubt don't use auto. The cases where auto can be used are described explicit.

But again the discussion is probably mood here because you can make the local variable disappear by installing proper error handling which is enforced my [nodiscard] in the new Qt version.

I assume this was for #16095 (comment)
I have complied with the request now.

JoergAtGithub · 2026-03-04T06:17:34Z

+# Normalized DESTINATION paths in install()
+if(POLICY CMP0177)
+  cmake_policy(SET CMP0177 NEW)
+endif()
+


AFAIR CMP0177 is new in CMake 3.31.

Before CMake 3.31 OLD was the default, now the default is NEW.

To unify the behaviour, we would need to set OLD - right? Or do I misunderstand the purpose?

daschuer

Some minor comments.

daschuer · 2026-03-04T10:45:49Z

          # When downloading the manual. 3.21 is required for installing the
          # ANGLE Dlls via IMPORTED_RUNTIME_ARTIFACTS
-          cmake-version: "3.21.x"
+          cmake-version: "3.22.x"


The comment above need to be adjusted

Simply increasing the version number is not enough to make the comment apply.

The ANGLE DLLs are no longer the reason for the minimum required version; something else is now. The reason for 3.22 should be described here.

This is the minim Qt requirement. Should I put this here or just remove it and document it in CMakeFile?

If the general requirement in CMakeLists.txt is the same, we don't need a comment here. Than CMakeLists.txt is the better place for this information.

daschuer · 2026-03-04T10:48:55Z

-          # When downloading the manual. 3.21 is required for installing the
-          # ANGLE Dlls via IMPORTED_RUNTIME_ARTIFACTS
-          cmake-version: "3.21.x"
+          cmake-version: "3.22.x"


Here the comment about 3.26.4 is lost. How is the situation now?

daschuer · 2026-03-04T10:51:40Z

        QByteArray contentsBa = contents.toLocal8Bit();
        ScopedTemporaryFile pFile = std::make_unique<QTemporaryFile>();
-        pFile->open();
+        [[maybe_unused]] auto result = pFile->open();


I thin we shall check her the result, instead of continue n the error case.
The use of auto is not helpful here because we can guess the type.
In general we should never use auto for trivial types like bool here.

I will add the `DEBUG_assert like for 7afc501

The use of auto is not helpful here because we can guess the type.

I disagree, since we don't care of the type of result. If agree this is relevant if we want to properly handle this, but since this is only solving a compilation policy change rather than a feature change, handling this correctly, and thus needing to know the type is out of scope IMO.

During the review I looked up the Qt docks to find out what the type is.

We don't use auto just because we don't care about the type.
Here are our rules:
https://github.com/mixxxdj/mixxx/wiki/Coding-Guidelines#auto

You can make the type go away by putting the call into an if statement.

During the review I looked up the Qt docks to find out what the type is.

I don't see what this have to do with the type.
You would have to look at the doc to confirm the returned value anyway.

Here are our rules:

I cannot see anything about this usecase.

daschuer · 2026-03-04T12:45:54Z

+# Move custom module path at the end, so system and VCPKG ones take precedence (particularly findFFmpeg)
+list(POP_FRONT CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules")
+list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules")
+


Solution like that change the standard behavior of cmake and should be avoided IMHO.

The file in question is this one:
https://github.com/qt/qtmultimedia/blob/0c08313895ce2f814281a993c83165c3282f9391/cmake/FindFFmpeg.cmake

There is another file in VCPKG with the upper case spelling:
https://github.com/microsoft/vcpkg/blob/master/ports/ffmpeg/FindFFMPEG.cmake.in

The spelling change has been introduced probably exactly for the same issue, see:
2674fa2

We had similar issue for project where files where provided via cmake directly or from upstream. We solved this by aligning our naming to them. I think it was libsqlite2 and sord, but unsure.

At least after looking around, our upercase spelling seems to be exceptional.
It is common with these find modules to have local and upstream versions. So I don't see an issue with the renaming. But we shall file an upstream issue for VCPKG.

…t 6.10

acolombier added 3 commits March 4, 2026 01:27

chore: update VCPKG to use the latest TagLib version

04aa7f6

chore: update CMake version to meet minimum Qt reqs

5c4a838

chore: add maybe_unused to support nodiscard added in Qt 6.10

04dbfb2

github-actions Bot added build code quality developer experience Issues, bugs and PRs related to the development process, development environment & developer docs labels Mar 4, 2026

acolombier commented Mar 4, 2026

View reviewed changes

acolombier mentioned this pull request Mar 4, 2026

Add support for stem edit or update #15888

Open

acolombier requested a review from JoergAtGithub March 4, 2026 01:34

JoergAtGithub reviewed Mar 4, 2026

View reviewed changes

daschuer requested changes Mar 4, 2026

View reviewed changes

acolombier added 4 commits March 4, 2026 22:56

chore: change FFmpeg CMake module to match Qt's casing

d6d95e7

fixup! chore: update VCPKG to use the latest TagLib version

11826e5

fixup! chore: update CMake version to meet minimum Qt reqs

81ef545

fixup! chore: add maybe_unused to support nodiscard added in Qt 6.10

a6cceed

github-actions Bot added the cmake label Mar 4, 2026

fixup! chore: update VCPKG to use the latest TagLib version

4880a76

acolombier force-pushed the chore/update-vcpkg branch from 884a8b4 to 4880a76 Compare March 5, 2026 20:59

acolombier added 3 commits March 7, 2026 01:37

fixup! fixup! chore: add maybe_unused to support nodiscard added in Q…

45d51c7

…t 6.10

fixup! fixup! chore: update VCPKG to use the latest TagLib version

b11bc1d

fixup! chore: update CMake version to meet minimum Qt reqs

3fa5fc7

acolombier marked this pull request as draft March 7, 2026 23:16

Uh oh!

Conversation

acolombier commented Mar 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daschuer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acolombier Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daschuer Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

acolombier Mar 5, 2026 •

edited

Loading

daschuer Mar 4, 2026 •

edited

Loading