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 5826 Build 9714: Bitcoin ABC Buildbot (legacy) Build 9713: 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 | 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. | |
| src/CMakeLists.txt | ||
|---|---|---|
| 323 | It's OK because we are not building miniupnpc, only the symbols are required for linkage. | |
| 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. |
| 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. |
| 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. |
| 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 |
Comment Actions
Move header check to the find file.
Avoid using a cmake hack to define a tristate definition.