Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rABC45706dfa7c29: [CMAKE] Add QT plugins according to the target platform

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

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 10 2019, 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)May 10 2019, 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.May 10 2019, 11:42

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

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

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

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

deadalnix requested changes to this revision.May 12 2019, 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.May 12 2019, 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.May 13 2019, 09:11

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

deadalnix requested changes to this revision.May 16 2019, 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.May 16 2019, 16:00

Fabien requested review of this revision.May 16 2019, 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.May 16 2019, 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.May 16 2019, 17:10

Fabien added inline comments.May 16 2019, 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.May 16 2019, 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.May 17 2019, 10:13

deadalnix requested changes to this revision.May 21 2019, 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.May 21 2019, 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)May 22 2019, 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.May 22 2019, 13:46

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

deadalnix accepted this revision.May 24 2019, 17:21

This revision is now accepted and ready to land.May 24 2019, 17:21

Closed by commit rABC45706dfa7c29: [CMAKE] Add QT plugins according to the target platform (authored by Fabien). · Explain WhyMay 25 2019, 07:13

This revision was automatically updated to reflect the committed changes.

Fabien added a commit: rABC45706dfa7c29: [CMAKE] Add QT plugins according to the target platform.

Fabien mentioned this in D2667: [CMAKE] Fix bitcoin-qt cross compilation for Windows.May 25 2019, 07:14

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

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 8870

src/qt/CMakeLists.txt

[CMAKE] Add QT plugins according to the target platformClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 8870

src/qt/CMakeLists.txt

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

Revision Contents
Changeset List