cmake: Consistent build option naming #198

hebasto · 2024-05-12T08:28:24Z

The build option naming convention

BUILD_* options control what binaries and libraries are built.
ENABLE_* options control what features are turned on.
If a feature is fully implemented in a standalone binary, a BUILD_* option should be used. For example, BUILD_GUI.
WITH_* options control what dependencies are turned on (internally, a CMake command find_* is used).

The resulted build option set being presented in the CMake GUI tool looks as follows:

An example of the configure summary:

Configure summary
=================
Executables:
  bitcoind ............................ ON
  bitcoin-node (multiprocess) ......... OFF
  bitcoin-qt (GUI) .................... OFF
  bitcoin-gui (GUI, multiprocess) ..... OFF
  bitcoin-cli ......................... ON
  bitcoin-tx .......................... ON
  bitcoin-util ........................ ON
  bitcoin-wallet ...................... ON
  bitcoin-chainstate (experimental) ... OFF
  libbitcoinkernel (experimental) ..... OFF
Optional features:
  wallet support ...................... ON
   - descriptor wallets (SQLite) ...... ON
   - legacy wallets (Berkeley DB) ..... OFF
  external signer ..................... ON
  port mapping:
   - using NAT-PMP .................... OFF
   - using UPnP ....................... OFF
  ZeroMQ .............................. OFF
  USDT tracing ........................ OFF
  QR code (GUI) ....................... OFF
  DBus (GUI, Linux only) .............. OFF
Tests:
  test_bitcoin ........................ ON
  test_bitcoin-qt ..................... OFF
  bench_bitcoin ....................... ON
  fuzz binary ......................... ON

...

Closes #194.

hebasto · 2024-05-12T08:38:10Z

I've implemented all @ryanofsky's siggestions from his #194 (comment).

However, I still have a few minor concerns.

CCACHE > WITH_CCACHE

The ccache tool shouldn't be considered as a dependency that affects the resulted binaries.

FUZZ > ENABLE_FUZZ

This option does not enable any features in the resulted binaries. It just forcibly enables BUILD_FUZZ_BINARY while disabling all other BUILD_* options.

ryanofsky · 2024-05-13T13:50:25Z

1. > `CCACHE` > `WITH_CCACHE`
The ccache tool shouldn't be considered as a dependency that affects the resulted binaries.

I think most consistent approach is to make WITH_ options control what external dependencies are used, and make ENABLE_ control what internal features are enabled. So WITH_ options control which find_package, find_library, find_program, etc, calls are made. You could draw the line somewhere else too, though.

2. > `FUZZ` > `ENABLE_FUZZ`
This option does not enable any features in the resulted binaries. It just forcibly enables BUILD_FUZZ_BINARY while disabling all other BUILD_* options.

I assumed FUZZ option controlled what compile options libraries were built with, so they would be instrumented for fuzzing. But if FUZZ only affects which targets are built, not how targets are built, it might make sense to use the BUILD_ prefix instead of ENABLE_. However, ENABLE_ also seems ok if we want to reserve BUILD_ for options that switch on build targets, instead of switching them off.

More generally, it seems ok to use ENABLE_ prefixes for everything and not have WITH_ or BUILD_ prefixes at all. The WITH_ prefixes are just nice to communicate what is internal and what is external. The the BUILD_ prefixes are nice to distinguish options that control what targets are built from options that control how targets are built.

(I also think ideally, the BUILD_ options might be eliminated, and CMake would just generate makefiles and project files that allow building every possible target. This way, developers could choose what targets they want to build when they run make, and see all available targets show up in IDE interfaces, without needing to change CMake options when they want to build new targets.)

fanquake · 2024-05-16T10:48:22Z

Shouldn't multiprocess be an ENABLE_ option? I think it should also return to only having a single line in the configure output, I don't think we need to list all the different potential binaries.

fanquake · 2024-05-16T10:53:30Z

I also think the distinction in the output is a bit inconsistent / confusing. Drawing a distinction between Optional features: and Optional packages: when all optional packages do is enable features doesn't seem that useful. You could just as well have "Port mapping" listed under features, instead of UPnP in packages; like you've done with the wallet.

hebasto · 2024-05-16T11:07:21Z

Shouldn't multiprocess be an ENABLE_ option?

This competes with @ryanofsky's suggestion in #194 (comment). What criteria we should consider to make a choice?

I think it should also return to only having a single line in the configure output, I don't think we need to list all the different potential binaries.

Would it be better to hide potential binaries from output when they are disabled?

hebasto · 2024-05-16T11:18:02Z

I also think the distinction in the output is a bit inconsistent / confusing. Drawing a distinction between Optional features: and Optional packages: when all optional packages do is enable features doesn't seem that useful. You could just as well have "Port mapping" listed under features, instead of UPnP in packages; like you've done with the wallet.

Does this look better:

Optional features:
  wallet support ...................... ON
   - descriptor wallets (SQLite) ...... ON
   - legacy wallets (Berkeley DB) ..... OFF
  external signer ..................... ON
  port mapping:
   - using NAT-PMP .................... ON
   - using UPnP ....................... OFF
  ZeroMQ .............................. OFF
  USDT tracing ........................ OFF
  QR code (GUI) ....................... ON
  DBus (GUI, Linux only) .............. ON

?

ryanofsky · 2024-05-16T13:35:23Z

Agree it is nicer to have feature summary list all features together, and not split them up based on which features require external dependencies.

Also the BUILD_MULTIPROCESS name I suggested was wrong, and I think it should be called WITH_LIBMULTIPROCESS, because it determines whether to call find_package(Libmultiprocess):

bitcoin/CMakeLists.txt

Lines 158 to 161 in 4ba2d95

    
           if(BUILD_MULTIPROCESS) 
        
             find_package(Libmultiprocess CONFIG REQUIRED) 
        
             find_package(LibmultiprocessGen CONFIG REQUIRED) 
        
           endif()

(The reason I suggested BUILD_ instead of ENABLE_, was because I was still thinking about the autoconf build where there's a separate --with-libmultiprocess option to control detection of the library, and an --enable-multiprocess option to build the extra binaries. But in a cmake world without yes/no/auto logic, it is better to have one option than two.)

vasild

ACK 4ba2d95

CMakeLists.txt

hebasto · 2024-05-21T10:06:21Z

@ryanofsky

Also the BUILD_MULTIPROCESS name I suggested was wrong, and I think it should be called WITH_LIBMULTIPROCESS ...

Done.

@vasild

I would rather have a disabled option being present in the summary with an OFF value instead of missing.

Done.

The PR description has been updated with a new screenshot and a summary excerpt.

CMakeLists.txt

hebasto · 2024-05-22T11:56:38Z

Both @TheCharlatan's comments have been addressed.

TheCharlatan

lgtm

hebasto · 2024-05-22T13:53:48Z

https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e has been updated according to this PR changes.

pablomartin4btc

tACK 79c3c89

Tested on terminal and built multiprocess on Ubuntu running successfully bitcoin-node.

make -C depends -j $(nproc) HOST=x86_64-pc-linux-gnu CC="" CXX="" MULTIPROCESS=1 LOG=1
...
cmake -B build --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake
...

Configure summary
=================
Executables:
  bitcoind ............................ ON
  bitcoin-node (multiprocess) ......... OFF
  bitcoin-qt (GUI) .................... OFF
  bitcoin-gui (GUI, multiprocess) ..... OFF
  bitcoin-cli ......................... ON
  bitcoin-tx .......................... ON
  bitcoin-util ........................ ON
  bitcoin-wallet ...................... ON
  bitcoin-chainstate (experimental) ... OFF
  libbitcoinkernel (experimental) ..... OFF
Optional features:
  wallet support ...................... ON
   - descriptor wallets (SQLite) ...... ON
   - legacy wallets (Berkeley DB) ..... OFF
  external signer ..................... ON
  port mapping:
   - using NAT-PMP .................... OFF
   - using UPnP ....................... OFF
  ZeroMQ .............................. OFF
  USDT tracing ........................ OFF
  QR code (GUI) ....................... OFF
  DBus (GUI, Linux only) .............. OFF
Tests:
  test_bitcoin ........................ ON
  test_bitcoin-qt ..................... OFF
  bench_bitcoin ....................... ON
  fuzz binary ......................... ON
...
[ 62%] Built target bitcoin_node
...
./build/src/bitcoin-node
2024-05-22T14:02:41Z Bitcoin Core version v27.99.0-79c3c897bb28 (release build)
...

nit: we don't want to indicate *multiprocess is also experimental (?).

...

Configure summary
=================
Executables:
  bitcoind ............................ ON
  bitcoin-node (*multiprocess) ........ OFF
  bitcoin-qt (GUI) .................... OFF
  bitcoin-gui (GUI, *multiprocess) .... OFF
  bitcoin-cli ......................... ON
  bitcoin-tx .......................... ON
  bitcoin-util ........................ ON
  bitcoin-wallet ...................... ON
  bitcoin-chainstate (experimental) ... OFF
  libbitcoinkernel (experimental) ..... OFF
  *multiprocess (experimental)
Optional features:
...

hebasto · 2024-05-22T14:17:19Z

nit: we don't want to indicate *multiprocess is also experimental (?).

The "multiprocess" project is a WIP -- bitcoin#28722.

97f4a60 fixup! cmake: Rename build option `FUZZ` to `ENABLE_FUZZ` (Hennadii Stepanov) Pull request description: This is a follow-up to #198. Rename missed cases. Top commit has no ACKs. Tree-SHA512: d2dab7cf45c8328b6658e9be60e739f736258af017a41039d19bd32443a01eac9423873ac6372eb85c77d8f1c32203020a6031aff62e8e6b1d83b2b97c938c65

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

ryanofsky mentioned this pull request May 16, 2024

[RFC] cmake: Build option naming #194

Closed

vasild approved these changes May 17, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

hebasto force-pushed the 240512-cmake-FB branch from 4ba2d95 to 87db7af Compare May 21, 2024 09:55

hebasto force-pushed the cmake-staging branch from 1d76056 to e8e0e69 Compare May 21, 2024 11:17

hebasto force-pushed the 240512-cmake-FB branch from 87db7af to 884831a Compare May 21, 2024 11:21

hebasto added 2 commits May 21, 2024 14:23

cmake: Rename build option CCACHE to WITH_CCACHE

2a8aba3

cmake: Rename build option FUZZ to ENABLE_FUZZ

576251f

hebasto force-pushed the 240512-cmake-FB branch from 884831a to 37b2090 Compare May 21, 2024 13:24

TheCharlatan reviewed May 22, 2024

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

hebasto added 6 commits May 22, 2024 12:54

cmake: Rename build option HARDENING to ENABLE_HARDENING

9d6ebf4

cmake: Rename build option MULTIPROCESS to WITH_MULTIPROCESS

bed5fea

cmake: Rename build option THREADLOCAL to ENABLE_THREADLOCAL

41a2159

cmake: Rename option WITH_EXTERNAL_SIGNER to ENABLE_EXTERNAL_SIGNER

7c151e7

cmake: Rework wallet report in summary

18abbb9

cmake: Group port mapping in summary

79c3c89

hebasto force-pushed the 240512-cmake-FB branch from 37b2090 to 79c3c89 Compare May 22, 2024 11:56

TheCharlatan approved these changes May 22, 2024

View reviewed changes

hebasto merged commit d9ba536 into cmake-staging May 22, 2024
27 of 34 checks passed

pablomartin4btc approved these changes May 22, 2024

View reviewed changes

hebasto mentioned this pull request May 23, 2024

[FIXUP] cmake: Rename build option FUZZ to ENABLE_FUZZ #210

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: Consistent build option naming #198

cmake: Consistent build option naming #198

hebasto commented May 12, 2024 •

edited

Loading

hebasto commented May 12, 2024

ryanofsky commented May 13, 2024

fanquake commented May 16, 2024

fanquake commented May 16, 2024

hebasto commented May 16, 2024

hebasto commented May 16, 2024

ryanofsky commented May 16, 2024

vasild left a comment

hebasto commented May 21, 2024

hebasto commented May 22, 2024

TheCharlatan left a comment

hebasto commented May 22, 2024

pablomartin4btc left a comment

hebasto commented May 22, 2024

cmake: Consistent build option naming #198

cmake: Consistent build option naming #198

Conversation

hebasto commented May 12, 2024 • edited Loading

The build option naming convention

hebasto commented May 12, 2024

ryanofsky commented May 13, 2024

fanquake commented May 16, 2024

fanquake commented May 16, 2024

hebasto commented May 16, 2024

hebasto commented May 16, 2024

ryanofsky commented May 16, 2024

vasild left a comment

Choose a reason for hiding this comment

hebasto commented May 21, 2024

hebasto commented May 22, 2024

TheCharlatan left a comment

Choose a reason for hiding this comment

hebasto commented May 22, 2024

pablomartin4btc left a comment

Choose a reason for hiding this comment

hebasto commented May 22, 2024

hebasto commented May 12, 2024 •

edited

Loading