Page MenuHomePhabricator

[CMAKE] Add support for Miniupnpc
ClosedPublic

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABCc234874c5988: [CMAKE] Add support for Miniupnpc
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
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 8 2019, 19:22
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 8 2019, 19:22
Fabien planned changes to this revision.May 9 2019, 09:44
Fabien updated this revision to Diff 8568.May 9 2019, 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)May 9 2019, 19:36
Fabien edited the test plan for this revision. (Show Details)
Fabien updated this revision to Diff 8626.May 13 2019, 09:44

Add missing include directory.

deadalnix requested changes to this revision.May 16 2019, 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.May 16 2019, 17:06
Fabien edited the summary of this revision. (Show Details)May 17 2019, 13:20
Fabien edited the test plan for this revision. (Show Details)
Fabien updated this revision to Diff 8712.May 17 2019, 13:26

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 ↗(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.May 17 2019, 13:27
deadalnix added inline comments.May 21 2019, 23:15
src/config/bitcoin-config.h.cmake.in
67 ↗(On Diff #8712)

WTF oO ?

Fabien added inline comments.May 22 2019, 10:14
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
Fabien added inline comments.May 27 2019, 09:06
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

Fabien updated this revision to Diff 8907.May 27 2019, 09:07

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

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