Page MenuHomePhabricator

[CMAKE] Fix bitcoin-qt cross compilation for Windows
AbandonedPublic

Authored by Fabien on Mar 7 2019, 17:14.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

bitcoin-qt.exe will build but won't run due to missing QT5
dependencies.
This patch adds the required dependencies.

Depends on D2666

Test Plan

Prerequisite:
Follow the doc/build-windows.md and depends/README.md documentation
and install the dependencies for i686-w64-mingw32 and
x86_64-w64-mingw32.

# 32-bit version
mkdir buildcmake && cd buildcmake
cmake -GNinja .. -DBUILD_BITCOIN_SEEDER=OFF \
  -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/Win32.cmake
ninja

Check the build succeeds.
Check the generated src/qt/bitcoin-qt.exe binary runs as expected.

Operate the same for Win64.

# 64-bit version
rm -rf *
cmake -GNinja .. -DBUILD_BITCOIN_SEEDER=OFF \
  -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/Win64.cmake
ninja

Check the build succeeds.
Check the generated src/qt/bitcoin-qt.exe binary runs as expected.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cmake_windows_bitcoin_qt
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5176
Build 8415: Bitcoin ABC Buildbot (legacy)
Build 8414: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Mar 7 2019, 17:30
deadalnix added inline comments.
src/qt/CMakeLists.txt
157

Please do the same as configure instead.

162

Qt should provide target for these with proper dependencies and all.

This revision now requires changes to proceed.Mar 7 2019, 17:30

Set QT_STATICPLUGIN for all static builds, improve libraries dependency

src/qt/CMakeLists.txt
162

Actually find_package searches for a Config.cmake suffix, and these files are only provided for high level modules.
These libs seems to be optional and not dependencies of the higher level modules, hence the find_library call.
Autotools solves it by using whether pkgconfig if enabled or AC_CHECK_LIB, which I believe is close to what find_library achieves.
There is room for improvement anyway, especially for working with older QT version (I only ported for version >= 5.8 here which can be too restrictive).

