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

hebasto · 2024-04-21T18:46:53Z

This PR partially pulled from #83 with options, which default values seem non-controversial.

As a bonus, the help string duplication in depends/toolchain.cmake.in has been eliminated for the touched lines.

The Autotools vs CMake Feature Parity Table has been updated as well.

vasild

Approach ACK d66f911

vasild · 2024-04-22T10:44:13Z

.github/workflows/cmake.yml

@@ -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 -DWITH_GUI=OFF -DBoost_INCLUDE_DIR="${PWD}/${{ matrix.conf.boost_archive }}"


I would expect commit a12b692 cmake: Add CI-specific presets to just move options from the command line to the presets file, however this is not the case. I guess this is unintentional, or the commit message needs to be more elaborate on why those changes are made:

removed from the command line, but not added to the presets file:

-DCMAKE_C_COMPILER=gcc-11

-DCMAKE_CXX_COMPILER=g++-11

-DENABLE_WALLET=ON

added to the command line, not in the presets file before or after this commit (for Linux):

-DWITH_GUI=OFF -- why not have it in the ci-linux preset?

I would expect commit a12b692 cmake: Add CI-specific presets to just move options from the command line to the presets file, however this is not the case.

Thanks! Reworked to meet expectations.

Thanks! Do you think it makes sense to have all options in the presents file, so that they are not scattered between the command line and the presets file? Or would that be too un-flexible?

I'm not sure. However, it won't affect flexibility, because there will be an opportunity to override any option in the command line.

depends/toolchain.cmake.in

vasild · 2024-04-22T11:17:54Z

depends/toolchain.cmake.in

+if("@no_upnp@")
+  set(WITH_MINIUPNPC OFF CACHE BOOL "")


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.

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.

The `HARDENING` is a boolean option, not a string type.

vasild

ACK df4e46b

fanquake · 2024-04-24T09:11:29Z

CMakeLists.txt

-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)


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

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

fanquake · 2024-04-24T09:11:53Z

CMakeLists.txt

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


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

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

fanquake · 2024-04-24T09:12:57Z

cmake/optional.cmake

@@ -189,12 +135,13 @@ else()
 endif()

 if(WITH_GUI AND WITH_QRENCODE)
-  if(MSVC)
+  if(VCPKG_TARGET_TRIPLET)


Why was this changed?

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

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

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.

5de23c7 fixup! cmake: Add preset for MSVC build (Hennadii Stepanov) 09b1d5b fixup! ci: Test CMake edge cases (Hennadii Stepanov) 820a538 cmake [KILL 3-STATE]: Switch `WITH_QRENCODE` to boolean w/ default ON (Hennadii Stepanov) 017aeda cmake [FIXUP]: Move `find_program(brew)` out from introspection file (Hennadii Stepanov) Pull request description: This PR is a continuation of #161, #162 and #164 and tackles with the `WITH_QRENCODE` options. It becomes enabled when building GUI with the default value `ON`. Top commit has no ACKs. Tree-SHA512: 9cd923ef89e0a485330205db4389e42c001cbd6683a14b1e0c665d8e790e8bb2cae7b8b1929818443aea8eb5cbf6cafa79f4e44f41a893f54a5cdc09abc49dc1

598eda8 cmake [KILL 3-STATE]: Switch `WITH_USDT` to boolean w/ default OFF (Hennadii Stepanov) f3c2fea fixup! cmake: Add `systemtap-sdt` optional package support (Hennadii Stepanov) Pull request description: Same as #161, but USDT-specific with refactoring of the USDT search logic into its own "FindUSDT" module. The first commit is a pure refactoring. Top commit has no ACKs. Tree-SHA512: 2d67c7e72509fc78eb906a8af93e7525b8125bdeb38a5f45ceda1c0c75f300c74501402c39d8d17de2585b2ed2939de0009a33231d1164438ce929236d4b742a

2b4ad50 cmake [KILL 3-STATE]: Switch `WITH_{SQLITE,BDB}` to boolean (Hennadii Stepanov) Pull request description: This PR is a continuation of #161 and #162 and tackles with the `WITH_SQLITE` and `WITH_BDB` options. For the latter the default value `OFF` is suggested. The #83 (comment): > Shouldn't `WITH_SQLITE=ON` imply `ENABLE_WALLET`? What (extra) does `ENABLE_WALLET` do here? I'm not sure we'll actually want to keep it as an option. has been addressed. ACKs for top commit: TheCharlatan: ACK 2b4ad50 Tree-SHA512: 13d5bd16709ab557ee736696fe1d5d3fe64bae8cb77c000401c26b8be2deab0473474f7fa9bb958884ba3ab9c4d0b66e44766ca5c1c86d0fab3957642ec996b9

fixup! cmake: Add libqrencode optional package support

8fe37b0

hebasto mentioned this pull request Apr 21, 2024

[FUTURE] cmake: Switch from tri-state options to ON/OFF #83

Closed

hebasto force-pushed the 240421-cmake-DV branch from 2c93976 to d66f911 Compare April 21, 2024 19:01

hebasto added the enhancement New feature or request label Apr 21, 2024

hebasto mentioned this pull request Apr 22, 2024

cmake: Switch from tri-state options to boolean. Stage TWO #162

Merged

vasild reviewed Apr 22, 2024

View reviewed changes

hebasto mentioned this pull request Apr 22, 2024

cmake: Fix libqrencode optional package support #160

Merged

hebasto added 6 commits April 22, 2024 13:26

cmake: Add CI-specific presets

63bbe26

fixup! build: Generate share/toolchain.cmake in depends

dc20cdf

The `HARDENING` is a boolean option, not a string type.

cmake [KILL 3-STATE]: Switch WITH_NATPMP to boolean w/ default OFF

a4d6a0c

cmake [KILL 3-STATE]: Switch WITH_MINIUPNPC to boolean w/ default OFF

5bfecf4

cmake [KILL 3-STATE]: Switch WITH_ZMQ to boolean w/ default OFF

732b4e9

cmake [KILL 3-STATE]: Switch MULTIPROCESS to boolean w/ default OFF

df4e46b

hebasto force-pushed the 240421-cmake-DV branch from d66f911 to df4e46b Compare April 22, 2024 12:27

vasild approved these changes Apr 22, 2024

View reviewed changes

hebasto merged commit a012404 into cmake-staging Apr 22, 2024
30 checks passed

hebasto mentioned this pull request Apr 22, 2024

cmake: Switch from tri-state options to boolean. Stage THREE #164

Merged

fanquake reviewed Apr 24, 2024

View reviewed changes

hebasto mentioned this pull request Apr 24, 2024

cmake: Switch from tri-state options to boolean. Stage FOUR #169

Merged

hebasto added this to the UI, 23th May milestone May 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hebasto commented Apr 21, 2024

vasild left a comment

vasild Apr 22, 2024

hebasto Apr 22, 2024

vasild Apr 22, 2024

hebasto Apr 22, 2024

vasild Apr 22, 2024

hebasto Apr 22, 2024

vasild Apr 22, 2024

vasild left a comment

fanquake Apr 24, 2024

hebasto Apr 24, 2024

fanquake Apr 24, 2024

hebasto Apr 24, 2024

fanquake Apr 24, 2024

hebasto Apr 24, 2024

fanquake Apr 24, 2024

hebasto Apr 24, 2024

	-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)\|' \

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

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

Conversation

hebasto commented Apr 21, 2024

vasild left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasild left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment