bitcoin_qt.m4: Minor fixes and clean-ups. #11222

pull lemzwerg wants to merge 6 commits into bitcoin:master from lemzwerg:config-fixes changing 1 files +180 −154

lemzwerg commented at 6:53 AM on September 3, 2017: contributor

This is the same as #11101 but moved to a separate branch.

bitcoin_qt.m4: Improve QT_VERSION tests.

Use '<QtCore/qconfig.h> and '<QtCore/qglobal.h>' for testing QT_VERSION.
This makes the tests work with both Qt4 and Qt5, even if '-fPIC' or '-fPIE'
is not used (the compiler might choke otherwise if QT_REDUCE_RELOCATIONS is
active).

b06bc2937b

bitcoin_qt.m4: Use correct M4 quoting characters. 28641e2bfe
bitcoin_qt.m4: Add missing braces around variables in autoconf messages. 0d9837ed39
bitcoin_qt.m4: Add missing dollar sign for variable. c6087c9b21

bitcoin_qt.m4: Orthogonalize string quoting.

Add double qoutes to string tests where arguments could (theoretically)
contain spaces.

Remove double quotes where not necessary.

e90d91c6f3

bitcoin_qt.m4: Orthogonalize M4 quoting.

Consequently quote all arguments of M4 functions.

Mark empty M4 arguments with '[]'.

40c8a65c56

fanquake added the label Build system on Sep 3, 2017
fanquake commented at 7:25 AM on September 3, 2017: member

Please update the PR title to something more explanatory, as they are now included in merge commits.
lemzwerg renamed this:
~~Config fixes~~
bitcoin_qt.m4: Minor fixes and clean-ups.
on Sep 3, 2017
lemzwerg commented at 7:36 AM on September 3, 2017: contributor

Better now?
fanquake assigned theuni on Sep 3, 2017
practicalswift commented at 4:07 PM on September 3, 2017: contributor

utACK 40c8a65c5653d6cfd7859700fe4e7d6468ce2231
laanwj commented at 1:03 PM on October 2, 2017: member

@theuni Can you take a look at the build system changes here please?

in build-aux/m4/bitcoin_qt.m4:324 in c6087c9b21 outdated

 320 | @@ -321,7 +321,7 @@ AC_DEFUN([_BITCOIN_QT_IS_STATIC],[
 321 |      [bitcoin_cv_static_qt=yes],
 322 |      [bitcoin_cv_static_qt=no])
 323 |    ])
 324 | -  if test xbitcoin_cv_static_qt = xyes; then
 325 | +  if test x$bitcoin_cv_static_qt = xyes; then

theuni commented at 9:49 PM on October 2, 2017:

This was clearly broken before, but it didn't matter because we define QT_STATICPLUGIN elsewhere. Let's just delete this.

theuni commented at 11:33 PM on October 2, 2017: member
I wasn't comfortable reviewing e90d91c6f31e4b8fe74029fecfacf56190c217aa accurately, so I scripted it. Could you please update the commit message to make it a scripted diff? See here for an explanation: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#scripted-diffs

The script I used, which matches your commit, is:
```
sed -iE -e 's/"x$yes\|no\|auto$\?"/x\1/g' -e 's/test $x\$[^ ]*$/test "\1"/g' build-aux/m4/bitcoin_qt.m4
```
theuni commented at 11:39 PM on October 2, 2017: member

Did you use some tool or script for the m4 macros? I think it's fine, but I'm nervous about some issue turning up months from now because of an errant bracket.
lemzwerg commented at 4:03 AM on October 3, 2017: contributor
No time for creating a script, sorry. I suggest that you do a comparison with
```
git diff --word-diff-regex=. --word-diff=color e90d91c^ e90d91c
```
for review; this shows the changes of this commit in the most compact form.

Just a final note regarding bitcoin's M4 stuff for configuring Qt: I now believe that this is the wrong way. Without the help of qmake it will always remain a guesswork what compiler flags should be really used, and what code should be used for static linking. Have a look at

http://repo.or.cz/ttfautohint.git/blob_plain/HEAD:/m4/autotroll.m4 http://repo.or.cz/ttfautohint.git/blob/HEAD:/configure.ac#l52 http://repo.or.cz/ttfautohint.git/blob/HEAD:/frontend/static-plugins.cpp.in

for my current solution. For example, if I configure ttfautohint for cross-building with mxe, static-plugins.cpp automatically contains the following code.
```
// This file is autogenerated by qmake. It imports static plugin classes for
// static plugins specified using QTPLUGIN and QT_PLUGIN_CLASS.<plugin> variables.
#include <QtPlugin>
Q_IMPORT_PLUGIN(QWindowsIntegrationPlugin)
```
lemzwerg commented at 3:59 AM on October 14, 2017: contributor

FYI: autotroll has been updated with my changes and is now available from https://github.com/tsuna/autotroll.
theuni commented at 11:13 PM on October 16, 2017: member

@lemzwerg I'd really rather not change all this around arbitrarily. And I'd certainly not require qmake. Especially considering that they'll be replacing it with qbs in the future.

Everything but the last commit looks good to me (after addressing #11222 (review)). I assume the last is probably ok too, but I'd rather not risk introducing some obscure new error if this doesn't fix a reported bug.
laanwj commented at 12:32 PM on November 17, 2017: member

Needs update for @cfields' comments.
fanquake commented at 2:27 PM on November 17, 2017: member

Closing in favour of #11711.
fanquake closed this on Nov 17, 2017
laanwj referenced this in commit 6378e5c514 on Jan 29, 2018
PastaPastaPasta referenced this in commit 33f47c7158 on Jan 17, 2020
PastaPastaPasta referenced this in commit 865dafe626 on Jan 22, 2020
PastaPastaPasta referenced this in commit db9890646f on Jan 22, 2020
PastaPastaPasta referenced this in commit da961e7122 on Jan 29, 2020
PastaPastaPasta referenced this in commit 99da06d40e on Jan 29, 2020
PastaPastaPasta referenced this in commit 661d8816ac on Jan 29, 2020
PastaPastaPasta referenced this in commit 4e230811b6 on Jan 31, 2020
ckti referenced this in commit 563644c178 on Mar 28, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#11101 bitcoin_qt.m4: Minor fixes, improve code orthogonality #11711 bitcoin_qt.m4: Minor fixes and clean-ups.