Differential D3017

[CMAKE] Add QT plugins according to the target platform
Needs ReviewPublic
Actions

Authored by Fabien on Fri, May 10, 11:27.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Summary

If the QT build is static, add the minimal integration plugin and any
platform specific plugin depending on the target.

Depends on D3091

Test Plan

mkdir buildcmake && cd buildcmake
cmake -GNinja ..
ninja
./src/qt/bitcoin-qt

Check that bitcoin-qt runs.

rm -rf *
cmake -GNinja -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/Win64.cmake \

-DBUILD_BITCOIN_SEEDER=OFF ..

ninja

Run the generated bitcoin-qt.exe on a 64 bits Windows.

rm -rf *
cmake -GNinja -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/Win32.cmake \

-DBUILD_BITCOIN_SEEDER=OFF ..

ninja

Run the generated bitcoin-qt.exe on a 32 or 64 bits Windows.

Note: OSX build needs another fix to build.

Diff Detail

Repository

rABC Bitcoin ABC

Branch

cmake_qt_plugins

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 5937
Build 9934: Bitcoin ABC Teamcity Staging
Build 9933: arc lint + arc unit

Event Timeline

Fabien created this revision.Fri, May 10, 11:27

Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, May 10, 11:27

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Fabien edited the summary of this revision. (Show Details)Fri, May 10, 11:29

Fabien added a parent revision: D2948: [CMAKE] Fix the prl conversion script library path issue.

Harbormaster completed remote builds in B5800: Diff 8593.Fri, May 10, 11:42

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Fabien added a child revision: D3018: [CMAKE] Fix CMake cross build for OSX.Fri, May 10, 11:54

Fabien added a child revision: D3019: [CMAKE] Add cross build toolchain files for PC Linux platform.Fri, May 10, 14:18

Fabien added a child revision: D3020: [CMAKE] Add cross compiling support for ARM32 and ARM64.Fri, May 10, 14:38

deadalnix requested changes to this revision.Sun, May 12, 23:22

deadalnix added inline comments.

