Page MenuHomePhabricator

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

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

Details

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

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
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

src/qt/CMakeLists.txt
190 ↗(On Diff #8622)

Or better concat them then loop

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.

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