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.
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
build: Fix Qt processing of configure script for depends with DEBUG=1 #18298
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
fad898c
to
597445c
Compare
Updated fad898c -> 597445c (pr18298.01 -> pr18298.02, diff):
|
…osts including Windows 8a26848 build: Fix m4 escaping (Hennadii Stepanov) 9123ec1 build: Remove extra tokens warning (Hennadii Stepanov) fded4f4 build: Remove duplicated QT_STATICPLUGIN define (Hennadii Stepanov) 05a93d5 build: Fix indentation in bitcoin_qt.m4 (Hennadii Stepanov) ddbb419 build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts (Hennadii Stepanov) 492971d build: Fix mingw pkgconfig file and dependency naming (Hennadii Stepanov) Pull request description: This PR makes `bitcoin_qt.m4` to use `pkg-config` for all hosts and removes non-pkg-config paths from it. This is a step towards the idea which was clear [stated](#8314 (comment)) by Cory Fields: > I believe the consensus is to treat Windows like the others and require pkg-config across the board. We can drop all of the non-pkg-config paths, and simply AC_REQUIRE(PKG_PROG_PKG_CONFIG) There are two unsolved problems with this PR. If depends is built with `DEBUG=1` the `configure` script fails to pickup Qt: - for macOS host (similar to, but not the same as #16391) - for Windows host (regression) The fix is ~on its way~ submitted in #18298 (as a followup). Also this PR picks some small improvements from #17820. ACKs for top commit: theuni: Code review ACK 8a26848 dongcarl: Code Review ACK 8a26848 laanwj: Code review ACK 8a26848 Tree-SHA512: 3b25990934b939121983df7707997b31d61063b1207d909f539d69494c7cb85212f353092956d09ecffebb9fef28b869914dd1216a596d102fcb9744bb5487f7
597445c
to
49ab294
Compare
Rebased 597445c -> 49ab294 (pr18298.02 -> pr18298.03) due to the conflict with #18297. |
…r all hosts including Windows 8a26848 build: Fix m4 escaping (Hennadii Stepanov) 9123ec1 build: Remove extra tokens warning (Hennadii Stepanov) fded4f4 build: Remove duplicated QT_STATICPLUGIN define (Hennadii Stepanov) 05a93d5 build: Fix indentation in bitcoin_qt.m4 (Hennadii Stepanov) ddbb419 build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts (Hennadii Stepanov) 492971d build: Fix mingw pkgconfig file and dependency naming (Hennadii Stepanov) Pull request description: This PR makes `bitcoin_qt.m4` to use `pkg-config` for all hosts and removes non-pkg-config paths from it. This is a step towards the idea which was clear [stated](bitcoin#8314 (comment)) by Cory Fields: > I believe the consensus is to treat Windows like the others and require pkg-config across the board. We can drop all of the non-pkg-config paths, and simply AC_REQUIRE(PKG_PROG_PKG_CONFIG) There are two unsolved problems with this PR. If depends is built with `DEBUG=1` the `configure` script fails to pickup Qt: - for macOS host (similar to, but not the same as bitcoin#16391) - for Windows host (regression) The fix is ~on its way~ submitted in bitcoin#18298 (as a followup). Also this PR picks some small improvements from bitcoin#17820. ACKs for top commit: theuni: Code review ACK 8a26848 dongcarl: Code Review ACK 8a26848 laanwj: Code review ACK 8a26848 Tree-SHA512: 3b25990934b939121983df7707997b31d61063b1207d909f539d69494c7cb85212f353092956d09ecffebb9fef28b869914dd1216a596d102fcb9744bb5487f7
Tested 49ab294 on macOS 10.15.5; works like a charm. |
49ab294
to
e25cb1d
Compare
Rebased 49ab294 -> e25cb1d (pr18298.03 -> pr18298.04) to prevent the merge conflict with #19689. |
@hebasto Sorry for the delay. Can you drop the two already-merged commits out of here? |
e25cb1d
to
924900e
Compare
Rebased e25cb1d -> 924900e (pr18298.04 -> pr18298.05).
Done. (it was the intention of the previous rebasing, but something gone wrong) |
@hebasto taking a look |
Concept ACK |
From IRC discussion today: Concept ACK. It would be nice if qt gave the option to disable adding the suffix, but since @hebasto says no such option exists, this seems like a reasonable solution. |
Now, if depends is built with DEBUG=1, the configure script correctly finds Qt for macOS and Windows.
924900e
to
76f52e3
Compare
Rebased 924900e -> 76f52e3 (pr18298.05 -> pr18298.06) due to the conflict with #19683. |
…r all hosts including Windows 8a26848 build: Fix m4 escaping (Hennadii Stepanov) 9123ec1 build: Remove extra tokens warning (Hennadii Stepanov) fded4f4 build: Remove duplicated QT_STATICPLUGIN define (Hennadii Stepanov) 05a93d5 build: Fix indentation in bitcoin_qt.m4 (Hennadii Stepanov) ddbb419 build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts (Hennadii Stepanov) 492971d build: Fix mingw pkgconfig file and dependency naming (Hennadii Stepanov) Pull request description: This PR makes `bitcoin_qt.m4` to use `pkg-config` for all hosts and removes non-pkg-config paths from it. This is a step towards the idea which was clear [stated](bitcoin#8314 (comment)) by Cory Fields: > I believe the consensus is to treat Windows like the others and require pkg-config across the board. We can drop all of the non-pkg-config paths, and simply AC_REQUIRE(PKG_PROG_PKG_CONFIG) There are two unsolved problems with this PR. If depends is built with `DEBUG=1` the `configure` script fails to pickup Qt: - for macOS host (similar to, but not the same as bitcoin#16391) - for Windows host (regression) The fix is ~on its way~ submitted in bitcoin#18298 (as a followup). Also this PR picks some small improvements from bitcoin#17820. ACKs for top commit: theuni: Code review ACK 8a26848 dongcarl: Code Review ACK 8a26848 laanwj: Code review ACK 8a26848 Tree-SHA512: 3b25990934b939121983df7707997b31d61063b1207d909f539d69494c7cb85212f353092956d09ecffebb9fef28b869914dd1216a596d102fcb9744bb5487f7
… depends with DEBUG=1 Now, if depends is built with DEBUG=1, the configure script correctly finds Qt for macOS and Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 76f52e3. Tested native darwin, and darwin/win cross compile with DEBUG=1
.
…r all hosts including Windows 8a26848 build: Fix m4 escaping (Hennadii Stepanov) 9123ec1 build: Remove extra tokens warning (Hennadii Stepanov) fded4f4 build: Remove duplicated QT_STATICPLUGIN define (Hennadii Stepanov) 05a93d5 build: Fix indentation in bitcoin_qt.m4 (Hennadii Stepanov) ddbb419 build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts (Hennadii Stepanov) 492971d build: Fix mingw pkgconfig file and dependency naming (Hennadii Stepanov) Pull request description: This PR makes `bitcoin_qt.m4` to use `pkg-config` for all hosts and removes non-pkg-config paths from it. This is a step towards the idea which was clear [stated](bitcoin#8314 (comment)) by Cory Fields: > I believe the consensus is to treat Windows like the others and require pkg-config across the board. We can drop all of the non-pkg-config paths, and simply AC_REQUIRE(PKG_PROG_PKG_CONFIG) There are two unsolved problems with this PR. If depends is built with `DEBUG=1` the `configure` script fails to pickup Qt: - for macOS host (similar to, but not the same as bitcoin#16391) - for Windows host (regression) The fix is ~on its way~ submitted in bitcoin#18298 (as a followup). Also this PR picks some small improvements from bitcoin#17820. ACKs for top commit: theuni: Code review ACK 8a26848 dongcarl: Code Review ACK 8a26848 laanwj: Code review ACK 8a26848 Tree-SHA512: 3b25990934b939121983df7707997b31d61063b1207d909f539d69494c7cb85212f353092956d09ecffebb9fef28b869914dd1216a596d102fcb9744bb5487f7
… depends with DEBUG=1 Now, if depends is built with DEBUG=1, the configure script correctly finds Qt for macOS and Windows.
… depends with DEBUG=1 Now, if depends is built with DEBUG=1, the configure script correctly finds Qt for macOS and Windows.
This PR:
configure
script correctly pickup Qt if depends is built withDEBUG=1
: