Page MenuHomePhabricator

[CMAKE] Add support for libqrencode
ClosedPublic

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABC07d4643cc6a5: [CMAKE] Add support for libqrencode
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.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.May 10 2019, 08:50
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 10 2019, 08:50
deadalnix requested changes to this revision.May 12 2019, 22:21

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
Fabien updated this revision to Diff 8625.May 13 2019, 09:37

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.

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 summary of this revision. (Show Details)May 17 2019, 10:42
Fabien edited the test plan for this revision. (Show Details)
Fabien edited the summary of this revision. (Show Details)
Fabien updated this revision to Diff 8710.May 17 2019, 10:48

Remove AUTO auto and environment variables.

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