Depends : Qt Use Top-Level Structure #20600

BlockMechanic · 2020-12-08T13:30:17Z

This PR reworks the way Qt dependency is built :-

Introduces a top level config/build which
Simplifies the way Qt is built, removing confusing lines
Resolves static build for WIP: Qt: add QML based mobile GUI #16883
Mimic how Qt is built from repo/all-in-one
Is related to build: use more legible (q)make commands in qt package #20504 which makes (q)make commands simpler
Continuation/Completion of icota@683710e which had incomplete dependency chain.

While trying to add Qml/QtQuick i took a closer look at how QT builds and found that it uses a top level qt.pro which by far simplifies the build process.

maflcko · 2020-12-08T13:54:12Z

The ci doesn't pass, but this looks like a major rewrite, so it would be better to wait for some Concept ACKs from the build people

sipa · 2020-12-08T18:27:42Z

so it would be better to wait for some Concept ACKs from the build people

Seven, to the Build People, great miners and craftsmen of the mountain halls.

DrahtBot · 2020-12-08T19:31:19Z

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: Drop workaround for a fixed bug in Qt build system #20650 (depends: Drop workaround for a fixed bug in Qt build system by hebasto)
depends: Drop unneeded patches for qt package #20642 (depends: Drop unneeded patches for qt package by hebasto)
depends: Use Qt top-level build facilities #20641 (depends: Use Qt top-level build facilities by hebasto)
build: use more legible (q)make commands in qt package #20504 (build: use more legible (q)make commands in qt package 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.

Temp revert, bitcoin#19882

Remove Debug line

Removed unused patch

jonasschnelli · 2020-12-09T07:46:16Z

Concept ACK on adding support for Qml/QtQuick.
Haven't studied the code in deep but the submodule part seems messy.

laanwj · 2020-12-10T13:22:02Z

depends/patches/qt/.gitmodules

@@ -0,0 +1,50 @@
+[submodule "qtbase"]


I'm not entirely sure what this does. Does this make qt submodules of our repository?

Hi,
No, it does not make qt submodule of bitcoin. The file is just used by qt.pro to determine which qt modules to build, and what their dependencies are. Perhaps, i can rename the file to prevent confusion ?

@laanwj I've renamed the files to prevent confusion

icota · 2020-12-10T13:55:14Z

Concept ACK but:

changing the way we build Qt and (subsequent) enabling of QML modules should probably be two separate PRs
.configure is a script so doesn't really belong in patches (actually none of the added files do)

BlockMechanic · 2020-12-10T14:04:03Z

Concept ACK but:

changing the way we build Qt and (subsequent) enabling of QML modules should probably be two separate PRs

.configure is a script so doesn't really belong in patches (actually none of the added files do)

Yeah, none of them are strictly patches. I was avoiding making a separate folder to maintain general logic/structure.

icota · 2020-12-10T14:12:13Z

depends/patches/qt/.configure

@@ -0,0 +1,17 @@
+#! /bin/sh


This files looks to be stripped of its licence (LGPL). It's better to extract the file (and others) directly from the submodules. If the files have been slightly modified from the Qt source add a patchfile - to be in line with the current practice.

These specific files are not within the submodules unfortunately nor are they listed in the download directory see :- https://download.qt.io/official_releases/qt/5.9/5.9.8/submodules. They are only in the all-in-one or the git, so I had copied them separately to avoid downloading the whole all-in-one only to extract these three files and then duplicate the work by separately downloading each module.

In that case I vote for downloading the all-in-one. It would reduce this PR by three files and most of its lines. It would also simplify existing code. The download would take a bit longer but if we want QML I think it's worth it.

I'm already working on that approach @icota. Also worth noting that single file was the old approach, and it was changed in ab67dd7, maybe @theuni had a motivation for using modules.

@promag I have an all in one build already functional, should i open a PR and we collaborate ?

@BlockMechanic sure! Happy to review and compare with my change.

hebasto · 2020-12-10T18:14:07Z

Concept ACK on adding QML support.

hebasto · 2020-12-13T14:34:04Z

@icota

changing the way we build Qt and (subsequent) enabling of QML modules should probably be two separate PRs

The first part is implemented in #20641.

DrahtBot · 2020-12-16T13:44:55Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

fanquake · 2020-12-17T06:03:17Z

I'm not a fan of this approach. It's also unclear exactly what's going on, or why this is being done this way from a quick glance, as most of the commits have no useful title or description. I'm going to close this in favour of #20641, as it seems that achieves the same goal, and is going to be far more maintainable in the long term.

1155978 build, qt: Do not install *.prl files (Hennadii Stepanov) 763793b build, qt: Fix wrong cross-compiling detection on macOS (Hennadii Stepanov) 3098272 build, qt: Force bootstrap while building linguist tools (Hennadii Stepanov) 689320e build, qt: Drop translations.pro hack (Hennadii Stepanov) 6a1f98f build, qt: Drop lrelease dependency patch (Hennadii Stepanov) 39e561e build, qt: Add linguist_tools list (Hennadii Stepanov) 27d3def build: Use Qt top-level build facilities (Hennadii Stepanov) Pull request description: This PR: - uses Qt top-level build facilities without the need to download all-in-one archive - is based on **BlockMechanic**'s [idea](bitcoin/bitcoin#20600), and is an alternative to #20600 - makes it easy to integrate [new modules](bitcoin/bitcoin#16883) into static builds - has the minimal diff - makes the qt package build process streamlined by dropping some patches and hacks (an alternative to #21420 and #20642) Fixes #18536 (a non-intrusive alternative to #21589 and #19785). Fixes #14648. Fixes #21588 (a non-intrusive alternative to #21591). Required for adding [Wayland support](bitcoin/bitcoin#19950) on Linux. --- **Note for reviewers**: With 9046de8a4cbc3899fed9eae084115f423e7ac5bd from #21995 it is easy to verify that there are no changes in the resulted `qt` package archive on the per commit basis. For example, for `HOST=i686-pc-linux-gnu` no commit in this PR introduces any changes. ACKs for top commit: fanquake: ACK 1155978 Tree-SHA512: 667b06b72cb7ff26d68b9b88e8dddb51084783ca9e3d80b3392710794c1dc7fd77bbcc3ccf4ccece9919d33b9bf8fce13c5059502bd228021dc7c93fdb87ca7a

