Page MenuHomePhabricator

[CMAKE] Add support for Miniupnpc
Needs ReviewPublic

Authored by Fabien on Wed, May 8, 19:22.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

This diff adds support for UPnP and an option to disable it by setting
-DENABLE_UPNP=OFF on the cmake command line.

It also adds an option START_WITH_UPNP which can be turned ON to set
UPnP as the default to map ports (OFF by default).

Test Plan

With libminiupnpc installed:

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

Go to Settings->Options...
Click on the Network tab.
Check the Map port using UPnP checkbox is dimmed.
Close bitcoin-qt.

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

Go to Settings->Options...
Click on the Network tab.
Check the Map port using UPnP checkbox is not dimmed.
Uncheck Map port using UPnP and close bitcoin-qt.

Remove the line fUseUPnP=false from ~/.config/BitcoinABC/BitcoinABC-Qt.conf.

rm -rf *
cmake -GNinja -DSTART_WITH_UPNP=ON ..
ninja
./src/qt/bitcoin-qt

Go to Settings->Options...
Click on the Network tab.
Check the Map port using UPnP checkbox is not dimmed and is checked.
Uncheck Map port using UPnP and close bitcoin-qt.

Uninstall libminiupnpc. On Debian sudo apt remove libminiupnpc*

rm -rf *
cmake -GNinja ..

Should return a CMake Error:

Could NOT find MiniUPnPc (missing: MINIUPNPC_INCLUDE_DIR MINIUPNPC_LIBRARY)

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cmake_miniupnpc
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5882
Build 9824: Bitcoin ABC Teamcity Staging
Build 9823: arc lint + arc unit

Event Timeline

Fabien created this revision.Wed, May 8, 19:22
Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, May 8, 19:22
Fabien planned changes to this revision.Thu, May 9, 09:44
Fabien updated this revision to Diff 8568.Thu, May 9, 19:35

Fix the windows case.
Refactor to match autotools (use configure, add the option to enable by default).

Fabien edited the summary of this revision. (Show Details)Thu, May 9, 19:36
Fabien edited the test plan for this revision. (Show Details)
Fabien updated this revision to Diff 8626.Mon, May 13, 09:44

Add missing include directory.

deadalnix requested changes to this revision.Thu, May 16, 17:06
deadalnix added inline comments.
cmake/modules/FindMiniUPnPc.cmake
12 ↗(On Diff #8626)

Do not make the build depend on environment more than it needs to.

src/CMakeLists.txt
23 ↗(On Diff #8626)

That is a bad idea to activate or deactivate options based on our buggy software.

27 ↗(On Diff #8626)

This is a very bad option name. It impossible to get a sense of what it does without the comment.

323 ↗(On Diff #8626)

Presumably, this library has headers somewhere and comes with an include path.

src/config/CMakeLists.txt
215 ↗(On Diff #8626)

No auto-detection.

This revision now requires changes to proceed.Thu, May 16, 17:06
Fabien edited the summary of this revision. (Show Details)Fri, May 17, 13:20
Fabien edited the test plan for this revision. (Show Details)
Fabien updated this revision to Diff 8712.Fri, May 17, 13:26

Get rid of the AUTO option.
Attempt to make the option name more meaningful.

Fabien planned changes to this revision.Fri, May 17, 13:26
Fabien added inline comments.
src/CMakeLists.txt
323 ↗(On Diff #8626)

It's OK because we are not building miniupnpc, only the symbols are required for linkage.

Fabien requested review of this revision.Fri, May 17, 13:27
deadalnix added inline comments.Tue, May 21, 23:15
src/config/bitcoin-config.h.cmake.in
67

WTF oO ?

Fabien added inline comments.Wed, May 22, 10:14
src/config/bitcoin-config.h.cmake.in
67

This is for compatibility with autotools. Yes, #define USE_UPNP 0 enables UPnP...

This is used in several places in the code, and impacts the public API (see the -upnp option in init.cpp), so I preferred to remain compatible.