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).
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project - Commits
- rSTAGINGc234874c5988: [CMAKE] Add support for Miniupnpc
rABCc234874c5988: [CMAKE] Add support for Miniupnpc
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
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- cmake_miniupnpc
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 5882 Build 9824: Bitcoin ABC Buildbot (legacy) Build 9823: arc lint + arc unit
Event Timeline
Comment Actions
Fix the windows case.
Refactor to match autotools (use configure, add the option to enable by default).
| 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. |
| src/CMakeLists.txt | ||
|---|---|---|
| 323 ↗ | (On Diff #8626) | It's OK because we are not building miniupnpc, only the symbols are required for linkage. |
| src/config/bitcoin-config.h.cmake.in | ||
|---|---|---|
| 67 | WTF oO ? | |
| 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. | |
| src/CMakeLists.txt | ||
|---|---|---|
| 314 | I can't find what this corresponds to. | |
| src/config/CMakeLists.txt | ||
| 166 | Shouldn't this be in the find mini upnp thing ? | |
| 171 | What is this doing ? | |
| 198 | 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. | |
| src/CMakeLists.txt | ||
|---|---|---|
| 314 | 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. | |
| src/config/CMakeLists.txt | ||
| 166 | 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 | Ensuring the cache won't prevent the check to work | |
Comment Actions
Move header check to the find file.
Avoid using a cmake hack to define a tristate definition.