Page MenuHomePhabricator

[CMAKE] Add support for Miniupnpc
ClosedPublic

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

Details

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 Passed
Unit
No Test Coverage
Build Status
Buildable 5826
Build 9714: Bitcoin ABC Buildbot (legacy)
Build 9713: arc lint + arc unit

Event Timeline

Fabien planned changes to this revision.May 9 2019, 09:44

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

Fabien edited the test plan for this revision. (Show Details)

Add missing include directory.

deadalnix requested changes to this revision.May 16 2019, 17:06
deadalnix added inline comments.
cmake/modules/FindMiniUPnPc.cmake
12

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

src/CMakeLists.txt
23

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

27

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

323

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

src/config/CMakeLists.txt
215

No auto-detection.

This revision now requires changes to proceed.May 16 2019, 17:06
Fabien edited the test plan for this revision. (Show Details)

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

Fabien planned changes to this revision.May 17 2019, 13:26
Fabien added inline comments.
src/CMakeLists.txt
323

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

Fabien requested review of this revision.May 17 2019, 13:27
src/config/bitcoin-config.h.cmake.in
67 ↗(On Diff #8712)

WTF oO ?

src/config/bitcoin-config.h.cmake.in
67 ↗(On Diff #8712)

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.

deadalnix requested changes to this revision.May 24 2019, 18:24
deadalnix added inline comments.
src/CMakeLists.txt
314 ↗(On Diff #8712)

I can't find what this corresponds to.

src/config/CMakeLists.txt
166 ↗(On Diff #8712)

Shouldn't this be in the find mini upnp thing ?

171 ↗(On Diff #8712)

What is this doing ?

198 ↗(On Diff #8712)

You could simply wrap the #define USE_UNP in the config file so that it doesn't get defined when some other variable isn't istead of suing a bag of trick that's going to break once someone use a different version fo cmake or gcc.

This revision now requires changes to proceed.May 24 2019, 18:24
src/CMakeLists.txt
314 ↗(On Diff #8712)

miniupnpc requires 2 librairies for mingw: ws2_32 and libhlpapi. The first one is already included because used in our code (winsock2.h in src/compat.h), while the later one is not used in our code.
Autotools does link a lot of windows libraries including libhlpapi, but does not check whether it is needed or not.

src/config/CMakeLists.txt
166 ↗(On Diff #8712)

I was hesitant about this, but as long as there is no use case for the lib without these headers this should be fine to move it to the find file.

171 ↗(On Diff #8712)

Ensuring the cache won't prevent the check to work

Move header check to the find file.
Avoid using a cmake hack to define a tristate definition.

This revision is now accepted and ready to land.May 27 2019, 16:05
This revision was automatically updated to reflect the committed changes.