Page MenuHomePhabricator

[CMAKE] Add support for libqrencode

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



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

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

rm -rf *
cmake -GNinja -DENABLE_QRCODE=OFF ..

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:

rm -rf *

The build should be successful.

Diff Detail

rABC Bitcoin ABC
Lint OK
No Unit Test Coverage
Build Status
Buildable 5825
Build 9712: Bitcoin ABC Buildbot (legacy)
Build 9711: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.May 12 2019, 22:21

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

12 ↗(On Diff #8585)

why ?

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

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.

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.

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

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.

This revision now requires changes to proceed.May 16 2019, 23:05
Fabien planned changes to this revision.May 17 2019, 10:27
Fabien edited the test plan for this revision. (Show Details)
Fabien edited the summary of this revision. (Show Details)

Remove AUTO auto and environment variables.

This revision is now accepted and ready to land.May 18 2019, 20:37
This revision was automatically updated to reflect the committed changes.