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

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:241001-findqt changing 2 files +13 −13
  1. 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.

  2. cmake: Avoid hardcoding Qt's major version in Find module
    This change facilitates future migration to Qt 6.
    deacf3c7cd
  3. hebasto added the label Build system on Oct 1, 2024
  4. 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.

  5. maflcko added the label DrahtBot Guix build requested on Oct 1, 2024
  6. 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...
  7. DrahtBot removed the label DrahtBot Guix build requested on Oct 1, 2024
  8. hebasto added the label Refactoring on Oct 2, 2024
  9. 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.

  10. 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
  11. 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")
    
  12. 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
  13. l0rinc commented at 5:29 pm on October 2, 2024: contributor
    Concept ACK
  14. l0rinc commented at 6:13 pm on October 2, 2024: contributor
    utACK deacf3c7cd68dd4ee973526740e45c7015b6433a
  15. 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 😄
  16. promag commented at 10:28 am on October 4, 2024: member
    Code review ACK deacf3c7cd68dd4ee973526740e45c7015b6433a.
  17. maflcko commented at 7:46 am on October 7, 2024: member
    lgtm ACK deacf3c7cd68dd4ee973526740e45c7015b6433a
  18. fanquake merged this on Oct 7, 2024
  19. fanquake closed this on Oct 7, 2024


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me