Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: Switch from tri-state options to boolean. Stage ONE #161

Merged
merged 7 commits into from
Apr 22, 2024
Merged

cmake: Switch from tri-state options to boolean. Stage ONE #161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8fe37b0
fixup! cmake: Add `libqrencode` optional package support
hebasto Apr 21, 2024
63bbe26
cmake: Add CI-specific presets
hebasto Apr 22, 2024
dc20cdf
fixup! build: Generate `share/toolchain.cmake` in depends
hebasto Apr 21, 2024
a4d6a0c
cmake [KILL 3-STATE]: Switch `WITH_NATPMP` to boolean w/ default OFF
hebasto Apr 21, 2024
5bfecf4
cmake [KILL 3-STATE]: Switch `WITH_MINIUPNPC` to boolean w/ default OFF
hebasto Apr 21, 2024
732b4e9
cmake [KILL 3-STATE]: Switch `WITH_ZMQ` to boolean w/ default OFF
hebasto Apr 21, 2024
df4e46b
cmake [KILL 3-STATE]: Switch `MULTIPROCESS` to boolean w/ default OFF
hebasto Apr 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/cmake.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ jobs:

- name: Generate build system
run: |
cmake -B build -DCMAKE_C_COMPILER=gcc-11 -DCMAKE_CXX_COMPILER=g++-11 -DBoost_INCLUDE_DIR="${PWD}/${{ matrix.conf.boost_archive }}" -DENABLE_WALLET=ON -DWITH_EXTERNAL_SIGNER=ON -DWITH_MINIUPNPC=ON -DWITH_NATPMP=ON -DWITH_ZMQ=ON -DWITH_USDT=ON -DWERROR=ON
cmake -B build --preset ci-linux -DCMAKE_C_COMPILER=gcc-11 -DCMAKE_CXX_COMPILER=g++-11 -DBoost_INCLUDE_DIR="${PWD}/${{ matrix.conf.boost_archive }}"

- name: Build
working-directory: build
Expand Down Expand Up @@ -576,7 +576,7 @@ jobs:

- name: Generate build system
run: |
${{ matrix.xcode.configure_env }} cmake -B build -DWERROR=ON
${{ matrix.xcode.configure_env }} cmake -B build --preset ci-darwin

