Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Apr 9, 2022 •
edited
Loading

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 #24798.

Here are examples from configure logs:

  • successful check:
...
checking for Qt5Core >= 5.11.3... yes
checking for QCoreApplication initialization... yes
checking for Qt5Gui >= 5.11.3... yes
checking for QGuiApplication initialization... yes
checking for Qt5Widgets >= 5.11.3... yes
checking for QApplication initialization... yes
checking for Qt5Network >= 5.11.3... yes
checking for Qt5Test >= 5.11.3... yes
checking for Qt5DBus >= 5.11.3... yes
...
  • failed check:
...
checking for QCoreApplication initialization... no
configure: WARNING: QCoreApplication failed to initialize.; bitcoin-qt frontend will not be built
checking whether to build Bitcoin Core GUI... no
...

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

hebasto added 2 commits April 9, 2022 17:27
@hebasto
build: Check Qt for PIE/PIC earlier
9379714 
This is a prerequisite for the following commit.

A move-only commit, use `diff --color-moved=dimmed-zebra` to verify.
@hebasto
build, qt: Add _BITCOIN_QT_CHECK_APP macro
2cb8319 
This change increases robustness of the build system. A passed test
guarantees that the required Qt resource system object have being linked
properly.
@hebasto hebasto mentioned this pull request Apr 9, 2022
Closed
5 tasks
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 9, 2022 •
edited
Loading

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.

dunxen
dunxen reviewed Apr 13, 2022
View reviewed changes
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK b3a6ca2

Confirmed that the new checks are successfully run with configure.

hebasto added 2 commits April 13, 2022 12:28
@hebasto
build, qt, doc: Improve _BITCOIN_QT_CHECK_STATIC_PLUGIN description
44d4235 
Due to appending `qt_lib_suffix`, only one library can be passed to
the `_BITCOIN_QT_CHECK_STATIC_PLUGIN` macro.
@hebasto
build: Pass qt_lib_path from depends to the configure script
43af01e 
This change makes behavior consistent among `with_qt_*` variables, and
it is required for Qt 6.
@hebasto
Copy link
Member Author

hebasto commented Apr 13, 2022

Updated b3a6ca2 -> 43af01e (pr24813.02 -> pr24813.03, diff):

@dunxen
Copy link
Contributor

dunxen commented Apr 13, 2022

reACK 43af01e

@DrahtBot DrahtBot mentioned this pull request Apr 17, 2022
Closed
@fanquake
Copy link
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.

@hebasto
Copy link
Member Author

hebasto commented Apr 20, 2022

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 9f61d6a forget to include ${qt_lib_path}/objects-Release/Widgets_resources_1/.rcc/qrc_qstyle.cpp.o

@fanquake
Copy link
Member

For example, in 9f61d6a 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.

@hebasto
Copy link
Member Author

hebasto commented Apr 20, 2022

... 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

@fanquake
Copy link
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.

@DrahtBot DrahtBot mentioned this pull request May 23, 2022
Closed
@fanquake
Copy link
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.

@hebasto
Copy link
Member Author

hebasto commented Aug 10, 2022

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 🐅

@hebasto hebasto closed this Aug 10, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Aug 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@dunxen dunxen dunxen left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hebasto @DrahtBot @dunxen @fanquake