cmake: Avoid hardcoding Qt's major version in Find module / variable names

hebasto commented at 12:45 pm on October 1, 2024: member

This PR facilitates future migration to Qt 6 and is a prerequisite for #30997.

No behaviour change.

cmake: Avoid hardcoding Qt's major version in Find module

This change facilitates future migration to Qt 6.

deacf3c7cd

hebasto added the label Build system on Oct 1, 2024

DrahtBot commented at 12:45 pm on October 1, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, promag, maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30997 (build: Switch to Qt 6 by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko added the label DrahtBot Guix build requested on Oct 1, 2024

DrahtBot commented at 7:31 pm on October 1, 2024: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit fc642c33ef28829eda0119a0fe39fd9bc4b84051(master)	commit 05d9a14c3df8665da6f2e506362cf893d5cd9195(master and this pull)
guix_build.log	`08c5fbe5d07150d8...`	`621e8b126d06680e...`
SHA256SUMS.part		`8d7f5731d2bd8c06...`
*-aarch64-linux-gnu-debug.tar.gz		`bafe314a204a7b0d...`
*-aarch64-linux-gnu.tar.gz		`b7ba214b5a49480d...`
*-arm-linux-gnueabihf-debug.tar.gz		`369109998e0028ea...`
*-arm-linux-gnueabihf.tar.gz		`d44b05a86c093532...`
*-arm64-apple-darwin-unsigned.tar.gz		`e3d5b544612c8bbc...`
*-arm64-apple-darwin-unsigned.zip		`f5a6ec285c97ec1c...`
*-arm64-apple-darwin.tar.gz		`9253378e8e9c3d6d...`
*-powerpc64-linux-gnu-debug.tar.gz		`ced736975a3f47a1...`
*-powerpc64-linux-gnu.tar.gz		`48097fb1753a7245...`
*-riscv64-linux-gnu-debug.tar.gz		`2c71bd13e9a85097...`
*-riscv64-linux-gnu.tar.gz		`4dc3169f59878f37...`
*-x86_64-apple-darwin-unsigned.tar.gz		`1472e26d1a620a3f...`
*-x86_64-apple-darwin-unsigned.zip		`9f44a51b27e65620...`
*-x86_64-apple-darwin.tar.gz		`217a98edcba97211...`
*-x86_64-linux-gnu-debug.tar.gz		`a2e7d2f6cca74058...`
*-x86_64-linux-gnu.tar.gz		`7b9ba448b9c5d57a...`
*.tar.gz		`6d35dde5dbb66ca4...`
guix_build.log.diff		`f8454806f959f803...`

DrahtBot removed the label DrahtBot Guix build requested on Oct 1, 2024

hebasto added the label Refactoring on Oct 2, 2024

in CMakeLists.txt:180 in deacf3c7cd

176@@ -177,7 +177,7 @@ if(BUILD_GUI)
177   if(BUILD_GUI_TESTS)
178     list(APPEND qt_components Test)
179   endif()
180-  find_package(Qt5 5.11.3 MODULE REQUIRED
181+  find_package(Qt 5.11.3 MODULE REQUIRED

l0rinc commented at 2:12 pm on October 2, 2024:

It seems to me we’re still hardcoding the major version - can’t we change this during migration instead?

hebasto commented at 2:17 pm on October 2, 2024:

It seems to me we’re still hardcoding the major version

I meant the variable names. The full minimum required version was not supposed to change.

can’t we change this during migration instead?

It is done in #30997. This PR split from it to make the #30997 easier to review.

hebasto renamed this:
~~cmake: Avoid hardcoding Qt's major version in Find module~~
cmake: Avoid hardcoding Qt's major version in Find module / variable names
on Oct 2, 2024

in cmake/module/FindQt.cmake:61 in deacf3c7cd

55@@ -56,11 +56,11 @@ else()
56 endif()
57 
58 include(FindPackageHandleStandardArgs)
59-find_package_handle_standard_args(Qt5
60-  REQUIRED_VARS Qt5_DIR
61-  VERSION_VAR Qt5_VERSION
62+find_package_handle_standard_args(Qt
63+  REQUIRED_VARS Qt${Qt_FIND_VERSION_MAJOR}_DIR
64+  VERSION_VAR Qt${Qt_FIND_VERSION_MAJOR}_VERSION

l0rinc commented at 5:29 pm on October 2, 2024:

Do we need to keep the version variables here?

hebasto commented at 6:11 pm on October 2, 2024:

It logs the actual Qt version in the output:

0-- Found Qt: /usr/lib/x86_64-linux-gnu/cmake/Qt5 (found suitable version "5.15.13", minimum required is "5.11.3")

Otherwise, the output will be less informative:

0-- Found Qt: /usr/lib/x86_64-linux-gnu/cmake/Qt5 (Required is at least version "5.11.3")

in cmake/module/FindQt.cmake:44 in deacf3c7cd

39@@ -40,10 +40,10 @@ endif()
40 # /usr/x86_64-w64-mingw32/lib/libm.a or /usr/arm-linux-gnueabihf/lib/libm.a.
41 set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY BOTH)
42 
43-find_package(Qt5 ${Qt5_FIND_VERSION}
44-  COMPONENTS ${Qt5_FIND_COMPONENTS}
45+find_package(Qt${Qt_FIND_VERSION_MAJOR} ${Qt_FIND_VERSION}
46+  COMPONENTS ${Qt_FIND_COMPONENTS}

l0rinc commented at 5:29 pm on October 2, 2024:

nit: is there a reason for keeping the t lowercase? Looks weird this way…

hebasto commented at 6:06 pm on October 2, 2024:

Variables such as <PackageName>_FIND_VERSION, <PackageName>_FIND_VERSION_MAJOR, and <PackageName>_FIND_COMPONENTS, are part of the Find<PackageName> module interface. In our case, <PackageName> is replaced with Qt.

l0rinc commented at 6:14 pm on October 2, 2024:

Wow, so much magic

l0rinc commented at 5:29 pm on October 2, 2024: contributor

Concept ACK

l0rinc commented at 6:13 pm on October 2, 2024: contributor

utACK deacf3c7cd68dd4ee973526740e45c7015b6433a

laanwj commented at 10:19 am on October 3, 2024: member

Seems fine with me, we could update the Qt5 to Qt6 as well, but i imagine removing the version number saves us work when we move to Qt7 in the future 😄

promag commented at 10:28 am on October 4, 2024: member

Code review ACK deacf3c7cd68dd4ee973526740e45c7015b6433a.

maflcko commented at 7:46 am on October 7, 2024: member

lgtm ACK deacf3c7cd68dd4ee973526740e45c7015b6433a

fanquake merged this on Oct 7, 2024

fanquake closed this on Oct 7, 2024

cmake: Avoid hardcoding Qt’s major version in Find module / variable names #31010

Code Coverage

Reviews

Conflicts

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]