[CMAKE] Add support for libqrencode
ClosedPublic
Actions

Authored by Fabien on May 10 2019, 08:50.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

Summary

This diff adds an option to enable the QR code display (in the payment
request dialog).

The QR code is enabled by default and can be disabled by setting
-DENABLE_QRCODE=OFF on the cmake command line.

If either QT or the wallet is not built, the option is not used.

Test Plan

With libqrencode installed:

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

Go the the Receive tab and click the Request payment button.
A QR code should be displayed.

rm -rf *
cmake -GNinja -DENABLE_QRCODE=OFF ..
ninja
./src/qt/bitcoin-qt

Go the the Receive tab and click the `` button.
The QR code should not be displayed.

Uninstall libqrencode. On Debian: sudo apt remove libqrencode*

rm -rf *
cmake -GNinja ..

CMake should output an error:

Could NOT find QREncode (missing: QRENCODE_INCLUDE_DIR QRENCODE_LIBRARY)

rm -rf *
cmake -GNinja .. -DBUILD_BITCOIN_WALLET=OFF
ninja

The build should be successful.

Repository

rABC Bitcoin ABC

Branch

cmake_qrencode

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 5880
Build 9820: Bitcoin ABC Buildbot (legacy)
Build 9819: arc lint + arc unit

This invent a new way to configure things for no apparent reason.

cmake/modules/FindQREncode.cmake
12 ↗	(On Diff #8585)	why ?
src/CMakeLists.txt
24 ↗	(On Diff #8585)	Setting things on auto is almost never a good idea. It might as well be called "do whatever the build system want to do today".
src/qt/CMakeLists.txt
185 ↗	(On Diff #8585)	Presumably, you need to add the proper include path.

This revision now requires changes to proceed.May 12 2019, 22:21

Fix missing include directory.

cmake/modules/FindQREncode.cmake
12 ↗	(On Diff #8585)	It allows to specify the location using an environment variable in the case it is not installed in a standard directory.
src/CMakeLists.txt
24 ↗	(On Diff #8585)	I did so to match the autotools behavior (so does miniupnpc). If optional dependencies are unwanted, it can be updated to a simpler ON/OFF option (I think default to ON is the best choice here).

deadalnix requested changes to this revision.May 16 2019, 23:05

deadalnix added inline comments.

cmake/modules/FindQREncode.cmake
12 ↗	(On Diff #8585)	A system wide global is about the only thing worse that a program wide global though. None of the other files do this and I'm fairly confident that this is possible to configure cmake to look into various repositories - in fact it's done to support brew in various places already.
src/CMakeLists.txt
24 ↗	(On Diff #8585)	AUTO is almost never desired. If I want the feature, then I want cmake to tell me it won't work, not randomly deactivate it. If I don't want the feature, then I can disable it. Why would I want cmake to pick a pseudo-random set of feature and build that ? You introduced a significant amount of logic and scaffolding to introduce that behavior rather than a simple on/off switch and I don't see any good reason for that anywhere. Unless there is something I'm missing, this is simply backporting tech debt from configure to cmake, which is not the most useful achievement.