- name: Build
env:
Expand Down
36 changes: 32 additions & 4 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,32 @@ option(REDUCE_EXPORTS "Attempt to reduce exported symbols in the resulting execu
option(WERROR "Treat compiler warnings as errors." OFF)

tristate_option(CCACHE "Use ccache for compiling." "if ccache is found." AUTO)
tristate_option(WITH_NATPMP "Enable NAT-PMP." "if libnatpmp is found." AUTO)
tristate_option(WITH_MINIUPNPC "Enable UPnP." "if libminiupnpc is found." AUTO)
tristate_option(WITH_ZMQ "Enable ZMQ notifications." "if libzmq is found." AUTO)

option(WITH_NATPMP "Enable NAT-PMP." OFF)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't we meant to be reming these, or is that happening at a later point?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm going to rename all options at once at a later point.

if(WITH_NATPMP)
find_package(NATPMP MODULE REQUIRED)
endif()

option(WITH_MINIUPNPC "Enable UPnP." OFF)
if(WITH_MINIUPNPC)
find_package(MiniUPnPc MODULE REQUIRED)
endif()

option(WITH_ZMQ "Enable ZMQ notifications." OFF)
if(WITH_ZMQ)
if(VCPKG_TARGET_TRIPLET)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is vcpkg being carved out here? Can just use the same logic for both until we can use the new logic?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The find_package(ZeroMQ CONFIG) invocation is recommended by the vcpkg. I didn't test pkg-config approach for it.

find_package(ZeroMQ CONFIG REQUIRED)
else()
# The ZeroMQ project has provided config files since v4.2.2.
# TODO: Switch to find_package(ZeroMQ) at some point in the future.
include(CrossPkgConfig)
cross_pkg_check_modules(libzmq REQUIRED IMPORTED_TARGET libzmq>=4)
target_link_libraries(PkgConfig::libzmq INTERFACE
$<$<PLATFORM_ID:Windows>:iphlpapi;ws2_32>
)
endif()
endif()

tristate_option(WITH_USDT
"Enable tracepoints for Userspace, Statically Defined Tracing."
"if sys/sdt.h is found."
Expand All @@ -84,7 +107,12 @@ tristate_option(WITH_EXTERNAL_SIGNER
AUTO
)
tristate_option(WITH_QRENCODE "Enable QR code support." "if libqrencode is found." AUTO)
tristate_option(MULTIPROCESS "Build multiprocess bitcoin-node, bitcoin-wallet, and bitcoin-gui executables in addition to monolithic bitcoind and bitcoin-qt executables. Requires libmultiprocess library. Experimental." "if libmultiprocess is found." OFF)

option(MULTIPROCESS "Build multiprocess bitcoin-node, bitcoin-wallet, and bitcoin-gui executables in addition to monolithic bitcoind and bitcoin-qt executables. Requires libmultiprocess library. Experimental." OFF)
if(MULTIPROCESS)
find_package(Libmultiprocess CONFIG REQUIRED)
find_package(LibmultiprocessGen CONFIG REQUIRED)
endif()

option(BUILD_TESTS "Build test_bitcoin executable." ON)
cmake_dependent_option(BUILD_GUI_TESTS "Build test_bitcoin-qt executable." ON "BUILD_TESTS" OFF)
Expand Down
42 changes: 42 additions & 0 deletions CMakePresets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,48 @@
"version": 3,
"cmakeMinimumRequired": {"major": 3, "minor": 21, "patch": 0},
"configurePresets": [
{
"name": "ci-common",
"hidden": true,
"cacheVariables": {
"ENABLE_WALLET": "ON",
"WITH_SQLITE": "ON",
"WITH_BDB": "ON",
"WITH_NATPMP": "ON",
"WITH_MINIUPNPC": "ON",
"WITH_ZMQ": "ON",
"WERROR": "ON",
"CCACHE": "ON"
}
},
{
"name": "ci-linux",
"inherits": "ci-common",
"displayName": "Build for CI tests on Linux",
"condition": {
"type": "equals",
"lhs": "${hostSystemName}",
"rhs": "Linux"
},
"cacheVariables": {
"WITH_EXTERNAL_SIGNER": "ON",
"WITH_USDT": "ON"
}
},
{
"name": "ci-darwin",
"inherits": "ci-common",
"displayName": "Build for CI tests on macOS",
"condition": {
"type": "equals",
"lhs": "${hostSystemName}",
"rhs": "Darwin"
},
"cacheVariables": {
"WITH_GUI": "Qt5",
"WITH_EXTERNAL_SIGNER": "ON"
}
},
{
"name": "vs2022",
"displayName": "Build using 'Visual Studio 17 2022' generator and 'x64-windows' triplet",
Expand Down
71 changes: 3 additions & 68 deletions cmake/optional.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,60 +50,6 @@ if(CCACHE)
mark_as_advanced(CCACHE_COMMAND)
endif()

if(WITH_NATPMP)
find_package(NATPMP MODULE)
if(NATPMP_FOUND)
set(WITH_NATPMP ON)
elseif(WITH_NATPMP STREQUAL "AUTO")
message(WARNING "libnatpmp not found, disabling.\n"
"To skip libnatpmp check, use \"-DWITH_NATPMP=OFF\".\n")
set(WITH_NATPMP OFF)
else()
message(FATAL_ERROR "libnatpmp requested, but not found.")
endif()
endif()

if(WITH_MINIUPNPC)
find_package(MiniUPnPc MODULE)
if(MiniUPnPc_FOUND)
set(WITH_MINIUPNPC ON)
elseif(WITH_MINIUPNPC STREQUAL "AUTO")
message(WARNING "libminiupnpc not found, disabling.\n"
"To skip libminiupnpc check, use \"-DWITH_MINIUPNPC=OFF\".\n")
set(WITH_MINIUPNPC OFF)
else()
message(FATAL_ERROR "libminiupnpc requested, but not found.")
endif()
endif()

if(WITH_ZMQ)
if(MSVC)
find_package(ZeroMQ CONFIG)
else()
# The ZeroMQ project has provided config files since v4.2.2.
# TODO: Switch to find_package(ZeroMQ) at some point in the future.
include(CrossPkgConfig)
cross_pkg_check_modules(libzmq IMPORTED_TARGET libzmq>=4)
if(libzmq_FOUND AND TARGET PkgConfig::libzmq)
target_compile_definitions(PkgConfig::libzmq INTERFACE
$<$<PLATFORM_ID:Windows>:ZMQ_STATIC>
)
target_link_libraries(PkgConfig::libzmq INTERFACE
$<$<PLATFORM_ID:Windows>:iphlpapi;ws2_32>
)
endif()
endif()
if(TARGET libzmq OR TARGET PkgConfig::libzmq)
set(WITH_ZMQ ON)
elseif(WITH_ZMQ STREQUAL "AUTO")
message(WARNING "libzmq not found, disabling.\n"
"To skip libzmq check, use \"-DWITH_ZMQ=OFF\".\n")
set(WITH_ZMQ OFF)
else()
message(FATAL_ERROR "libzmq requested, but not found.")
endif()
endif()

if(WITH_USDT)
find_path(SystemTap_INCLUDE_DIR
NAMES sys/sdt.h
Expand Down Expand Up @@ -189,12 +135,13 @@ else()
endif()

if(WITH_GUI AND WITH_QRENCODE)
if(MSVC)
if(VCPKG_TARGET_TRIPLET)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this changed?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes an intention clearer because the logic is specific to vcpkg package manager, not to the MSVC compiler.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is anyone using MSVC without vcpkg? Is this being done consistently?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is anyone using MSVC without vcpkg?

I don't think so. But vcpkg might be used on other OSes.

Is this being done consistently?

I hope it is consistent now in the staging branch.

# TODO: vcpkg fails to build libqrencode package due to
# the build error in its libiconv dependency.
# See: https://github.com/microsoft/vcpkg/issues/36924.
else()
cross_pkg_check_modules(libqrencode libqrencode IMPORTED_TARGET)
include(CrossPkgConfig)
cross_pkg_check_modules(libqrencode IMPORTED_TARGET libqrencode)
endif()
if(TARGET PkgConfig::libqrencode)
set_target_properties(PkgConfig::libqrencode PROPERTIES
Expand All @@ -207,15 +154,3 @@ if(WITH_GUI AND WITH_QRENCODE)
message(FATAL_ERROR "libqrencode requested, but not found.")
endif()
endif()

if(MULTIPROCESS)
find_package(Libmultiprocess CONFIG)
find_package(LibmultiprocessGen CONFIG)
if(TARGET Libmultiprocess::multiprocess AND TARGET Libmultiprocess::mpgen)
set(MULTIPROCESS ON)
elseif(MULTIPROCESS STREQUAL "AUTO")
set(MULTIPROCESS OFF)
else()
message(FATAL_ERROR "\"-DMULTIPROCESS=ON\" specified, but libmultiprocess library was not found.")
endif()
endif()
34 changes: 20 additions & 14 deletions depends/toolchain.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,10 @@ if(NOT WITH_QRENCODE AND "@no_qr@" STREQUAL "1")
set(WITH_QRENCODE OFF CACHE STRING "Enable QR code support.")
endif()

if(NOT WITH_ZMQ AND "@no_zmq@" STREQUAL "1")
set(WITH_ZMQ OFF CACHE STRING "Enable ZMQ notifications.")
if("@no_zmq@")
set(WITH_ZMQ OFF CACHE BOOL "")
else()
set(WITH_ZMQ ON CACHE BOOL "")
endif()

if(NOT ENABLE_WALLET AND "@no_wallet@" STREQUAL "1")
Expand All @@ -138,27 +140,31 @@ if(NOT WITH_SQLITE AND "@no_sqlite@" STREQUAL "1")
set(WITH_SQLITE OFF CACHE STRING "Enable SQLite wallet support.")
endif()

if(NOT WITH_MINIUPNPC AND "@no_upnp@" STREQUAL "1")
set(WITH_MINIUPNPC OFF CACHE STRING "Enable UPnP.")
if("@no_upnp@")
set(WITH_MINIUPNPC OFF CACHE BOOL "")
Comment on lines +143 to +144
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how those "@no_foo@" are generated, but there is some inconsistency between upnp and the others. For example, CMakeLists.txt defines WITH_NATPMP and in depends/toolchain.cmake.in the check is using no_natpmp. But for upnp: CMakeLists.txt: WITH_MINIUPNPC, depends/toolchain.cmake.in: no_upnp.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how those "@no_foo@" are generated...

bitcoin/depends/Makefile

Lines 288 to 298 in d66f911

-e 's|@no_qt@|$(NO_QT)|' \
-e 's|@no_qr@|$(NO_QR)|' \
-e 's|@no_zmq@|$(NO_ZMQ)|' \
-e 's|@no_wallet@|$(NO_WALLET)|' \
-e 's|@no_bdb@|$(NO_BDB)|' \
-e 's|@no_sqlite@|$(NO_SQLITE)|' \
-e 's|@no_upnp@|$(NO_UPNP)|' \
-e 's|@no_natpmp@|$(NO_NATPMP)|' \
-e 's|@no_usdt@|$(NO_USDT)|' \
-e 's|@no_harden@|$(NO_HARDEN)|' \
-e 's|@multiprocess@|$(MULTIPROCESS)|' \

... but there is some inconsistency between upnp and the others. For example, CMakeLists.txt defines WITH_NATPMP and in depends/toolchain.cmake.in the check is using no_natpmp. But for upnp: CMakeLists.txt: WITH_MINIUPNPC, depends/toolchain.cmake.in: no_upnp.

You mean, WITH_MINIUPNPC should be renamed to WITH_UPNP?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean, WITH_MINIUPNPC should be renamed to WITH_UPNP?

Either this or change the above to -e 's|@no_miniupnpc@|$(NO_MINIUPNPC)|'. No strong opinion, but it is nice to be more consistent.

else()
set(WITH_MINIUPNPC ON CACHE BOOL "")
endif()

if(NOT WITH_NATPMP AND "@no_natpmp@" STREQUAL "1")
set(WITH_NATPMP OFF CACHE STRING "Enable NAT-PMP.")
if("@no_natpmp@")
set(WITH_NATPMP OFF CACHE BOOL "")
else()
set(WITH_NATPMP ON CACHE BOOL "")
endif()

if(NOT WITH_USDT AND "@no_usdt@" STREQUAL "1")
set(WITH_USDT OFF CACHE STRING "Enable tracepoints for Userspace, Statically Defined Tracing.")
endif()

if(NOT HARDENING AND "@no_harden@" STREQUAL "1")
set(HARDENING OFF CACHE STRING "Attempt to harden the resulting executables.")
if("@no_harden@")
set(HARDENING OFF CACHE BOOL "")
else()
set(HARDENING ON CACHE BOOL "")
endif()

if("@multiprocess@" STREQUAL "1")
if(NOT MULTIPROCESS)
set(MULTIPROCESS ON CACHE STRING "")
endif()
if(NOT LibmultiprocessGen_DIR)
set(LibmultiprocessGen_DIR "${CMAKE_FIND_ROOT_PATH}/native/lib/cmake/LibmultiprocessGen" CACHE PATH "")
endif()
set(MULTIPROCESS ON CACHE BOOL "")
set(LibmultiprocessGen_DIR "${CMAKE_FIND_ROOT_PATH}/native/lib/cmake/LibmultiprocessGen" CACHE PATH "")
else()
set(MULTIPROCESS OFF CACHE BOOL "")
endif()
2 changes: 1 addition & 1 deletion src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ target_link_libraries(bitcoin_consensus
)

if(WITH_ZMQ)
add_subdirectory(zmq EXCLUDE_FROM_ALL)
add_subdirectory(zmq)
endif()

# Home for common functionality shared by different executables and libraries.
Expand Down
4 changes: 3 additions & 1 deletion src/zmq/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# Distributed under the MIT software license, see the accompanying
# file COPYING or https://opensource.org/license/mit/.

add_library(bitcoin_zmq STATIC
add_library(bitcoin_zmq STATIC EXCLUDE_FROM_ALL
zmqabstractnotifier.cpp
zmqnotificationinterface.cpp
zmqpublishnotifier.cpp
Expand All @@ -12,6 +12,8 @@ add_library(bitcoin_zmq STATIC
target_compile_definitions(bitcoin_zmq
INTERFACE
ENABLE_ZMQ=1
PRIVATE
$<$<AND:$<PLATFORM_ID:Windows>,$<CXX_COMPILER_ID:GNU>>:ZMQ_STATIC>
)
target_link_libraries(bitcoin_zmq
PRIVATE
Expand Down
Loading