cmake: Adjust build option defaults by hebasto · Pull Request #196 · hebasto/bitcoin

hebasto · 2024-05-09T19:39:22Z

This PR adjusts build option defaults per the discussion during today's call.

hebasto · 2024-05-09T19:40:34Z

cc @theuni @fanquake @TheCharlatan @vasild @m3dwards @laanwj

vasild · 2024-05-10T09:48:08Z

-option(BUILD_UTIL "Build bitcoin-util executable." ON)
+
+option(BUILD_TESTS "Build test_bitcoin executable." ON)
+option(BUILD_TX "Build bitcoin-tx executable." ${BUILD_TESTS})


This expands BUILD_TESTS to control (by default) whether to build test_bitcoin, bitcoin-tx, bitcoin-util, bitcoin-wallet, bench_bitcoin and fuzz.

Those are beyond the scope of "tests" and so using BUILD_TESTS to control them is confusing. One way to avoid this is to not have the dependency and have each option be independent of the others (ditch this PR, I think it is like this with autotools?).

Or if we insist on having all those being controlled by one option (for convenience?), then maybe introduce a new meta option BUILD_EXTRA that turns ON/OFF all of BUILD_TESTS (maybe BUILD_UNIT_TESTS is a better name because there are also functional tests and benchmark tests and fuzz tests), BUILD_TX, BUILD_UTIL, BUILD_WALLET_TOOL, BUILD_BENCH, BUILD_FUZZ_BINARY.

We decided (on this week's call) on this approach because the functional tests depend on these binaries and silently skip tests if they're not present. It's not ideal, but we agreed it's better than the alternative.

A new angle on this:

Having in mind the problem that if the user compiles without e.g. bitcoin-tx, then some functional tests will be skipped.

Before this PR: BUILD_TX is ON, so the problem does not exist if the user uses the defaults and does not fiddle with the options. The problem arises if they explicitly set BUILD_TX is OFF.

After this PR: same as above, so this PR brings no improvement?

However, there is an undesirable side effect with this PR: if BUILD_TESTS is set to OFF, then automatically BUILD_TX will be turned to OFF as well.

Maybe it would be better if each option is kept independent. Then if BUILD_TX is explicitly changed from its default ON to OFF, then emit a warning during configure: "some functional tests will be skipped without bitcoin-tx".

Two other objections to this:

In bitcoind we have some options turn on/off other options. This is oftentimes complicated and difficult to asses what will happen if a given option is changed. It is hard on both ends - from the user's perspective and from developer's perspective when changing the code. I think it is better to keep it simple stupid and avoid having one option flip the value of another, which is more aligned with https://en.wikipedia.org/wiki/Principle_of_least_astonishment.

"Full execution of functional tests" and "Whether to build unit tests executable" are unrelated and tying them together would be odd.

I do agree with @vasild on this:

Maybe it would be better if each option is kept independent. Then if BUILD_TX is explicitly changed from its default ON to OFF, then emit a warning during configure: "some functional tests will be skipped without bitcoin-tx".

I still disagree with this as I'd rather err on the side of "whoops, I built an extra binary but all the tests got run".

vasild · 2024-05-17T11:31:20Z

@@ -65,8 +65,11 @@ include(CMakeDependentOption)
 option(BUILD_DAEMON "Build bitcoind executable." ON)
 option(BUILD_GUI "Build bitcoin-qt executable." OFF)
 option(BUILD_CLI "Build bitcoin-cli executable." ON)


bitcoin-cli is also referenced from the functional tests. Shouldn't it also be treated like the others, e.g. bitcoin-tx?

I'd keep it ON by default and, as you said, independently from tests.

vasild · 2024-05-17T11:47:20Z

-option(BUILD_UTIL "Build bitcoin-util executable." ON)
+
+option(BUILD_TESTS "Build test_bitcoin executable." ON)
+option(BUILD_TX "Build bitcoin-tx executable." ${BUILD_TESTS})


A new angle on this:

Having in mind the problem that if the user compiles without e.g. bitcoin-tx, then some functional tests will be skipped.

Before this PR: BUILD_TX is ON, so the problem does not exist if the user uses the defaults and does not fiddle with the options. The problem arises if they explicitly set BUILD_TX is OFF.

After this PR: same as above, so this PR brings no improvement?

However, there is an undesirable side effect with this PR: if BUILD_TESTS is set to OFF, then automatically BUILD_TX will be turned to OFF as well.

Maybe it would be better if each option is kept independent. Then if BUILD_TX is explicitly changed from its default ON to OFF, then emit a warning during configure: "some functional tests will be skipped without bitcoin-tx".

pablomartin4btc · 2024-05-18T07:59:47Z

@@ -65,8 +65,11 @@ include(CMakeDependentOption)
 option(BUILD_DAEMON "Build bitcoind executable." ON)
 option(BUILD_GUI "Build bitcoin-qt executable." OFF)


Currently, in master, we build bitcoin-qt by default, what's the reasoning behind this to be changed?

During our calls, we discussed the concept of build options. We have reached a rough consensus to make them "opt-in" rather than "opt-out" as much as possible.

theuni · 2024-05-22T18:36:21Z

      "cacheVariables": {
        "VCPKG_TARGET_TRIPLET": "x64-windows",
        "BUILD_GUI": "ON",
+        "BUILD_BENCH": "ON",


Why on for Windows?

Thanks!. Moved to the CI script.

theuni · 2024-05-22T18:39:54Z

I'm afraid we could bikeshed these forever. IMO it's good enough for a first pass, and we'll learn quickly post-merge if people are upset/confused.

hebasto · 2024-05-23T10:13:23Z

Rebased to resolve a conflict.

@theuni's #196 (comment) has been addressed.

theuni

ACK b1b4977

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

vasild reviewed May 10, 2024

View reviewed changes

vasild reviewed May 17, 2024

View reviewed changes

pablomartin4btc reviewed May 18, 2024

View reviewed changes

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

hebasto force-pushed the 240509-cmake-EZ branch 2 times, most recently from 4d4cf13 to 8bb7538 Compare May 21, 2024 19:40

hebasto mentioned this pull request May 22, 2024

cmake, doc: Update build-unix.md #208

Merged

theuni reviewed May 22, 2024

View reviewed changes

hebasto added 2 commits May 23, 2024 11:11

cmake: Disable by default BUILD_BENCH and BUILD_FUZZ_BINARY

9f55197

cmake: Make BUILD_<util> defaults dependent on BUILD_TESTS

b1b4977

hebasto force-pushed the 240509-cmake-EZ branch from 8bb7538 to b1b4977 Compare May 23, 2024 10:12

theuni approved these changes May 23, 2024

View reviewed changes

hebasto merged commit 06872ef into cmake-staging May 24, 2024

		@@ -65,8 +65,11 @@ include(CMakeDependentOption)
		option(BUILD_DAEMON "Build bitcoind executable." ON)
		option(BUILD_GUI "Build bitcoin-qt executable." OFF)

Conversation

hebasto commented May 9, 2024

Uh oh!

hebasto commented May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented May 22, 2024

Uh oh!

hebasto commented May 23, 2024

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hebasto commented May 9, 2024 •

edited

Loading