1155978 build, qt: Do not install *.prl files (Hennadii Stepanov) 763793b build, qt: Fix wrong cross-compiling detection on macOS (Hennadii Stepanov) 3098272 build, qt: Force bootstrap while building linguist tools (Hennadii Stepanov) 689320e build, qt: Drop translations.pro hack (Hennadii Stepanov) 6a1f98f build, qt: Drop lrelease dependency patch (Hennadii Stepanov) 39e561e build, qt: Add linguist_tools list (Hennadii Stepanov) 27d3def build: Use Qt top-level build facilities (Hennadii Stepanov) Pull request description: This PR: - uses Qt top-level build facilities without the need to download all-in-one archive - is based on **BlockMechanic**'s [idea](bitcoin#20600), and is an alternative to bitcoin#20600 - makes it easy to integrate [new modules](bitcoin#16883) into static builds - has the minimal diff - makes the qt package build process streamlined by dropping some patches and hacks (an alternative to bitcoin#21420 and bitcoin#20642) Fixes bitcoin#18536 (a non-intrusive alternative to bitcoin#21589 and bitcoin#19785). Fixes bitcoin#14648. Fixes bitcoin#21588 (a non-intrusive alternative to bitcoin#21591). Required for adding [Wayland support](bitcoin#19950) on Linux. --- **Note for reviewers**: With 9046de8 from bitcoin#21995 it is easy to verify that there are no changes in the resulted `qt` package archive on the per commit basis. For example, for `HOST=i686-pc-linux-gnu` no commit in this PR introduces any changes. ACKs for top commit: fanquake: ACK 1155978 Tree-SHA512: 667b06b72cb7ff26d68b9b88e8dddb51084783ca9e3d80b3392710794c1dc7fd77bbcc3ccf4ccece9919d33b9bf8fce13c5059502bd228021dc7c93fdb87ca7a

BlockMechanic added 2 commits December 6, 2020 17:00

Add QML/ QtQuick Depends

ef283d5

Start using top level Qt for depends

633660c

fanquake added the Build system label Dec 8, 2020

Update qt.mk

8068300

BlockMechanic added 2 commits December 8, 2020 22:10

Update funcs.mk

7eb9c96

Temp revert, bitcoin#19882

Update qt.mk

4ff303e

Remove Debug line

DrahtBot mentioned this pull request Dec 8, 2020

build: use more legible (q)make commands in qt package #20504

Closed

Delete fix_android_base_head_conf.patch

88da239

Removed unused patch

DrahtBot mentioned this pull request Dec 8, 2020

build, ci: Add file-based logging for individual packages #19952

Merged

BlockMechanic added 2 commits December 9, 2020 09:53

Version Agnostic

f7aefca

Remove unnecessary line

e96fb4a

BlockMechanic referenced this pull request in icota/bitcoin Dec 10, 2020

depends: add qtdeclarative and qtquickcontrols2 to qt

683710e

laanwj reviewed Dec 10, 2020

View reviewed changes

Rename to qt.modules

f295c9d

icota suggested changes Dec 10, 2020

View reviewed changes

Add complete configure file

37b7460

BlockMechanic added 2 commits December 10, 2020 20:22

Create qt (copy).mk

6f500bb

Update qt.mk

a606ebd

dongcarl mentioned this pull request Dec 11, 2020

RFC: Migrate from Qt5 to Qt6's Qt5 compat #20627

Closed

BlockMechanic added 4 commits December 13, 2020 05:17

Delete qt (copy).mk

158b0a9

Resolve Modules

b02b2a8

Resolve Modules

68b9816

Resolve Modules

9630653

hebasto mentioned this pull request Dec 13, 2020

depends: Use Qt top-level build facilities #20641

Merged

This was referenced Dec 13, 2020

depends: Drop unneeded patches for qt package #20642

Closed

depends: Drop workaround for a fixed bug in Qt build system #20650

Merged

DrahtBot added the Needs rebase label Dec 16, 2020

fanquake closed this Dec 17, 2020

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

Depends : Qt Use Top-Level Structure #20600

Depends : Qt Use Top-Level Structure #20600

Uh oh!

Conversation

BlockMechanic commented Dec 8, 2020

Uh oh!

maflcko commented Dec 8, 2020

Uh oh!

sipa commented Dec 8, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

jonasschnelli commented Dec 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icota commented Dec 10, 2020

Uh oh!

BlockMechanic commented Dec 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 10, 2020

Uh oh!

hebasto commented Dec 13, 2020

Uh oh!

DrahtBot commented Dec 16, 2020

Uh oh!

fanquake commented Dec 17, 2020

Uh oh!

Uh oh!

DrahtBot commented Dec 8, 2020 •

edited

Loading