Conversation
The `find_package(Qt .. MODULE REQUIRED COMPONENTS ...)` call must treat any missing component as a fatal error.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32709. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste |
Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use] |
This change cleans up the user-facing part of the CMake cache following the migration to Qt 6.
2fa8e44 to
89d3daf
Compare
| mark_as_advanced( | ||
| QT_ADDITIONAL_HOST_PACKAGES_PREFIX_PATH | ||
| QT_ADDITIONAL_PACKAGES_PREFIX_PATH | ||
| XKB_INCLUDE_DIR |
There was a problem hiding this comment.
They are created by Qt 6 configuration scripts internally.
Consider the following:
$ rm -rf build && cmake -B build --log-level=ERROR -L | tail -n +5 > /tmp/master_no_gui
$ rm -rf build && cmake -B build -DBUILD_GUI=ON --log-level=ERROR -L | tail -n +5 > /tmp/master_gui
$ diff -u /tmp/master_no_gui /tmp/master_gui
--- /tmp/master_no_gui 2025-06-16 14:47:31.494243561 +0100
+++ /tmp/master_gui 2025-06-16 14:48:25.931426493 +0100
@@ -8,7 +8,8 @@
BUILD_DAEMON:BOOL=ON
BUILD_FOR_FUZZING:BOOL=OFF
BUILD_FUZZ_BINARY:BOOL=OFF
-BUILD_GUI:BOOL=OFF
+BUILD_GUI:BOOL=ON
+BUILD_GUI_TESTS:BOOL=ON
BUILD_KERNEL_LIB:BOOL=OFF
BUILD_SHARED_LIBS:BOOL=ON
BUILD_TESTS:BOOL=ON
@@ -22,6 +23,12 @@
ENABLE_IPC:BOOL=OFF
ENABLE_WALLET:BOOL=ON
INSTALL_MAN:BOOL=ON
+QT_ADDITIONAL_HOST_PACKAGES_PREFIX_PATH:STRING=
+QT_ADDITIONAL_PACKAGES_PREFIX_PATH:STRING=
+Qt6CoreTools_DIR:PATH=/usr/lib/x86_64-linux-gnu/cmake/Qt6CoreTools
+Qt6DBusTools_DIR:PATH=/usr/lib/x86_64-linux-gnu/cmake/Qt6DBusTools
+Qt6GuiTools_DIR:PATH=/usr/lib/x86_64-linux-gnu/cmake/Qt6GuiTools
+Qt6WidgetsTools_DIR:PATH=/usr/lib/x86_64-linux-gnu/cmake/Qt6WidgetsTools
REDUCE_EXPORTS:BOOL=OFF
SECP256K1_APPEND_CFLAGS:STRING=
SECP256K1_APPEND_LDFLAGS:STRING=
@@ -44,7 +51,13 @@
SECP256K1_INSTALL:BOOL=OFF
SECP256K1_USE_EXTERNAL_DEFAULT_CALLBACKS:BOOL=OFF
SECP256K1_VALGRIND:STRING=AUTO
+SED_EXECUTABLE:FILEPATH=/usr/bin/sed
WERROR:BOOL=OFF
WITH_CCACHE:BOOL=ON
+WITH_DBUS:BOOL=ON
+WITH_QRENCODE:BOOL=ON
WITH_USDT:BOOL=OFF
WITH_ZMQ:BOOL=OFF
+XGETTEXT_EXECUTABLE:FILEPATH=/usr/bin/xgettext
+XKB_INCLUDE_DIR:PATH=XKB_INCLUDE_DIR-NOTFOUND
+XKB_LIBRARY:FILEPATH=XKB_LIBRARY-NOTFOUNDThere was a problem hiding this comment.
If they are internal, why do we need to do anything? It seems odd that we'd need to do something specifically, for this single Qt dependency. This doesn't really explain why this is needed.
There was a problem hiding this comment.
CMake’s normal (non-advanced) cache variables are intended for users to override values set by the build system. I believe we should keep the list of such variables concise, including only those that may be genuinely useful to users.
There was a problem hiding this comment.
Ok. So then this looks like a bug in Qt.
There was a problem hiding this comment.
To be clear, I think these changes could be made, with the XKB_ change dropped. If that's going to be kept, then there should be a link to an upstream issue.
There was a problem hiding this comment.
I don't think upstream considers this as an issue. Whether those variables should be considered advanced or not is a matter of taste.
There was a problem hiding this comment.
If anything, that would seem inconsistent, because they have marked every other dependency as advanced, and there's no explanation for why XKB hasn't been marked as such (my guess is just oversight).
There was a problem hiding this comment.
Is this going to be addressed / reported upstream? I'd rather not undocumented work arounds, for suspected upstream issues (given that they mark all other dependencies as advanced), just to fix the output of a GUI tool.
|
ACK 89d3daf |
BrandonOdiwuor
left a comment
There was a problem hiding this comment.
Tested ACK 89d3daf
System: macOS 15.6
Confirmed that with this change find_package(Qt .. MODULE REQUIRED COMPONENTS ...) now fails with a fatal error when components are missing (whereas master only issued a warning), and verified that the following variables are no longer exposed in the default CMake cache: QT_ADDITIONAL_HOST_PACKAGES_PR, QT_ADDITIONAL_PACKAGES_PREFIX_ , Qt6CoreTools_DIR, Qt6GuiTools_DIR, Qt6WidgetsTools_DIR
<commit: 89d3daf>
<branch: master>
|
Approach ACK Did a guix build as an additional regression check. Agree with #32709 (comment) though that this should be reported upstream. |
|
Given that half of this is now in #34578, what are we doing with the remaining commit? |
0a6724a doc: Update Windows build notes (Hennadii Stepanov) 473e5f8 qt: Add patch to fix SFINAE warnings in QAnyStringView with gcc16 (Hennadii Stepanov) 3cb4d60 qt: add patches to fix SFINAE errors/warnings with gcc16 (Cory Fields) d7e972a qt: add patch to fix build with gcc16 (Cory Fields) 19693a8 depends: Update Qt to 6.8.3 (Hennadii Stepanov) c555845 cmake: Fix `FindQt` module (Hennadii Stepanov) Pull request description: This PR updates the `qt` package in depends to the latest open-source [6.8.3](https://www.qt.io/blog/qt-6.8.3-released) LTS release. The update includes numerous bugfixes, which allows us to drop `qtbase_plugins_windows11style.patch`. Additionally, it includes [patches](#34569 (comment)) for compatibility with GCC 16 (along with one extra patch), and incorporates a [commit](8f1b55d) from #32709. Closes #34569. ACKs for top commit: achow101: ACK 0a6724a sedited: ACK 0a6724a Tree-SHA512: b66fe6f75bae00fb5c525c5fad56d39273f53f6bfd58206da8a55c6e41d14533137c72fb03e9537ba3a3d0b3463b6dcbef6a88ac2f4559fa6e9abf045fe1beaa
This PR improves the
FindQtmodule in two ways:Ensures that the
find_package(Qt .. MODULE REQUIRED COMPONENTS ...)call treats any missing component as a fatal error. In the master branch, only a warning is currently issued.Cleans up the user-facing part of the CMake cache following the migration to Qt 6. An exception (
Qt6_DIR) is documented.