build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts including Windows #18297

hebasto · 2020-03-09T02:00:29Z

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 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 build: configure doesn't pickup Qt if depends is built with DEBUG=1 #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.

This change adds the correct suffix to debug mode .pc filenames for MinGW and also to the Qt libraries listed in the `Requires` field. The filename adjustment fixes the accidental overwriting of release mode .pc files with the debug mode variant which required the wrong variant of the libraries when `debug_and_release` is active. Note that macOS also supports the `debug_and_release' configuration but may use the regular library names together with DYLD_IMAGE_SUFFIX. Creation of *_debug.pc files is turned off as they're identical to their non-debug counterparts. More info: - QTBUG-4155 - Qt commit a0d8fb4ac3cb7bafdb39f340055eacee4f957513

hebasto · 2020-03-09T02:05:23Z

May I ask for gitian builds?

DrahtBot · 2020-03-09T05:30:21Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

build: optionally skip external warnings #18750 (build: optionally skip external warnings by vasild)

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.

hebasto · 2020-03-09T09:19:00Z

Updated f71b560 -> e7cfcf7 (pr18297.01 -> pr18297.02, compare):

removed redundant include and lib tracking for MinGW in config.site

This change adds to the BITCOIN_QT_CONFIGURE script ability to use pkg-config for MinGW. All of the non-pkg-config paths are removed as needless. If depends is built with DEBUG=1 the configure script fails to pickup Qt: - for macOS host (similar, but not the same as issue 16391) - for Windows host (regression)

QT_STATICPLUGIN is defined in BITCOIN_QT_CONFIGURE macro.

hebasto · 2020-03-16T09:14:55Z

Updated e7cfcf7 -> 8a26848 (pr18297.02 -> pr18297.03, diff):

removed unused now internal functions _BITCOIN_QT_CHECK_QT5 and _BITCOIN_QT_CHECK_QT58

hebasto · 2020-03-24T13:54:49Z

Just noted that some dead code remains:

bitcoin/build-aux/m4/bitcoin_qt.m4

Lines 299 to 301 in 8a26848

    
                   if test x$bitcoin_cv_qt58 = xno; then 
        
                     PKG_CHECK_MODULES([QTPLATFORM], [Qt5PlatformSupport], [QT_LIBS="$QTPLATFORM_LIBS $QT_LIBS"]) 
        
                   else

The bitcoin_cv_qt58 variable is always unset (see #17821).

This code path runs for static Qt builds, including gitian builds. On xenial with Qt 5.5.1 (the minimal supported version) builds against system libs are ok (tested locally + Travis).

Going to remove this dead code in a followup or with other changes requested by reviewers.

laanwj · 2020-06-04T14:13:49Z

Code review ACK 8a26848

laanwj · 2020-06-04T14:15:25Z

This makes significant changes to the build system and introduces a Qt patch. @dongcarl @theuni can you take a look at this plase?

theuni · 2020-06-05T21:53:43Z

@laanwj Thanks for the ping, will review thoroughly in the next few days.

DrahtBot · 2020-06-07T06:28:44Z

Gitian builds

File	commit `17cfa52` (master)	commit `61fe2c6` (master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`1c72e8e4fc35859a...`	`89515a9227643763...`
*-aarch64-linux-gnu.tar.gz	`b30e3199e8acfa25...`	`4fc7b7ba50988d45...`
*-arm-linux-gnueabihf-debug.tar.gz	`8e836573afc2d1ae...`	`a508ed33f3e55610...`
*-arm-linux-gnueabihf.tar.gz	`be966fe2599f3c9f...`	`b2c1523cdf385c79...`
*-osx-unsigned.dmg	`db0084dc80753606...`	`b48ee8f744c28379...`
*-osx64.tar.gz	`825419899dbec827...`	`a1da373d7ed6caac...`
*-riscv64-linux-gnu-debug.tar.gz	`becb75bee8ee9eb5...`	`67eb20fb8eac295e...`
*-riscv64-linux-gnu.tar.gz	`2de2cde85b0c26bd...`	`3c230ab7cab5656a...`
*-win64-debug.zip	`d7b65f642b3fb503...`	`bcbc103170e4f8d2...`
*-win64-setup-unsigned.exe	`9810912dcfaa9c1a...`	`13ebeaa3b21eac35...`
*-win64.zip	`adb8a6aa49116574...`	`cee1bbe4ceab905e...`
*-x86_64-linux-gnu-debug.tar.gz	`fdd050cd554445f4...`	`03a6912ab8474bfa...`
*-x86_64-linux-gnu.tar.gz	`70a422b24c1d11a2...`	`1ac74df3aa638f7f...`
*.tar.gz	`7ea084c8d5c43474...`	`0c762d096a48f3e4...`
bitcoin-core-linux-0.21-res.yml	`bf589606c87f1ef8...`	`6ba31c52f2426198...`
bitcoin-core-osx-0.21-res.yml	`d882ce2c1d471957...`	`a805b41ee34a7e27...`
bitcoin-core-win-0.21-res.yml	`e3b7dec01dc025d2...`	`6760ee0762cd4a74...`
linux-build.log	`18119261df2a62e9...`	`ad266b3f1d65acc8...`
osx-build.log	`86f016a18d174458...`	`b27a527d34c3350d...`
win-build.log	`a256aaeed3dd26c2...`	`d6ae318cec88d163...`
bitcoin-core-linux-0.21-res.yml.diff		`44023913fc31ddeb...`
bitcoin-core-osx-0.21-res.yml.diff		`98f1b79aa8eb8eac...`
bitcoin-core-win-0.21-res.yml.diff		`6078c048e92d93dc...`
linux-build.log.diff		`fc46cecc6125f163...`
osx-build.log.diff		`d6a15b82e2abf05e...`
win-build.log.diff		`e7076213c81ffb67...`

dongcarl

Code Review ACK 8a26848

dongcarl · 2020-06-12T17:15:08Z

depends/patches/qt/fix_qt_pkgconfig.patch

 -!internal_module:!lib_bundle:if(unix|mingw) {
-+unix|mingw {


Do you or @jonasschnelli or @theuni understand why this original patch was necessary?

@dongcarl I don't recall, but from a quick glance, it looks like it was necessary to get qt to generate pkg-config files for internal/bundled libs (png/harfbuzz/pcre, etc).

theuni

Code review ACK 8a26848

…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

92bc268 build: Detect missed pkg-config early (Hennadii Stepanov) 1739eb2 build: Drop unused use_pkgconfig variable (Hennadii Stepanov) a661449 build: Drop use_pkgconfig check for libmultiprocess check (Hennadii Stepanov) 90b95e7 build: Drop dead non-pkg-config code for libevent check (Hennadii Stepanov) 44a14af build: Drop dead non-pkg-config code for qrencode check (Hennadii Stepanov) 10cbae0 build: Drop dead non-pkg-config code for ZMQ check (Hennadii Stepanov) 06cfc9c build: Fix indentation in UNIVALUE check (Hennadii Stepanov) 6fd2118 build: Drop dead non-pkg-config code for UNIVALUE check (Hennadii Stepanov) e9edbe4 build: Always use pkg-config (Hennadii Stepanov) 9e2e753 build: Always define ZMQ_STATIC for MinGW (Hennadii Stepanov) Pull request description: This PR: - is based on #18297 (already merged) - drops all of the non-pkg-config paths from the `configure` script Ref: #17768 ACKs for top commit: fanquake: ACK 92bc268. I re-gitian-built. There are a couple follow-ups that I'll PR shortly. Thanks for addressing my feedback above. I took too long to get back to this. laanwj: ACK 92bc268 Tree-SHA512: 83c2d9cf03518867a1ebf7e26a8fc5b6dd8962ef983fe0d84e0c7eb74717f4c36a834da02faf0e503ffd87167005351671cf040c0d4ddae57ee152a6ff84012b

4af4672 build, qt: Add Qt version checking (Hennadii Stepanov) 30e336f build: Drop unused bitcoin_cv_qt58 (Hennadii Stepanov) Pull request description: Now `configure` script checks that Qt version is not less then minimum required (currently [5.5.1](#15393)). This PR is an alternative to #15706 (see #15706 (comment)). Closes #15688. The first commit removes dead code (see #18297 (comment)). ACKs for top commit: fanquake: ACK 4af4672 - this looks ok. I've tested this with Qt 5.15.0 and Qt 5.7.1 system libs, as well as 5.9.8 from depends. Tree-SHA512: 8e3b82fa3a98926814923331038185633fabad962c271f31bd158e1ab293dcde52ab1dbf997745540a9ed27e16835cf5b5f3701d405876d877fa561eb03cc619

4af4672 build, qt: Add Qt version checking (Hennadii Stepanov) 30e336f build: Drop unused bitcoin_cv_qt58 (Hennadii Stepanov) Pull request description: Now `configure` script checks that Qt version is not less then minimum required (currently [5.5.1](bitcoin#15393)). This PR is an alternative to bitcoin#15706 (see bitcoin#15706 (comment)). Closes bitcoin#15688. The first commit removes dead code (see bitcoin#18297 (comment)). ACKs for top commit: fanquake: ACK 4af4672 - this looks ok. I've tested this with Qt 5.15.0 and Qt 5.7.1 system libs, as well as 5.9.8 from depends. Tree-SHA512: 8e3b82fa3a98926814923331038185633fabad962c271f31bd158e1ab293dcde52ab1dbf997745540a9ed27e16835cf5b5f3701d405876d877fa561eb03cc619

…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

4af4672 build, qt: Add Qt version checking (Hennadii Stepanov) 30e336f build: Drop unused bitcoin_cv_qt58 (Hennadii Stepanov) Pull request description: Now `configure` script checks that Qt version is not less then minimum required (currently [5.5.1](bitcoin#15393)). This PR is an alternative to bitcoin#15706 (see bitcoin#15706 (comment)). Closes bitcoin#15688. The first commit removes dead code (see bitcoin#18297 (comment)). ACKs for top commit: fanquake: ACK 4af4672 - this looks ok. I've tested this with Qt 5.15.0 and Qt 5.7.1 system libs, as well as 5.9.8 from depends. Tree-SHA512: 8e3b82fa3a98926814923331038185633fabad962c271f31bd158e1ab293dcde52ab1dbf997745540a9ed27e16835cf5b5f3701d405876d877fa561eb03cc619

…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

4af4672 build, qt: Add Qt version checking (Hennadii Stepanov) 30e336f build: Drop unused bitcoin_cv_qt58 (Hennadii Stepanov) Pull request description: Now `configure` script checks that Qt version is not less then minimum required (currently [5.5.1](bitcoin#15393)). This PR is an alternative to bitcoin#15706 (see bitcoin#15706 (comment)). Closes bitcoin#15688. The first commit removes dead code (see bitcoin#18297 (comment)). ACKs for top commit: fanquake: ACK 4af4672 - this looks ok. I've tested this with Qt 5.15.0 and Qt 5.7.1 system libs, as well as 5.9.8 from depends. Tree-SHA512: 8e3b82fa3a98926814923331038185633fabad962c271f31bd158e1ab293dcde52ab1dbf997745540a9ed27e16835cf5b5f3701d405876d877fa561eb03cc619

fanquake added the Build system label Mar 9, 2020

fanquake added the Needs gitian build label Mar 9, 2020

This was referenced Mar 9, 2020

build: add --enable-isystem and change --enable-werror to enable -Werror #18149

Closed

build: bitcoin_qt.m4 fixes and improvements #17820

Closed

hebasto mentioned this pull request Mar 9, 2020

build: various build system improvements #17768

Closed

9 tasks

hebasto force-pushed the 20200308-pkgconfig-mingw branch from f71b560 to e7cfcf7 Compare March 9, 2020 09:14

This was referenced Mar 9, 2020

WIP: Qt: add QML based mobile GUI #16883

Closed

build: Check QT library version #15706

Closed

This was referenced Mar 9, 2020

build: Fix Qt processing of configure script for depends with DEBUG=1 #18298

Merged

build: Require pkg-config for all of the hosts #18307

Merged

This was referenced Mar 10, 2020

build: Optionally enable -Wzero-as-null-pointer-constant #15112

Closed

Build: enable -Wdocumentation via isystem #14920

Closed

DrahtBot removed the Needs gitian build label Mar 11, 2020

hebasto added 5 commits March 16, 2020 11:08

build: Fix indentation in bitcoin_qt.m4

05a93d5

build: Remove duplicated QT_STATICPLUGIN define

fded4f4

QT_STATICPLUGIN is defined in BITCOIN_QT_CONFIGURE macro.

build: Remove extra tokens warning

9123ec1

build: Fix m4 escaping

8a26848

hebasto force-pushed the 20200308-pkgconfig-mingw branch from e7cfcf7 to 8a26848 Compare March 16, 2020 09:12

hebasto mentioned this pull request Mar 16, 2020

build: Make the help string for --with-gui configure option unambiguous #18361

Closed

ryanofsky mentioned this pull request Mar 16, 2020

build: Ambiguous help string for --with-gui configure option #17813

Closed

bitcoin deleted a comment from DrahtBot Mar 19, 2020

maflcko added the Needs gitian build label Mar 19, 2020

DrahtBot removed the Needs gitian build label Mar 20, 2020

DrahtBot mentioned this pull request Apr 23, 2020

build: optionally skip external warnings #18750

Merged

bitcoin deleted a comment from DrahtBot Jun 4, 2020

maflcko added Needs gitian build DrahtBot Guix build requested labels Jun 4, 2020

DrahtBot removed the Needs gitian build label Jun 7, 2020

dongcarl reviewed Jun 12, 2020

View reviewed changes

theuni approved these changes Jun 12, 2020

View reviewed changes

fanquake merged commit 2654927 into bitcoin:master Jun 13, 2020

hebasto deleted the 20200308-pkgconfig-mingw branch June 13, 2020 08:20

hebasto mentioned this pull request Jun 13, 2020

build: mingw32 issues with Qt if depends is built with DEBUG=1 #19266

Closed

maflcko removed the DrahtBot Guix build requested label Jul 29, 2020

hebasto mentioned this pull request Aug 9, 2020

build: Add Qt version checking #19689

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts including Windows #18297

build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts including Windows #18297

Uh oh!

hebasto commented Mar 9, 2020 •

edited

Loading

Uh oh!

hebasto commented Mar 9, 2020

Uh oh!

DrahtBot commented Mar 9, 2020 •

edited

Loading

Uh oh!

hebasto commented Mar 9, 2020

Uh oh!

hebasto commented Mar 16, 2020

Uh oh!

hebasto commented Mar 24, 2020

Uh oh!

laanwj commented Jun 4, 2020

Uh oh!

laanwj commented Jun 4, 2020

Uh oh!

theuni commented Jun 5, 2020

Uh oh!

DrahtBot commented Jun 7, 2020

Uh oh!

dongcarl left a comment

Uh oh!

dongcarl Jun 12, 2020

Uh oh!

theuni Jun 12, 2020

Uh oh!

theuni left a comment

Uh oh!

Uh oh!

		-!internal_module:!lib_bundle:if(unix\|mingw) {
		+unix\|mingw {

build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts including Windows #18297

build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts including Windows #18297

Uh oh!

Conversation

hebasto commented Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Mar 9, 2020

Uh oh!

DrahtBot commented Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Mar 9, 2020

Uh oh!

hebasto commented Mar 16, 2020

Uh oh!

hebasto commented Mar 24, 2020

Uh oh!

laanwj commented Jun 4, 2020

Uh oh!

laanwj commented Jun 4, 2020

Uh oh!

theuni commented Jun 5, 2020

Uh oh!

DrahtBot commented Jun 7, 2020

Gitian builds

Uh oh!

dongcarl left a comment

Choose a reason for hiding this comment

Uh oh!

dongcarl Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

theuni Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Mar 9, 2020 •

edited

Loading

DrahtBot commented Mar 9, 2020 •

edited

Loading