build: document and cleanup Qt hacks #19867

fanquake · 2020-09-04T07:39:09Z

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

hebasto · 2020-09-04T08:12:00Z

Concept ACK.

DrahtBot · 2020-09-04T08:25:16Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

depends: Patch qt_intersect_spans to avoid non-deterministic behavior in LLVM 8 #20447 (depends: Patch qt_intersect_spans to avoid non-deterministic behavior in LLVM 8 by achow101)
build: fix determinism issue when building qt with Clang 8 #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.

depends/packages/miniupnpc.mk

laanwj · 2020-09-04T10:01:36Z

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

depends/packages/miniupnpc.mk

fanquake · 2020-11-13T03:46:42Z

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.

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.

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.

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.

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.

hebasto · 2020-11-15T19:15:50Z

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

fanquake · 2020-11-17T13:14:58Z

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:

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

laanwj · 2020-11-19T11:30:08Z

Code review ACK e1f2553

DrahtBot · 2020-11-21T05:40:20Z

Guix builds

File	commit `0946638` (master)	commit `6e61cb5` (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...`

180dc3c build: miniupnpc 2.2.2 (fanquake) Pull request description: Creating the dll subdir is no-longer required. We can drop our wingen patch. One issue that came up in [#19867](bitcoin/bitcoin#19867 (comment)): > unrelated to this change but: why are we inserting the architecture in here, seems like something not necessary to reveal > 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? I've dropped our `sed` and added a patch that just removes the OS string and miniupnpc version from the User-Agent. i.e: ```bash # master strings depends/x86_64-apple-darwin19.6.0/lib/libminiupnpc.a | rg -i User-Agent User-Agent: x86_64-apple-darwin19.6.0, UPnP/1.1, MiniUPnPc/2.0.20180203 User-Agent: x86_64-apple-darwin19.6.0, UPnP/1.1, MiniUPnPc/2.0.20180203 # this PR strings depends/x86_64-apple-darwin19.6.0/lib/libminiupnpc.a | rg -i User-Agent User-Agent: UPnP/1.1 User-Agent: UPnP/1.1 ``` Note that built unmodified (miniupnp/miniupnp@22c1386), the User-Agent would be: ```bash strings libminiupnpc.dylib | rg User-Agent User-Agent: Darwin/19.6.0, UPnP/1.1, MiniUPnPc/2.2.0 User-Agent: Darwin/19.6.0, UPnP/1.1, MiniUPnPc/2.2.0 ``` ACKs for top commit: laanwj: Code review ACK 180dc3c hebasto: ACK 180dc3c. Tree-SHA512: b0b6e623dbc5499e28faedf992d84278d6a11887a45a3806957b9e08886c5e56044cdfa2e7d7ec81cb1dd55f89be99834367905315d6bc611ba530e91d889ad1

180dc3c build: miniupnpc 2.2.2 (fanquake) Pull request description: Creating the dll subdir is no-longer required. We can drop our wingen patch. One issue that came up in [bitcoin#19867](bitcoin#19867 (comment)): > unrelated to this change but: why are we inserting the architecture in here, seems like something not necessary to reveal > 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? I've dropped our `sed` and added a patch that just removes the OS string and miniupnpc version from the User-Agent. i.e: ```bash # master strings depends/x86_64-apple-darwin19.6.0/lib/libminiupnpc.a | rg -i User-Agent User-Agent: x86_64-apple-darwin19.6.0, UPnP/1.1, MiniUPnPc/2.0.20180203 User-Agent: x86_64-apple-darwin19.6.0, UPnP/1.1, MiniUPnPc/2.0.20180203 # this PR strings depends/x86_64-apple-darwin19.6.0/lib/libminiupnpc.a | rg -i User-Agent User-Agent: UPnP/1.1 User-Agent: UPnP/1.1 ``` Note that built unmodified (miniupnp/miniupnp@22c1386), the User-Agent would be: ```bash strings libminiupnpc.dylib | rg User-Agent User-Agent: Darwin/19.6.0, UPnP/1.1, MiniUPnPc/2.2.0 User-Agent: Darwin/19.6.0, UPnP/1.1, MiniUPnPc/2.2.0 ``` ACKs for top commit: laanwj: Code review ACK 180dc3c hebasto: ACK 180dc3c. Tree-SHA512: b0b6e623dbc5499e28faedf992d84278d6a11887a45a3806957b9e08886c5e56044cdfa2e7d7ec81cb1dd55f89be99834367905315d6bc611ba530e91d889ad1

merge bitcoin#16110, bitcoin#16413, bitcoin#16837, bitcoin#17466, bitcoin#19959, bitcoin#19867, bitcoin#20447, bitcoin#21363, partial bitcoin#17730: bump qt

fanquake added the Build system label Sep 4, 2020

DrahtBot mentioned this pull request Sep 4, 2020

build: Qt 5.15.x #19716

Closed

laanwj reviewed Sep 4, 2020

View reviewed changes

depends/packages/miniupnpc.mk Outdated Show resolved Hide resolved

maflcko reviewed Sep 5, 2020

View reviewed changes

depends/packages/miniupnpc.mk Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Sep 19, 2020

build: patch qt libpng to fix powerpc build #19959

Merged

fanquake force-pushed the document_remaining_sed branch from 08594df to df282dd Compare November 13, 2020 03:46

fanquake marked this pull request as ready for review November 13, 2020 03:46

fanquake added 6 commits November 14, 2020 08:54

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

49473ef

build: pass XCODE_VERSION through to qt macOS cross compile conf

fdde4c7

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.

build: document preprocessing steps in qt package

498fa16

build: remove global_init_link_order from mac qt qmake.conf

e1f2553

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.

fanquake force-pushed the document_remaining_sed branch from df282dd to e1f2553 Compare November 14, 2020 00:54

fanquake mentioned this pull request Nov 19, 2020

build: miniupnpc 2.2.2 #20421

Merged

laanwj added DrahtBot Guix build requested Needs gitian build labels Nov 19, 2020

DrahtBot removed the DrahtBot Guix build requested label Nov 21, 2020

This was referenced Nov 21, 2020

build: fix determinism issue when building qt with Clang 8 #20440

Closed

depends: Patch qt_intersect_spans to avoid non-deterministic behavior in LLVM 8 #20447

Merged

fanquake merged commit e9a1c9f into bitcoin:master Nov 22, 2020

maflcko removed the Needs gitian build label Nov 22, 2020

maflcko added the Needs gitian build label Nov 23, 2020

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 23, 2020

Merge bitcoin#19867: build: document and cleanup Qt hacks

ebbdd03

maflcko removed the Needs gitian build label Nov 26, 2020

fanquake deleted the document_remaining_sed branch February 9, 2021 07:11

kwvg added a commit to kwvg/dash that referenced this pull request Nov 24, 2021

merge bitcoin#19867: document and cleanup Qt hacks

381326a

kwvg added a commit to kwvg/dash that referenced this pull request Nov 30, 2021

merge bitcoin#19867: document and cleanup Qt hacks

f5a9a49

gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 12, 2022

merge bitcoin#19867: document and cleanup Qt hacks

901f0fe

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

build: document and cleanup Qt hacks #19867

build: document and cleanup Qt hacks #19867

Uh oh!

fanquake commented Sep 4, 2020

Uh oh!

hebasto commented Sep 4, 2020

Uh oh!

DrahtBot commented Sep 4, 2020 •

edited

Loading

Uh oh!

Uh oh!

laanwj commented Sep 4, 2020

Uh oh!

Uh oh!

fanquake commented Nov 13, 2020

Uh oh!

hebasto commented Nov 15, 2020 •

edited

Loading

Uh oh!

fanquake commented Nov 17, 2020

Uh oh!

laanwj commented Nov 19, 2020

Uh oh!

DrahtBot commented Nov 21, 2020

Uh oh!

Uh oh!

build: document and cleanup Qt hacks #19867

build: document and cleanup Qt hacks #19867

Uh oh!

Conversation

fanquake commented Sep 4, 2020

Uh oh!

hebasto commented Sep 4, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

Uh oh!

laanwj commented Sep 4, 2020

Uh oh!

Uh oh!

fanquake commented Nov 13, 2020

Uh oh!

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

Uh oh!

Uh oh!

fanquake commented Nov 17, 2020

Uh oh!

laanwj commented Nov 19, 2020

Uh oh!

DrahtBot commented Nov 21, 2020

Guix builds

Uh oh!

Uh oh!

DrahtBot commented Sep 4, 2020 •

edited

Loading

hebasto commented Nov 15, 2020 •

edited

Loading