build: disable bitcoin-node if daemon is not built

Sjors commented at 1:29 pm on February 10, 2025: member

When building for fuzzing with multiprocess enabled, we were still trying to build bitcoin-node. This PR fixes that, by applying a similar check as for bitcoin-gui.

Before:

 0cmake -B build -DBUILD_FOR_FUZZING=ON -DWITH_MULTIPROCESS=ON
 1
 2...
 3
 4Configure summary
 5=================
 6Executables:
 7  bitcoind ............................ OFF
 8  bitcoin-node (multiprocess) ......... ON
 9  bitcoin-qt (GUI) .................... OFF
10  bitcoin-gui (GUI, multiprocess) ..... OFF
11
12...
13
14cmake --build build
15
16...
17
18[ 84%] Built target bitcoin-node

After:

0  bitcoin-node (multiprocess) ......... OFF

And no bitcoin-node target gets built (not to be confused with bitcoin_node).

DrahtBot commented at 1:29 pm on February 10, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31834.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, ryanofsky, laanwj

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31741 (multiprocess: Add libmultiprocess git subtree by ryanofsky)
#31679 (cmake: Move internal binaries from bin/ to libexec/ by ryanofsky)
#30975 (ci: build multiprocess on most jobs by Sjors)
#30437 (multiprocess: add bitcoin-mine test program by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Build system on Feb 10, 2025

hebasto commented at 1:32 pm on February 10, 2025: member

When building for fuzzing with multiprocess enabled, we were still trying to build bitcoin-node. This PR fixes that…

But 7e11ee6ad9b04546b900e38dee349db821415ba7 modifies only the summary output.

ryanofsky commented at 1:58 pm on February 10, 2025: contributor

But 7e11ee6 modifies only the summary output.

Maybe also should include:

 0diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
 1index 89fdd855a459..8c42359d2d55 100644
 2--- a/src/CMakeLists.txt
 3+++ b/src/CMakeLists.txt
 4@@ -320,7 +320,7 @@ if(BUILD_DAEMON)
 5   )
 6   list(APPEND installable_targets bitcoind)
 7 endif()
 8-if(WITH_MULTIPROCESS)
 9+if(WITH_MULTIPROCESS AND BUILD_DAEMON)
10   add_executable(bitcoin-node
11     bitcoind.cpp
12     init/bitcoin-node.cpp
13@@ -332,8 +332,9 @@ if(WITH_MULTIPROCESS)
14     $<TARGET_NAME_IF_EXISTS:bitcoin_wallet>
15   )
16   list(APPEND installable_targets bitcoin-node)
17+endif()
18 
19-  if(BUILD_TESTS)
20+if(WITH_MULTIPROCESS AND BUILD_TESTS)
21     # bitcoin_ipc_test library target is defined here in src/CMakeLists.txt
22     # instead of src/test/CMakeLists.txt so capnp files in src/test/ are able to
23     # reference capnp files in src/ipc/capnp/ by relative path. The Cap'n Proto
24@@ -347,7 +348,6 @@ if(WITH_MULTIPROCESS)
25       test/ipc_test.capnp
26     )
27     add_dependencies(bitcoin_ipc_test bitcoin_ipc_headers)
28-  endif()
29 endif()
30 
31

build: disable bitcoin-node if daemon is not built

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

2ffea09820

Sjors force-pushed on Feb 10, 2025

Sjors commented at 2:17 pm on February 10, 2025: member

Added, and updated the PR description.

bitcoin deleted a comment on Feb 10, 2025

hebasto commented at 2:46 pm on February 10, 2025: member

Concept ACK.

On the master branch:

0$ gmake -C depends NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_USDT=1 MULTIPROCESS=1
1$ cmake -B build -G Ninja --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake -DBUILD_FOR_FUZZING=ON
2$ ninja -C build -t targets | grep -E "bitcoind|bitcoin-node"
3bitcoin-node: phony

With this PR, the last command’s output is empty.

hebasto approved

hebasto commented at 2:53 pm on February 10, 2025: member

ACK 2ffea09820e66e25ab639c9fc14810fd96ad6213.

ryanofsky approved

ryanofsky commented at 3:01 pm on February 10, 2025: contributor

Code review ACK 2ffea09820e66e25ab639c9fc14810fd96ad6213

Sjors referenced this in commit 11063dc13d on Feb 11, 2025

in src/CMakeLists.txt:323 in 2ffea09820

319@@ -320,7 +320,7 @@ if(BUILD_DAEMON)
320   )
321   list(APPEND installable_targets bitcoind)
322 endif()
323-if(WITH_MULTIPROCESS)
324+if(WITH_MULTIPROCESS AND BUILD_DAEMON)

laanwj commented at 10:53 am on February 11, 2025:

Would it be possible to use bitcoin_daemon_status here somehow, instead of repeating the expression ? (just a small maintainability thing to keep the reporting and usage consistent if it changes in the future)

Sjors commented at 11:10 am on February 11, 2025:

It turns out yes, as long as you define bitcoin_daemon_status before add_subdirectory(src). Pushing that.

hebasto commented at 11:12 am on February 11, 2025:

TBH, I’d refrain from making bitcoin_daemon_status a “global” variable.

Every time it is used elsewhere, developers have to check out its definition.

Sjors commented at 11:22 am on February 11, 2025:

In that case I’m inclined to not use the variable at all.

Sjors commented at 11:26 am on February 11, 2025:

Oh but message() can’t handle that. Alright, let’s go back to the previous version.

laanwj commented at 11:38 am on February 11, 2025:

In that case I’m inclined to not use the variable at all.

Ok, yes, that would have been my other proposal. But a variable seems to be needed then, let’s leave it as-is.

laanwj approved

laanwj commented at 10:54 am on February 11, 2025: member

Code review ACK 2ffea09820e66e25ab639c9fc14810fd96ad6213

Sjors force-pushed on Feb 11, 2025

Sjors referenced this in commit ef0204c236 on Feb 11, 2025

ryanofsky merged this on Feb 11, 2025

ryanofsky closed this on Feb 11, 2025

Sjors deleted the branch on Feb 11, 2025

build: disable bitcoin-node if daemon is not built #31834

Code Coverage & Benchmarks

Reviews

Conflicts