build: document and cleanup Qt hacks

fanquake commented at 7:39 am on September 4, 2020: member

Follow up on removing sed usage in #19761. Also nice to revisit & cleanup before 5.15.x.

fanquake added the label Build system on Sep 4, 2020

hebasto commented at 8:12 am on September 4, 2020: member

Concept ACK.

DrahtBot commented at 8:25 am on September 4, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20447 (depends: Patch qt_intersect_spans to avoid non-deterministic behavior in LLVM 8 by achow101)
#20440 (build: fix determinism issue when building qt with Clang 8 by fanquake)

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.

in depends/packages/miniupnpc.mk:18 in 08594dfc10 outdated

11@@ -12,6 +12,10 @@ $(package)_build_opts_mingw32=-f Makefile.mingw
12 $(package)_build_env+=CFLAGS="$($(package)_cflags) $($(package)_cppflags)" AR="$($(package)_ar)"
13 endef
14 
15+# sed is used here to replace the host and version that will end up as part of
16+# the miniupnpc User-Agent string, that will ultimately end up in bitcoind. i.e
17+# before: User-Agent: Debian/10, UPnP/1.1, MiniUPnPc/2.0
18+# after : User-Agent: x86_64-pc-linux-gnu, UPnP/1.1, MiniUPnPc/2.0.20180203

laanwj commented at 9:56 am on September 4, 2020:

unrelated to this change but: why are we inserting the architecture in here, seems like something not necessary to reveal

practicalswift commented at 3:06 pm on September 4, 2020:

I agree. Why make life easier for an attacker? :)

Ideally we shouldn’t leak anything here that might be of interest for an attacker.

fanquake commented at 6:36 am on September 11, 2020:

My assumption is that it was being inserted to make depends more deterministic. However I think we can improve this, as there’s no reason to reveal more of the version information either. Could leave the version as is /2.0 and either drop the architecture, or insert something else?

laanwj commented at 1:10 pm on September 15, 2020:

I guess UPnP/1.1, the version of the standard used, not the version of the software, is the only relevant thing here. The rest just aids exploiters if there’s another remotely exploitable UPnP bug.

laanwj commented at 10:01 am on September 4, 2020: member

Concept ACK it’s good to document these things carefully.

in depends/packages/miniupnpc.mk:16 in 80e24c640d outdated

11@@ -12,6 +12,10 @@ $(package)_build_opts_mingw32=-f Makefile.mingw
12 $(package)_build_env+=CFLAGS="$($(package)_cflags) $($(package)_cppflags)" AR="$($(package)_ar)"
13 endef
14 
15+# sed is used here to replace the host and version that will end up as part of
16+# the miniupnpc User-Agent string, that will ultimately end up in bitcoind. i.e

MarcoFalke commented at 10:50 am on September 5, 2020:

Which symbol or variable name contains this ua string? Not the p2p ua string of the version message, right?

laanwj commented at 11:03 am on September 5, 2020:

no, not the P2P UA string, it’s an UA string sent to the UPnP server (so broadcasted to the network you’[re connected to, if you have UPnP enabled)

fanquake force-pushed on Nov 13, 2020

fanquake marked this as ready for review on Nov 13, 2020

fanquake commented at 3:46 am on November 13, 2020: member

I’ve rebased, dropped the miniupnpc change (will be dealt with separately), added a commit to drop global_init_link_order from the mac-qmake.conf and fixed the the TODO in the preprocessing doc commit.

build: convert "echo" usage into a patch in qt package 49473ef211

build: pass XCODE_VERSION through to qt macOS cross compile conf

This should mostly be a no-op, however it would seem to make more sense
that we pass through the XCODE_VERSION we now have in depends, rather
than leaving the version set to 4.3.

fdde4c7ce6

build: remove plugin_no_soname from mac qt qmake.conf

plugin_no_soname was removed from Qt some time ago, see upstream commit
1d034244c261520d5e739534dc264c2500e02b5f. It was replaced with
plugin_with_soname, however that is currently only used (as of 5.15.x)
in the Android Clang mkspec.

bfd7e33b4b

build: don't copy Info.plist.* into mkspec for macOS qt build

We generate our own Info.plist as part of make deploy, and as far as I
can tell, it doesn't seem to have an effect wether these are present
during qt's build.

I also can't find a single mention of the .app plist in the qt code,
whereas there are multiple instances of .lib.

bd5d9336d9

build: document preprocessing steps in qt package 498fa16bea

build: remove global_init_link_order from mac qt qmake.conf

This has been around since the original import of Qt
(38be0d13830efd2d98281c645c3a60afe05ffece), however there
are now only two instatnces of it left in the qt codebase,
and from what I can gather, it's unused.

e1f2553e11

fanquake force-pushed on Nov 14, 2020

hebasto commented at 7:15 pm on November 15, 2020: member

Not sure how the “build: remove global_init_link_order from mac qt qmake.conf” (e1f2553e1148de9bd57818b40b5fe9da7ff5e246) commit could be tested/reviewed?

fanquake commented at 1:14 pm on November 17, 2020: member

Not sure how the “build: remove global_init_link_order from mac qt qmake.conf” (e1f2553) commit could be tested/reviewed?

I’d suggest diffing bitcoin-qt (for macOS) after building qt with and without global_init_link_order in mac-qmake.conf.

You could also check the qt 5.9 source and verify that global_init_link_order is not used anywhere (the only two occurences are in mkspec files). As opposed to an option like lib_version_first, which you’ll find multiple usages of, i.e:

0if(project->isActiveConfig("lib_version_first"))
1    project->values("TARGET_x").append(prefix + project->first("TARGET") + "." +
2                                            project->first("VER_MAJ") + "." +
3                                            project->first("QMAKE_EXTENSION_PLUGIN"));

laanwj added the label Needs Guix build on Nov 19, 2020

laanwj added the label Needs gitian build on Nov 19, 2020

laanwj commented at 11:30 am on November 19, 2020: member

Code review ACK e1f2553e1148de9bd57818b40b5fe9da7ff5e246

DrahtBot commented at 5:40 am on November 21, 2020: member

Guix builds

File	commit 094663842d0b342183d494b6f97e049ba2a5701b(master)	commit 6e61cb5624b3fdde3781c73c13f5305441dae9c6(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`3f8119c2423a44b8...`	`60bdc1a59b51a7fb...`
*-aarch64-linux-gnu.tar.gz	`22ff65251d630978...`	`75a2c7aa26ed498d...`
*-arm-linux-gnueabihf-debug.tar.gz	`3f0061c80e494815...`	`c4f210f9c03fea6a...`
*-arm-linux-gnueabihf.tar.gz	`09f215608bb786a6...`	`e231dfb74cc973d9...`
*-riscv64-linux-gnu-debug.tar.gz	`9e931544752956a4...`	`00dfcefc0ae1bdbb...`
*-riscv64-linux-gnu.tar.gz	`d9beb4a5b3f2ed87...`	`9e413b987fe4706b...`
*-win-unsigned.tar.gz	`d9f36ded7121be70...`	`afa5cc0bc1919aee...`
*-win64-debug.zip	`20e9e5f80e96e7ec...`	`7246b4c268e4140b...`
*-win64-setup-unsigned.exe	`0f4faa57c2fc8265...`	`f269a0b3314130a1...`
*-win64.zip	`36754cfde9d8c20a...`	`853badd4daa092b0...`
*-x86_64-linux-gnu-debug.tar.gz	`9bb911a9a4bb8c0e...`	`1c20909dac0c8e25...`
*-x86_64-linux-gnu.tar.gz	`627478d9b9a7c884...`	`2f45c01382d65cd2...`
*.tar.gz	`d020f231e8df6e91...`	`6ae7dde4ff2b5275...`
guix_build.log	`a5f89f8feacab638...`	`ebcc508454d3e9bd...`
guix_build.log.diff		`25f93fc76abfb725...`

DrahtBot removed the label Needs Guix build on Nov 21, 2020

fanquake merged this on Nov 22, 2020

fanquake closed this on Nov 22, 2020

MarcoFalke removed the label Needs gitian build on Nov 22, 2020

MarcoFalke added the label Needs gitian build on Nov 23, 2020

sidhujag referenced this in commit ebbdd0341e on Nov 23, 2020

MarcoFalke removed the label Needs gitian build on Nov 26, 2020

fanquake deleted the branch on Feb 9, 2021

laanwj referenced this in commit fa2a5b8f3a on Mar 23, 2021

sidhujag referenced this in commit e17ca02bea on Mar 23, 2021

kittywhiskers referenced this in commit 381326ac89 on Nov 24, 2021

kittywhiskers referenced this in commit f5a9a491c0 on Nov 30, 2021

PastaPastaPasta referenced this in commit 25a965d691 on Nov 30, 2021

gades referenced this in commit 901f0fef93 on May 12, 2022

DrahtBot locked this on Aug 18, 2022

build: document and cleanup Qt hacks #19867

Conflicts

Guix builds