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.

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

[RFC] cmake: Build option naming #194

Closed
hebasto opened this issue May 8, 2024 · 5 comments
Closed

[RFC] cmake: Build option naming #194

hebasto opened this issue May 8, 2024 · 5 comments
Milestone

Comments

@hebasto
Copy link
Owner

hebasto commented May 8, 2024

All build options effectively are cache variables, whose values persist across multiple runs within a project build tree.

From the Professional CMake: A Practical Guide 18th Edition:

Cache variables are primarily intended as a customization point for developers.

The current (@ 9857482) build option set being presented in the CMake GUI tool looks as follows:

Screenshot from 2024-05-08 22-45-52

Please note that selecting the "Grouped" checkbox groups variables together based on the start of the variable name up to the first underscore. CMake reserves names that begin with CMAKE_.

  1. The group of BUILD_* options is presented in the "Executables" and "Tests" sections of the "Configure summary".

Please note, that CMake's CTest module:

automatically creates a BUILD_TESTING option...

  1. The group of WITH_* options is presented in the "Wallet support" and "Optional packages features" section of the "Configure summary".

However, the MULTIPROCESS option has no WITH_ prefix.

  1. Switching to upstream CMake-based build systems for subtrees (see cmake: Switch to libsecp256k1 upstream build system #192) will expose their own options. Thus, libsecp256k1 introduces SECP256K1_* names.

This issue aims to gather suggestions and opinions regarding better naming. Please vote for each suggestion using 👍 for approval and 👎 for disapproval.

@hebasto
Copy link
Owner Author

hebasto commented May 8, 2024

ENABLE_WALLET --> WALLET

@hebasto
Copy link
Owner Author

hebasto commented May 9, 2024

cc @ryanofsky @achow101 @maflcko

@theuni
Copy link

theuni commented May 9, 2024

I like the BUILD_/WITH_/ENABLE_ groups personally. I think that's a nice split.
BUILD_: add a binary/library
WITH_: allow a dependency
ENABLE_: turn on a feature

@ryanofsky
Copy link

ryanofsky commented May 9, 2024

I haven't looked into this, and I might have an uninformed or minority opinion, but having WITH_ and ENABLE_ prefixes in most cases does make sense to me.

I think WITH_ options should control what dependencies are turned on. ENABLE_ options should control what features are turned on. BUILD_ options should control what binaries and libraries are built by default (when you run make).

For example, for multiprocess, there could be WITH_CAPNP to control whether the find_package(CapnProto) is called and that protocol is available, WITH_GRPC to control whether find_package(gRPC) is called and that protocol is available, and ENABLE_MULTIPROCESS to control whether multiprocess features will be enabled using the previous packages.

For the wallet, there could be WITH_SQLITE and WITH_LMDB options to enable those packages and ENABLE_WALLET to enable wallet features.

For the gui, there could be WITH_QT5, WITH_QT6, and ENABLE_GUI.

(I know we don't like having dependencies, so these examples are theoretical, but I think we should still choose a naming convention that can accommodate having dependencies cleanly.)

In some of these examples, the features controlled by ENABLE_ flags are built as standalone binaries, so having the ENABLE_ option is redundant, and a BUILD_ flag is sufficient. For example, the GUI lives in a separate bitcoin-qt binary from bitcoind so having ENABLE_GUI would be redundant and BUILD_GUI would be sufficient. The multiprocess binary bitcoin-node is also currently separate from bitcoind so having ENABLE_MULTIPROCESS would be redundant and having just BUILD_MULTIPROCESS would be sufficient for now. But wallet features are currently built into bitcoind so there should be an ENABLE_WALLET option to allow building with or without those features, and a separate WITH_SQLITE option to control whether the sqlite database library is used. (Assuming we drop bdb support, building with WITH_SQLITE=OFF and ENABLE_WALLET=ON would fail, but that seems fine. Not every combination of build flags that could work theoretically needs to be supported in practice.)

I also don't like the the convention of having options like MULTIPROCESS or WALLET without any prefixes at all. CMake builds can have dozens of options, or more, so I think ENABLE_ WITH_ and BUILD_ prefixes provide useful groupings and an indication of what the types of options are and what they do.

Looking at the screenshot in #194 (comment), I think in practice this just means that more variables would get prefixes. CCACHE > WITH_CCACHE, FUZZ > ENABLE_FUZZ, HARDENING > ENABLE_HARDENING, MULTIPROCESS > BUILD_MULTIPROCESS WITH_LIBMULTIPROCESS, THREADLOCAL > ENABLE_THREADLOCAL. And some names might change: WITH_EXTERNAL_SIGNER > ENABLE_EXTERNAL_SIGNER.

This is just my (not completely informed) preference though. There could be things I'm overlooking, and I wouldn't have a problem with a different approach.

(EDIT: Changed BUILD_MULTIPROCESS to WITH_LIBMULTIPROCESS as explained #198 (comment))

@hebasto hebasto added this to the UI, 23th May milestone May 9, 2024
hebasto added a commit that referenced this issue May 9, 2024
87d3b72 fixup! cmake [KILL 3-STATE]: Make WITH_GUI binary option w/ default OFF (Hennadii Stepanov)

Pull request description:

  Mark `Qt5*_DIR` cache variables as advanced to hide them in the GUI and avoid clutter.

  See #194.

ACKs for top commit:
  pablomartin4btc:
    ACK 87d3b72

Tree-SHA512: b4ed70214486919a6a9ac517864ff7c2cd32137bf7a2e116dde8a6b374ea36e3f0e92846accf47ebb5321323cf4c7715ca5751ad562ff42f9362c4b7450191f0
hebasto added a commit that referenced this issue May 22, 2024
79c3c89 cmake: Group port mapping in summary (Hennadii Stepanov)
18abbb9 cmake: Rework wallet report in summary (Hennadii Stepanov)
7c151e7 cmake: Rename option `WITH_EXTERNAL_SIGNER` to `ENABLE_EXTERNAL_SIGNER` (Hennadii Stepanov)
41a2159 cmake: Rename build option `THREADLOCAL` to `ENABLE_THREADLOCAL` (Hennadii Stepanov)
bed5fea cmake: Rename build option `MULTIPROCESS` to `WITH_MULTIPROCESS` (Hennadii Stepanov)
9d6ebf4 cmake: Rename build option `HARDENING` to `ENABLE_HARDENING` (Hennadii Stepanov)
576251f cmake: Rename build option `FUZZ` to `ENABLE_FUZZ` (Hennadii Stepanov)
2a8aba3 cmake: Rename build option `CCACHE` to `WITH_CCACHE` (Hennadii Stepanov)

Pull request description:

  Based on #194 (comment).

  ### The build option naming convention

  1. `BUILD_*` options control what binaries and libraries are built.

  2. `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`.

  3. `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:

  ![image](https://github.com/hebasto/bitcoin/assets/32963518/10e3f241-6649-437d-a698-d57ed14501b1)

  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.

Top commit has no ACKs.

Tree-SHA512: 21ab517502c11b92e7c173e9390311c470d481fd533671dc9445f4673dfedf76b96ff09dfa52d3c93dcc64e97c561d3ef75c70f7dc0ca513d7d6ff3cfff7368e
@hebasto
Copy link
Owner Author

hebasto commented May 22, 2024

Closing as resolved by #198.

@hebasto hebasto closed this as completed May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants