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

pull hebasto wants to merge 6 commits into bitcoin:master from hebasto:20200308-pkgconfig-mingw changing 4 files +44 −217

hebasto commented at 2:00 AM on March 9, 2020: member
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 #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.

build: Fix mingw pkgconfig file and dependency naming

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

492971de35

fanquake added the label Build system on Mar 9, 2020
hebasto commented at 2:05 AM on March 9, 2020: member

May I ask for gitian builds?
fanquake added the label Needs gitian build on Mar 9, 2020
DrahtBot commented at 5:30 AM on March 9, 2020: 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:
- #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.
DrahtBot cross-referenced this on Mar 9, 2020 from issue build: add --enable-isystem and change --enable-werror to enable -Werror by vasild
DrahtBot cross-referenced this on Mar 9, 2020 from issue build: bitcoin_qt.m4 fixes and improvements by hebasto
hebasto cross-referenced this on Mar 9, 2020 from issue build: various build system improvements by fanquake
hebasto force-pushed on Mar 9, 2020
hebasto commented at 9:19 AM on March 9, 2020: member
Updated f71b56047bc319184c1f771e0757faeff26cf242 -> e7cfcf79ea9992e2b844ffdcd64ee02af4f9fc07 (pr18297.01 -> pr18297.02, compare):
- removed redundant include and lib tracking for MinGW in config.site
DrahtBot cross-referenced this on Mar 9, 2020 from issue WIP: Qt: add QML based mobile GUI by icota
DrahtBot cross-referenced this on Mar 9, 2020 from issue build: Check QT library version by lucayepa
hebasto cross-referenced this on Mar 9, 2020 from issue build: Fix Qt processing of configure script for depends with DEBUG=1 by hebasto
hebasto cross-referenced this on Mar 9, 2020 from issue build: Require pkg-config for all of the hosts by hebasto
DrahtBot cross-referenced this on Mar 10, 2020 from issue build: Optionally enable -Wzero-as-null-pointer-constant by Empact
DrahtBot cross-referenced this on Mar 10, 2020 from issue Build: enable -Wdocumentation via isystem by Empact
DrahtBot removed the label Needs gitian build on Mar 11, 2020

build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts

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)

ddbb419310

build: Fix indentation in bitcoin_qt.m4 05a93d5d96

build: Remove duplicated QT_STATICPLUGIN define

QT_STATICPLUGIN is defined in BITCOIN_QT_CONFIGURE macro.

fded4f48c3

build: Remove extra tokens warning 9123ec15db
build: Fix m4 escaping 8a26848c46
hebasto force-pushed on Mar 16, 2020
hebasto commented at 9:14 AM on March 16, 2020: member
Updated e7cfcf79ea9992e2b844ffdcd64ee02af4f9fc07 -> 8a26848c460160e1279f26bc413f693a34e33b9d (pr18297.02 -> pr18297.03, diff):
- removed unused now internal functions _BITCOIN_QT_CHECK_QT5 and _BITCOIN_QT_CHECK_QT58
hebasto cross-referenced this on Mar 16, 2020 from issue build: Make the help string for --with-gui configure option unambiguous by hebasto
ryanofsky cross-referenced this on Mar 16, 2020 from issue build: Ambiguous help string for --with-gui configure option by hebasto
bitcoin deleted a comment on Mar 19, 2020
MarcoFalke added the label Needs gitian build on Mar 19, 2020
DrahtBot removed the label Needs gitian build on Mar 20, 2020
hebasto commented at 1:54 PM on March 24, 2020: member

Just noted that some dead code remains:https://github.com/bitcoin/bitcoin/blob/8a26848c460160e1279f26bc413f693a34e33b9d/build-aux/m4/bitcoin_qt.m4#L299-L301

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.
DrahtBot cross-referenced this on Apr 23, 2020 from issue build: optionally skip external warnings by vasild
laanwj added this to the "Blockers" column in a project

laanwj commented at 2:13 PM on June 4, 2020: member

Code review ACK 8a26848c460160e1279f26bc413f693a34e33b9d
laanwj commented at 2:15 PM on June 4, 2020: member

This makes significant changes to the build system and introduces a Qt patch. @dongcarl @theuni can you take a look at this plase?
bitcoin deleted a comment on Jun 4, 2020
MarcoFalke added the label Needs gitian build on Jun 4, 2020
MarcoFalke added the label Needs Guix build on Jun 4, 2020
theuni commented at 9:53 PM on June 5, 2020: member

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

DrahtBot commented at 6:28 AM on June 7, 2020: contributor

Gitian builds

File	commit 17cfa52d380673eaec3d428383b163ca4eb83f04<br>(master)	commit 61fe2c614d38ecf678eb001d7eb4acaedd3ec5dd<br>(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...`

DrahtBot removed the label Needs gitian build on Jun 7, 2020
in depends/patches/qt/fix_qt_pkgconfig.patch:8 in 492971de35 outdated
```
   0 | @@ -1,11 +1,23 @@
   1 |  --- old/qtbase/mkspecs/features/qt_module.prf
   2 |  +++ new/qtbase/mkspecs/features/qt_module.prf
   3 | -@@ -245,7 +245,7 @@
   4 | +@@ -264,7 +264,7 @@
   5 |   load(qt_targets)
   6 |   
   7 |   # this builds on top of qt_common
   8 |  -!internal_module:!lib_bundle:if(unix|mingw) {
   9 | -+unix|mingw {
```
dongcarl commented at 5:15 PM on June 12, 2020:

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

theuni commented at 6:07 PM on June 12, 2020:

@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).
dongcarl commented at 5:15 PM on June 12, 2020: contributor

Code Review ACK 8a26848c460160e1279f26bc413f693a34e33b9d
theuni approved
theuni commented at 6:14 PM on June 12, 2020: member

Code review ACK 8a26848c460160e1279f26bc413f693a34e33b9d
fanquake merged this on Jun 13, 2020
fanquake closed this on Jun 13, 2020
hebasto deleted the branch on Jun 13, 2020
hebasto cross-referenced this on Jun 13, 2020 from issue build: mingw32 issues with Qt if depends is built with DEBUG=1 by hebasto
sidhujag referenced this in commit c8addbe7ea on Jun 13, 2020
fanquake removed this from the "Blockers" column in a project

fanquake referenced this in commit 7d9008f43e on Jul 3, 2020
MarcoFalke removed the label Needs Guix build on Jul 29, 2020
hebasto cross-referenced this on Aug 9, 2020 from issue build: Add Qt version checking by hebasto
fanquake referenced this in commit 38c13a4a60 on Aug 24, 2020
sidhujag referenced this in commit dafb919f5c on Aug 24, 2020
xdustinface referenced this in commit e2d3235eb2 on Feb 17, 2021
xdustinface referenced this in commit 3e8687603d on Feb 17, 2021
gades referenced this in commit ac855be7fd on Jun 27, 2021
gades referenced this in commit 0e12d032a0 on Jun 27, 2021
bitcoin locked this on Feb 15, 2022