Details

Reviewers

Group Reviewers

Restricted Project

Commits

rSTAGING45706dfa7c29: [CMAKE] Add QT plugins according to the target platform
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

Branch

cmake_qt_plugins

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 5800
Build 9662: Bitcoin ABC Buildbot (legacy)
Build 9661: arc lint + arc unit

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	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	It's not what you are checking for
221	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	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.