deadalnix requested changes to this revision.Mar 12 2019, 20:08
deadalnix added inline comments.
src/qt/CMakeLists.txt
164 ↗(On Diff #7683)

There is a variation of this for each plateform.

203 ↗(On Diff #7683)

Why is that required ? If it is include on other plateform automatically, why isn't it on windows ? If it isn't, why is it necessary specifically on windows ?

162

Qt should be able to set it's dependencies properly, or to provide target for these dependencies.

This revision now requires changes to proceed.Mar 12 2019, 20:08

Fix bugs in the contrib/convert-prl-libs-to-cmake.pl. This allows for
converting the windows plugin prl script rather than manually linking
the dependencies.

src/qt/CMakeLists.txt
162

There is no package nor config for these targets.
What QT offers for these is a .prl file (for qmake) and a .pc file (for package-config).

Intended that we already use a script to parse the .prl files and convert them to cmake, I will use that to link the dependencies.

contrib/qt/convert-prl-libs-to-cmake.pl
88 ↗(On Diff #8256)

If there is no INTERFACE_LINK_LIBRARIES for this target, get_target_property set _link_libs to _link_libs_NOTFOUND.
This is correctly interpreted as unset by the if() statement, but displayed as-is when interpreted inside a string, causing the target to get a link flag -l_link_libs_NOTFOUND which obviously fails.

119 ↗(On Diff #8256)

The QT directory structure is so that the .a library files are located under a lib directory. This lib directory is referenced in the .prl files as $$[QT_INSTALL_LIBS]$$.

The replacement here used to assume that the .prl files are also located under lib, but there are exceptions to this rule: the plugins are located in a plugin subdirectory, plugin being at the same level as lib.

When parsing plugins .prl files, the script would have linked to static libs from the wrong directory.
Now the $qt_install_dir is set by a new script parameter, making it independent from the .prl location.

deadalnix requested changes to this revision.Apr 24 2019, 19:43
deadalnix added inline comments.
contrib/qt/convert-prl-libs-to-cmake.pl
61 ↗(On Diff #8256)

Te case of the prl not being in the lib repository is handled here.

119 ↗(On Diff #8256)

I'm not sure what you are trying to do but.

1/ The assumptions stated in this comment are erroneous, so I highly doubt the code is correct. The actual assumption made is that the prl is either in the same directory as the library, or in its parent directory.
2/ The fact you felt the need to write that comment is a strong tell that you should have refactored the code and/or added comments if refactoring isn't possible before proceeding. You *KNOW* the code doesn't stand on its own, so why submit for review at all ?

src/qt/CMakeLists.txt
32 ↗(On Diff #8256)

This makes even more assumptions than the previous way, relying on private variable being set the way you expect and being the same for all modules, which none are guaranteed.

46 ↗(On Diff #8256)

It looks like you should add this to QT_REQUIRED_COMPONENTS as it clearly is a component from Qt that is required in this case.

It's also probably a good idea to port whatever autotool is doing for these libraries instead of hacking each plateform one by one.

165 ↗(On Diff #8256)

What problem is this solving ? Presumably, this should be set the proper way by the cmake files provided by Qt and if not, then actually figuring out what the problem is seems like the first step here.

178 ↗(On Diff #8256)

Would that be a Qt component that is required ? And if so, why isn't QT_REQUIRED_COMPONENTS used ?

This revision now requires changes to proceed.Apr 24 2019, 19:43
Fabien requested review of this revision.Apr 24 2019, 20:22
Fabien added inline comments.
contrib/qt/convert-prl-libs-to-cmake.pl
61 ↗(On Diff #8256)

This does not handle the case where the file is in another directory, only a special case where there is no "lib" directory.

119 ↗(On Diff #8256)

1/ The assumptions are what the previous version did. This code removes these assumptions by using an independent variable. This is actually a fix.
2/ The code itself is very simple, I found it useful to show the use case from a higher level, hence the comment. I could maybe have added a comment in the code itself.

src/qt/CMakeLists.txt
32 ↗(On Diff #8256)

The QT directory structure is fixed, so I think this is safe. It does not make more assumptions as it breaks the previous one which is known to be wrong.
Regarding the variable being private, it is already used in this cmake file and already relied upon, so I reused it. I can eventually replace it with the Qt5 directory from the find_package and some ../.

46 ↗(On Diff #8256)

You can't run the find_package on this one, as there is no package definition for it. This is why it is not part of the required components.
I found it less clear to append it to the QT_REQUIRED_COMPONENT after their initial use, let me know if you disagree.
Autotools don't use the prl files at all, all the dependencies are hard coded and added manually.

165 ↗(On Diff #8256)

There is an #if defined(QT_STATICPLUGIN) in the code to include <QTPlugin>. Cmake has no way to determine this.

178 ↗(On Diff #8256)

See my comment above.

deadalnix requested changes to this revision.Apr 24 2019, 21:42
deadalnix added inline comments.
src/qt/CMakeLists.txt
32 ↗(On Diff #8256)

I still do not know what previous assumption was wrong. It is however highly suspicious that leaving Qt's install tree is ever a good idea, because by definition you aren't in Qt there anymore are are making assumption as to what, at least, the top folder is.

This is also making the assumptions that you can rely on internal variable from Qt which previous use is clearly marked with

  1. HACK: We must explicitly add LIB path of the Qt installation
  2. to correctly find qtpcre

Which isn't exactly a brilliant endorsement of the practice.

I would keep arguing here but you already *KNOW* that all of this is wrong because you commented on it by yourself. So do the right thing.

46 ↗(On Diff #8256)

Here is your problem here. You notice bitcoin-qt doesn't work on win32. You try to fix it.

That's good except, bitcoin-qt work on win32 on the good old build system. Therefore, porting that good old build system, or at least the missing part, is the way to go, unless you want to be playing "where is waldo?" trying to figure out why both build do not match in obscure conditions.

Note that i already pointed to this in https://reviews.bitcoinabc.org/D2667?id=7683#inline-16405 .

165 ↗(On Diff #8256)

I could get to the bottom of this by grepping into the autotool build, but we are looping back to https://reviews.bitcoinabc.org/D2667#inline-17832 and code review shouldn't involve playing Where is Waldo.

This revision now requires changes to proceed.Apr 24 2019, 21:42