src/qt/CMakeLists.txt
72 ↗	(On Diff #8593)	The fact you added this here but also felt the need to create a second block based on the exact same check is quite puzzling.
208 ↗	(On Diff #8593)	It's not what you are checking for
221 ↗	(On Diff #8593)	Presumably, that would need this to happen also if the plugin are not static.

This revision now requires changes to proceed.Sun, May 12, 23:22

Move all the static build code into a single section.
Fix comments.

src/qt/CMakeLists.txt
221 ↗	(On Diff #8593)	My understanding is that it is not necessary unless you build static. From https://doc.qt.io/qt-5/plugins-howto.html: `Qt applications automatically know which plugins are available, because plugins are stored in the standard plugin subdirectories. Because of this applications don't require any code to find and load plugins, since Qt handles them automatically.` Mode details in the link sections `Locating Plugins` and `Static Plugins`.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5822: Diff 8622.Mon, May 13, 09:11

Fabien added a child revision: D3034: [CMAKE] Disable OpenGL in Qt static build for OSX.Mon, May 13, 10:23

deadalnix requested changes to this revision.Thu, May 16, 16:00

deadalnix added inline comments.

src/qt/CMakeLists.txt
173 ↗	(On Diff #8622)	Why has this whole block moved ? This makes it impossible to check what actually changed. There doesn't seems to be a good reason for moving this around.
190 ↗	(On Diff #8622)	You have two loops that do the exact same thing now.

This revision now requires changes to proceed.Thu, May 16, 16:00

Fabien requested review of this revision.Thu, May 16, 16:06

Fabien added inline comments.

src/qt/CMakeLists.txt
173 ↗	(On Diff #8622)	It get moved with the block to keep related things together. The static dependencies file and the macro are used just below.
190 ↗	(On Diff #8622)	They are looping over 2 different lists.

deadalnix requested changes to this revision.Thu, May 16, 17:10

deadalnix added inline comments.

src/qt/CMakeLists.txt
173 ↗	(On Diff #8622)	And the things that come together needs to be here because ?
190 ↗	(On Diff #8622)	I'm fairly confident that there are way to loop over two different lists without duplicating the whole loop and loop body.

This revision now requires changes to proceed.Thu, May 16, 17:10

Fabien added inline comments.Thu, May 16, 17:27

src/qt/CMakeLists.txt
173 ↗	(On Diff #8622)	Because the bitcoin-qt-base target needs to be defined
190 ↗	(On Diff #8622)	OK, I'll wrap this into some macro

Fabien added inline comments.Thu, May 16, 17:32

src/qt/CMakeLists.txt
190 ↗	(On Diff #8622)	Or better concat them then loop

Avoid duplicated loop

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5879: Diff 8709.Fri, May 17, 10:13

deadalnix requested changes to this revision.Tue, May 21, 21:24

deadalnix added inline comments.

src/qt/CMakeLists.txt
27 ↗	(On Diff #8709)	There are other plateform check down bellow. It's unclear to me why the code is scattered all over the file, especially since you moved a chunk of code because It get moved with the block to keep related things together. The static dependencies file and the macro are used just below.
38 ↗	(On Diff #8709)	This code belong with what was moved down.
180 ↗	(On Diff #8709)	That comment doesn't address the code that comes next.

This revision now requires changes to proceed.Tue, May 21, 21:24

Rebase on top of D3091.
Move comment explaining static plugins to a better place.

Fabien edited the summary of this revision. (Show Details)Wed, May 22, 13:32

Fabien added a parent revision: D3091: [CMAKE] Move-only: refactor qt/CMakeLists.txt.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B5937: Diff 8801.Wed, May 22, 13:46

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Revision Contents
Changeset List

			Path	Packages
M			src/qt/CMakeLists.txt (38 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	8593	86cdd72		Fri, May 10, 11:27	★	★
Diff 2	8622	53b3b14	Move all the static build code into a single section.	Mon, May 13, 08:55	★	★
Diff 3	8709	53b3b14	Avoid duplicated loop	Fri, May 17, 09:59	★	★
Diff 4	8801	d7779af	Rebase on top of D3091.	Wed, May 22, 13:32	★	★

Commit	Tree	Parents	Author	Summary	Date
dd5e2772a080	6c462b062cbf	d7779afad978	Fabien	[CMAKE] Add QT plugins according to the target platform (Show More…)	Mon, May 6, 13:22

Status	Author	Revision
Needs Revision	Fabien	D3018 [CMAKE] Fix CMake cross build for OSX
Accepted	Fabien	D3019 [CMAKE] Add cross build toolchain files for PC Linux platform
Accepted	Fabien	D3020 [CMAKE] Add cross compiling support for ARM32 and ARM64
Accepted	Fabien	D3034 [CMAKE] Disable OpenGL in Qt static build for OSX
Closed	Fabien	D3033 [CMAKE] Add missing files to build bitcoin-qt on OSX
Closed	Fabien	D3032 [CMAKE] Fix bitcoind cross build for OSX
Closed	Fabien	D3031 [CMAKE] Cleanup the OSX platform file
Needs Review	Fabien	D3017 [CMAKE] Add QT plugins according to the target platform
Closed	Fabien	D2948 [CMAKE] Fix the prl conversion script library path issue
Closed	Fabien	D2947 [CMAKE] Determine the Qt library directory from a public variable
Needs Review	Fabien	D3091 [CMAKE] Move-only: refactor qt/CMakeLists.txt

Diff 8801

View Options

[CMAKE] Add QT plugins according to the target platformNeeds ReviewPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 8801

src/qt/CMakeLists.txt

[CMAKE] Add QT plugins according to the target platform
Needs ReviewPublic
Actions

Revision Contents
Changeset List