build: Getting ready to Qt 6 (4/n). Improve build-aux/m4/bitcoin_qt.m4 #24813

pull hebasto wants to merge 4 commits into bitcoin:master from hebasto:220409-appcheck changing 2 files +89 −51
  1. hebasto commented at 1:10 pm on April 9, 2022: member

    This PR adds a test macro _BITCOIN_QT_CHECK_APP, which ensures that Qt resource system object (*.rcc) have being compiled and linked properly. Found it such checks very useful while working on bitcoin/bitcoin#24798.

    Here are examples from configure logs:

    • successful check:
     0...
 1checking for Qt5Core >= 5.11.3... yes
 2checking for QCoreApplication initialization... yes
 3checking for Qt5Gui >= 5.11.3... yes
 4checking for QGuiApplication initialization... yes
 5checking for Qt5Widgets >= 5.11.3... yes
 6checking for QApplication initialization... yes
 7checking for Qt5Network >= 5.11.3... yes
 8checking for Qt5Test >= 5.11.3... yes
 9checking for Qt5DBus >= 5.11.3... yes
10...
    • failed check:
    0...
1checking for QCoreApplication initialization... no
2configure: WARNING: QCoreApplication failed to initialize.; bitcoin-qt frontend will not be built
3checking whether to build Bitcoin Core GUI... no
4...

    The fourth commit may fix build in some weird setups, but it is definitely required for Qt 6.

  2. hebasto added the label Build system on Apr 9, 2022
  3. build: Check Qt for PIE/PIC earlier 
    This is a prerequisite for the following commit.

A move-only commit, use `diff --color-moved=dimmed-zebra` to verify.
    9379714c9a
  4. build, qt: Add `_BITCOIN_QT_CHECK_APP` macro 
    This change increases robustness of the build system. A passed test
guarantees that the required Qt resource system object have being linked
properly.
    2cb8319f62
  5. hebasto force-pushed on Apr 9, 2022
  6. DrahtBot commented at 10:52 pm on April 9, 2022: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #21570 (build, qt: Simplifies checks for -fPIE and -fPIC compatibility 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.

  7. in build-aux/m4/bitcoin_qt.m4:321 in b3a6ca23d3 outdated 
    319+dnl ------------------------------------------------------------
320 dnl
321 dnl Requires: INCLUDES and LIBS must be populated as necessary.
322 dnl Inputs: $1: A static plugin name.
323-dnl Inputs: $2: The libraries that resolve $1.
324+dnl Inputs: $2: The library that resolve $1.
    dunxen commented at 9:14 am on April 13, 2022:

    nit: grammar should change

    0dnl Inputs: $2: The library that resolves $1.
    hebasto commented at 10:33 am on April 13, 2022:
    Thanks! Updated.
  8. dunxen commented at 10:21 am on April 13, 2022: contributor

    tACK b3a6ca2

    Confirmed that the new checks are successfully run with configure.

  9. build, qt, doc: Improve `_BITCOIN_QT_CHECK_STATIC_PLUGIN` description 
    Due to appending `qt_lib_suffix`, only one library can be passed to
the `_BITCOIN_QT_CHECK_STATIC_PLUGIN` macro.
    44d423549d
  10. build: Pass `qt_lib_path` from depends to the `configure` script 
    This change makes behavior consistent among `with_qt_*` variables, and
it is required for Qt 6.
    43af01e23b
  11. hebasto force-pushed on Apr 13, 2022
  12. hebasto commented at 10:32 am on April 13, 2022: member

    Updated b3a6ca23d3e55a81bee573b92e3946a284f05761 -> 43af01e23b5e7e5afba2da2863aafade6f8ad752 (pr24813.02 -> pr24813.03, diff):

  13. dunxen commented at 10:34 am on April 13, 2022: contributor
    reACK 43af01e
  14. fanquake commented at 10:42 am on April 20, 2022: member

    which ensures that Qt resource system object (*.rcc) have being compiled and linked properly. Found it such checks very useful while working on #24798.

    Can you elaborate on when a user or developer would actually run into the scenario of these resources not being linked properly? I’m not against adding additional tests that might be useful to others, but if these are checks that were only helpful in your dev environment while things were broken / you were debugging, then it’s less clear they should be added to configure.

  15. hebasto commented at 10:47 am on April 20, 2022: member

    which ensures that Qt resource system object (*.rcc) have being compiled and linked properly. Found it such checks very useful while working on #24798.

    Can you elaborate on when a user or developer would actually run into the scenario of these resources not being linked properly? I’m not against adding additional tests that might be useful to others, but if these are checks that were only helpful in your dev environment while things were broken / you were debugging, then it’s less clear they should be added to configure.

    For example, in 9f61d6a7605eee4b116b8791276bd71baf75bf10 forget to include ${qt_lib_path}/objects-Release/Widgets_resources_1/.rcc/qrc_qstyle.cpp.o

  16. fanquake commented at 10:54 am on April 20, 2022: member

    For example, in https://github.com/bitcoin/bitcoin/commit/9f61d6a7605eee4b116b8791276bd71baf75bf10 forget to include ${qt_lib_path}/objects-Release/Widgets_resources_1/.rcc/qrc_qstyle.cpp.o

    In that case, concept ~0 for the new checks. They seem to be a symptom of other hacks (trying to link against specific .o files directly in configure checks? I’ll comment in #24798), which I would hope have a better solution, and this isn’t really guarding against a situation that someone is going to run into in the wild.

  17. hebasto commented at 11:10 am on April 20, 2022: member

    … which I would hope have a better solution

    Me too :)

    Not talking about a “better” solution, but here are some alternative ones:

    • wait for Qt6 patch for *.pc files which works with static builds
    • develop our own Qt6 patch for *.pc files which works with static builds
    • use CMake for Qt6 builds
  18. fanquake commented at 8:30 am on April 29, 2022: member

    Not talking about a “better” solution, but here are some alternative ones:

    That discussion might be better saved for #24798. I’m a Concept NACK here. I don’t think these tests should be added, as they are guarding against situations that, from what I understand, can only could occur if we introduce the fairly fragile workarounds from #24798 into our code (it would have been handy if this was outlined in the PR description), which I don’t think we should do.

  19. fanquake commented at 11:04 am on August 10, 2022: member
    I think this should be closed / combined into #24798 for now. In any case, I don’t think this is the approach (linking/testing object files) we would want to use in our build system to add Qt 6 support.
  20. hebasto commented at 1:09 pm on August 10, 2022: member

    I think this should be closed / combined into #24798 for now.

    Agree.

    In any case, I don’t think this is the approach (linking/testing object files) we would want to use in our build system to add Qt 6 support.

    Agree again. Moving to CMake-based build system fixes it :tiger2:

  21. hebasto closed this on Aug 10, 2022
  22. bitcoin locked this on Aug 10, 2023


hebasto DrahtBot dunxen fanquake

Labels
Build system

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-09-29 01:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me