cmake: Set top-level target output locations

hebasto commented at 2:08 pm on October 26, 2024: member

This PR sets the target output locations to the bin and lib subdirectories within the build tree, creating a directory structure that mirrors that of the installed targets.

This approach is widely adopted by the large projects, such as LLVM:

0set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
1set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
2set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})

The libsecp256k1 project has also recently adopted this approach.

With this PR, all binaries are conveniently located. For example, run:

0$ ./build/bin/fuzz

instead of:

0$ ./build/src/test/fuzz/fuzz

On Windows, all required DLLs are now located in the same directory as the executables, allowing to run bitcoin-chainstate.exe (which loads bitcoinkernel.dll) without the need to copy DLLs or modify the PATH variable.

The idea was briefly discussed among the build team during the recent CoreDev meeting.

hebasto added the label Build system on Oct 26, 2024

DrahtBot commented at 2:08 pm on October 26, 2024: 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/31161.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, theuni
Concept ACK	TheCharlatan, theStack, BrandonOdiwuor, Sjors

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:

#31394 ([POC] cmake: Introduce LLVM’s Source-based Code Coverage reports by hebasto)
#31332 (contrib: fix BUILDDIR in gen-bitcoin-conf script by MarnixCroes)
#31268 (cmake: add optional source files to bitcoin_crypto directly by purpleKarrot)
#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.

TheCharlatan commented at 2:25 pm on October 26, 2024: contributor

Concept ACK

hebasto force-pushed on Oct 26, 2024

DrahtBot added the label CI failed on Oct 26, 2024

DrahtBot removed the label CI failed on Oct 26, 2024

theStack commented at 9:03 pm on October 26, 2024: contributor

Concept ACK

fanquake commented at 3:37 pm on October 29, 2024: member

This seems fine, but it’d be good to primarily list the benefits to our project, rather than justifying making changes based on if some other open source project happens to do it, as other projects may have different constraints, or various reasoning for making the same change. Also, in this PR you say This approach is widely adopted by the large projects, such as LLVM, but in the secp256k1 thread for the same change you said that our current behaviour is “the default behaviour adopted by many projects.”, so it’d seem like either way of doing things is mostly fine/normal.

My understanding is that this change is mostly a convenience change for native Windows developers, so that it becomes easier to run binaries after compilation (without installing?). Possibly with the potential for us to follow up with / remove rpath usage entirely (cc @theuni). What I don’t really understand is how every other project that uses CMake as we do now, has solved this issue, if they haven’t made the same change as in this PR (or why this wouldn’t be the default CMake behaviour, if it otherwise results in broken (native) Windows binaries).

hebasto commented at 3:46 pm on October 29, 2024: member

My understanding is that this change is mostly a convenience change for native Windows developers, so that it becomes easier to run binaries after compilation (without installing?).

Correct.

What I don’t really understand is how every other project that uses CMake as we do now, has solved this issue, if they haven’t made the same change as in this PR (or why this wouldn’t be the default CMake behaviour, if it otherwise results in broken (native) Windows binaries).

There are alternatives, such as:

Copying/moving build artifacts into a single directory. This can be done using an add_custom_command(TARGET ... POST_BUILD ...) command.
Adding a path to the DLLs location to the PATH environment variable.

theStack commented at 4:11 pm on October 29, 2024: contributor

My understanding is that this change is mostly a convenience change for native Windows developers, so that it becomes easier to run binaries after compilation (without installing?).

Even as non-Windows developer, I personally find it quite handy if all binaries end up in a single folder and are not scattered around in different places, unnecessarily still reflecting the in-tree source structure (resulting sometimes in long nested paths, as one can see in the scripted-diff). Turned out to be more convenient in libsecp256k1 as well.

fanquake added this to the milestone 29.0 on Oct 29, 2024

fanquake commented at 4:54 pm on October 29, 2024: member

Also added this to 29.x, as if we are going to change this, it needs to happen along with the CMake switchover, and not be a new breaking change after that.

DrahtBot added the label Needs rebase on Oct 30, 2024

hebasto force-pushed on Oct 31, 2024

hebasto commented at 10:36 am on October 31, 2024: member

Rebased due to the conflict with the merged bitcoin/bitcoin#31015.

DrahtBot removed the label Needs rebase on Oct 31, 2024

theuni commented at 1:32 pm on October 31, 2024: member

Concept ACK.

As @fanquake mentioned above, we currently have:

0  set_target_properties(bitcoin-chainstate PROPERTIES
1    SKIP_BUILD_RPATH OFF
2  )

Which I think can be removed now, no?

Edit: Also, I think this obsoletes the CMAKE_SKIP_BUILD_RPATH comment in CMakeLists.txt.

hebasto commented at 4:49 pm on October 31, 2024: member

Concept ACK.

… we currently have:
0  set_target_properties(bitcoin-chainstate PROPERTIES
1    SKIP_BUILD_RPATH OFF
2  )
Which I think can be removed now, no?

No, we can’t. Only for Windows builds *.exe and *.dll files reside in a single bin subdirectory. On other platforms, shared libraries still reside in their own lib subdirectory, which requires the continued use of RPATH.

Edit: Also, I think this obsoletes the CMAKE_SKIP_BUILD_RPATH comment in CMakeLists.txt.

I believe this comment is still relevant.

DrahtBot added the label Needs rebase on Nov 5, 2024

hebasto force-pushed on Nov 5, 2024

hebasto commented at 2:16 pm on November 5, 2024: member

Rebased due to the conflict with the merged bitcoin/bitcoin#31191.

DrahtBot removed the label Needs rebase on Nov 5, 2024

maflcko added the label DrahtBot Guix build requested on Nov 5, 2024

DrahtBot commented at 1:37 pm on November 10, 2024: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit 018e5fcc462caebb25cf5d3eb6a19dc2787466c8(master)	commit 319ddc1bcda53b27a92483cc709ff7bfc8f165ea(master and this pull)
SHA256SUMS.part	`be3d1c137911ae9e...`	`b166bbac12dfcd7b...`
*-aarch64-linux-gnu-debug.tar.gz	`b4eeb3b873f1b60b...`	`71b8599f1f9dce92...`
*-aarch64-linux-gnu.tar.gz	`f632a8f0a22571ae...`	`d88bd9764c4ae6c4...`
*-arm-linux-gnueabihf-debug.tar.gz	`fb4abea62f9be905...`	`3b1a1ccf09efb551...`
*-arm-linux-gnueabihf.tar.gz	`2ac5f71078b41bbb...`	`342437f702ab66e4...`
*-arm64-apple-darwin-unsigned.tar.gz	`4525ad1e3dd0c9cb...`	`c141f6c42f43ae78...`
*-arm64-apple-darwin-unsigned.zip	`117a99d06509fb4a...`	`1f2c0f9391e01b42...`
*-arm64-apple-darwin.tar.gz	`d4e55fd1ed6b86ed...`	`3f6f243f0fc1eb35...`
*-powerpc64-linux-gnu-debug.tar.gz	`520afaf8aed42ff3...`	`edb1bdb108d7ec6e...`
*-powerpc64-linux-gnu.tar.gz	`a351714f9c6473a6...`	`622fda212331bb52...`
*-riscv64-linux-gnu-debug.tar.gz	`67cf604997e3518a...`	`fd285f3a10a951c0...`
*-riscv64-linux-gnu.tar.gz	`1d4eea93ae340ab0...`	`fe64f3767452250e...`
*-x86_64-apple-darwin-unsigned.tar.gz	`ea358917931d7e9a...`	`d38e488df17d9c9d...`
*-x86_64-apple-darwin-unsigned.zip	`6ab1cdbc6d56bc93...`	`8ec183ea2a40eec5...`
*-x86_64-apple-darwin.tar.gz	`d760b91d6983de5e...`	`76e1ead9513c03f6...`
*-x86_64-linux-gnu-debug.tar.gz	`8f240a932233ef25...`	`dde0aadb1db6f5da...`
*-x86_64-linux-gnu.tar.gz	`8e8aed42b35afa97...`	`92745bffd5534dba...`
*.tar.gz	`9399b03097df8b89...`	`f5c8b945b2974d19...`
guix_build.log	`982560f3b4e395e9...`	`9b70483f067be0a5...`
guix_build.log.diff		`08b969038243e63e...`

DrahtBot removed the label DrahtBot Guix build requested on Nov 10, 2024

DrahtBot added the label Needs rebase on Nov 11, 2024

BrandonOdiwuor commented at 3:04 pm on November 12, 2024: contributor

Concept ACK

theuni commented at 4:44 pm on November 12, 2024: member

Which I think can be removed now, no?

No, we can’t. Only for Windows builds *.exe and *.dll files reside in a single bin subdirectory. On other platforms, shared libraries still reside in their own lib subdirectory, which requires the continued use of RPATH.

Edit: Also, I think this obsoletes the CMAKE_SKIP_BUILD_RPATH comment in CMakeLists.txt.

I believe this comment is still relevant.

Yes, you’re right on both counts. I forgot about the different lib path for Windows (which is what makes it work there). Thanks for clarifying.

hebasto force-pushed on Nov 16, 2024

hebasto commented at 6:00 pm on November 16, 2024: member

Rebased due to the conflict with the merged #26593.

DrahtBot removed the label Needs rebase on Nov 16, 2024

theStack commented at 10:47 pm on November 18, 2024: contributor

Did some quick testing on Ubuntu 22.04 LTS and OpenBSD 7.6, verifying that the binaries end up in a single place as expected:

0$ ls ./build/bin/                                                                                                             
1bitcoin-cli      bitcoin-util     bitcoind         noverify_tests   test_bitcoin     unitester
2bitcoin-tx       bitcoin-wallet   exhaustive_tests object           tests

Not sure how people feel about binaries from subtree projects also being placed there, especially if they have very generic names like “tests” or “object”? (I thought first that https://github.com/bitcoin-core/secp256k1/pull/1582 would also add the secp256k1_ prefix to the binaries, but apparently it only affects test names for ctest…)

hebasto force-pushed on Nov 19, 2024

hebasto commented at 2:13 pm on November 19, 2024: member

Rebased on top of the #31307 to workaround MSVC compiler issue.

ryanofsky approved

ryanofsky commented at 3:14 pm on January 6, 2025: contributor

Code review ACK 06ff9429d32943aa4debec38ea105c8af4283aec. It seems like a improvement to me overall if binaries in the build directory have the same layout as binaries in releases. I’m not sure current implementation of this is ideal but it seems like a a step in the right direction.

Having binaries built in consistent locations that match install locations should make it is easier to find them, make scripts and wrappers simpler, and avoid bugs that only happen in developer builds but not final installs or vice-versa.

Things I think would be good to improve:

It would be good if binaries from subtrees were handled differently from our own binaries. See #31161 (review). I think ideally subtrees would be unaffected by this change and their binaries would remain in same locations they would be if subtrees were built separately.
It would be good if multiprocess binaries bitcoin-node and bitcoin-gui could be built in libexec/ instead of than bin/ since with #30975 these binaries are meant to be installed to libexec/ and not present in the PATH or called directly by users.
Possibly, it might make sense to build bitcoin-chainstate.exe and bitcoinkernel.dll to libexec/ instead of bin/.
Maybe it would be good to temporarily add symlinks from old locations to new locations to avoid breaking developers workflows abruptly and make it easier to migrate. Currently this PR seems to be a breaking change: #31161 (comment)

re: #31161 (comment)

What I don’t really understand is how every other project that uses CMake as we do now, has solved this issue, if they haven’t made the same change as in this PR (or why this wouldn’t be the default CMake behaviour, if it otherwise results in broken (native) Windows binaries)

I think most CMake projects avoid the library-finding issue by having a flatter hierarchy, and not using add_subdirectory calls as much. This avoids tying binary locations to source layout, and puts binaries in one place instead of scattering them in subdirectories.

I think the reason for bin/ and lib/ build directories not being cmake default behavior is also out of a preference for flatness and not having unnecessary nesting, but also because cmake is meant to be platform agnostic and tries to follow conventions of each platform. It wouldn’t use bin/ and lib/ directories on windows because that is a unix convention.

The reasons for cmake default behavior don’t really apply in our case though, since we are already building things in subdirectories, and our windows releases already follow unix conventions by having a top level archive directory and bin/ and share/ subdirectories.

DrahtBot requested review from theStack on Jan 6, 2025

DrahtBot requested review from theuni on Jan 6, 2025

DrahtBot requested review from BrandonOdiwuor on Jan 6, 2025

DrahtBot requested review from TheCharlatan on Jan 6, 2025

hebasto force-pushed on Jan 9, 2025

hebasto commented at 9:52 pm on January 9, 2025: member

@ryanofsky

Code review ACK 06ff942. It seems like a improvement to me overall if binaries in the build directory have the same layout as binaries in releases. I’m not sure current implementation of this is ideal but it seems like a a step in the right direction.

Having binaries built in consistent locations that match install locations should make it is easier to find them, make scripts and wrappers simpler, and avoid bugs that only happen in developer builds but not final installs or vice-versa.

Thank you for your thoughtful review!

Things I think would be good to improve:

It would be good if binaries from subtrees were handled differently from our own binaries. See #31161 (review). I think ideally subtrees would be unaffected by this change and their binaries would remain in same locations they would be if subtrees were built separately.

Reworked. Additionally, I treat the univalue ex-subtree the same way as other subtrees because of its test binaries. I doubt if we are interested in locating them in bin/.

It would be good if multiprocess binaries bitcoin-node and bitcoin-gui could be built in libexec/ instead of than bin/ since with #30975 these binaries are meant to be installed to libexec/ and not present in the PATH or called directly by users.

Thanks! implemented.

Possibly, it might make sense to build bitcoin-chainstate.exe and bitcoinkernel.dll to libexec/ instead of bin/.

I don’t have a strong opinion about that. Leaving it as is for now.

Maybe it would be good to temporarily add symlinks from old locations to new locations to avoid breaking developers workflows abruptly and make it easier to migrate. Currently this PR seems to be a breaking change: #31161 (comment)

TBH, I don’t like the idea of temporary code. If this PR lands before the v29 branch-off, the potential disruption will be limited.

DrahtBot added the label CI failed on Jan 9, 2025

hebasto force-pushed on Jan 10, 2025

hebasto commented at 1:10 pm on January 10, 2025: member

A silent conflict has been fixed.

fanquake commented at 3:49 pm on January 10, 2025: member

https://cirrus-ci.com/task/6223844022681600?logs=ci#L2898:

 0[08:40:37.725] + LD_LIBRARY_PATH=/ci_container_base/depends/i686-pc-linux-gnu/lib
 1[08:40:37.725] + test/functional/test_runner.py --ci -j10 --tmpdirprefix /ci_container_base/ci/scratch/test_runner/ --ansi --combinedlogslen=99999999 --timeout-factor=40 --v2transport --quiet --failfast
 2[08:40:38.317] 2025-01-10T13:40:38.202000Z TestFramework (INFO): PRNG seed is: 5569585375096173377
 3[08:40:38.317] 2025-01-10T13:40:38.206000Z TestFramework (INFO): Initializing test directory /ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20250110_134037/cache
 4[08:40:38.317] 2025-01-10T13:40:38.211000Z TestFramework (ERROR): Unexpected exception caught during testing
 5[08:40:38.317] Traceback (most recent call last):
 6[08:40:38.317]   File "/ci_container_base/test/functional/test_framework/test_framework.py", line 131, in main
 7[08:40:38.317]     self.setup()
 8[08:40:38.317]   File "/ci_container_base/test/functional/test_framework/test_framework.py", line 314, in setup
 9[08:40:38.317]     self.setup_chain()
10[08:40:38.317]   File "/ci_container_base/test/functional/test_framework/test_framework.py", line 405, in setup_chain
11[08:40:38.317]     self._initialize_chain()
12[08:40:38.317]   File "/ci_container_base/test/functional/test_framework/test_framework.py", line 875, in _initialize_chain
13[08:40:38.317]     self.start_node(CACHE_NODE_ID)
14[08:40:38.317]   File "/ci_container_base/test/functional/test_framework/test_framework.py", line 575, in start_node
15[08:40:38.317]     node.start(*args, **kwargs)
16[08:40:38.317]   File "/ci_container_base/test/functional/test_framework/test_node.py", line 256, in start
17[08:40:38.317]     self.process = subprocess.Popen(self.args + extra_args, env=subp_env, stdout=stdout, stderr=stderr, cwd=cwd, **kwargs)
18[08:40:38.317]                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
19[08:40:38.317]   File "/usr/lib/python3.12/subprocess.py", line 1026, in __init__
20[08:40:38.317]     self._execute_child(args, executable, preexec_fn, close_fds,
21[08:40:38.317]   File "/usr/lib/python3.12/subprocess.py", line 1955, in _execute_child
22[08:40:38.317]     raise child_exception_type(errno_num, err_msg, err_filename)
23[08:40:38.317] FileNotFoundError: [Errno 2] No such file or directory: 'bitcoin-node'
24[08:40:38.317] 2025-01-10T13:40:38.271000Z TestFramework (INFO): Stopping nodes

Sjors commented at 4:03 pm on January 10, 2025: member

Concept ACK

Lightly tested 8c44a947b501c8417454527637bd3adb3ce2ff4e by building on macOS 15.2 (M4). I like how the documentation is changed with a scripted-diff. The build system changes in the first commit look reasonable to me at first glance, but did not review thoroughly. @ryanofsky wrote:

Possibly, it might make sense to build bitcoin-chainstate.exe and bitcoinkernel.dll to libexec/ instead of bin/.

I like that as well. If we add them to the release later, they won’t clutter bin/.

Now is also a good time to move bitcoin-util there, because it’s only used by our test code (and for calibrating a new signet).

Re symlinks: I would also prefer to avoid that, if we can get this into v29.

theuni commented at 4:28 pm on January 10, 2025: member

It would be good if multiprocess binaries bitcoin-node and bitcoin-gui could be built in libexec/ instead of than bin/

This makes sense to me if we’re going to go down the route of the wrapper executable, but I don’t think we should do that until users aren’t actually supposed to be launching them directly.

IMO it’s not a problem to move them after a release cycle or two if the expected user behavior changes, if nothing else that’s actually a good indication that their behavior should change.

hebasto force-pushed on Jan 10, 2025

DrahtBot removed the label CI failed on Jan 10, 2025

hebasto commented at 10:29 am on January 11, 2025: member

re #31161 (comment)

@ryanofsky wrote:

Possibly, it might make sense to build bitcoin-chainstate.exe and bitcoinkernel.dll to libexec/ instead of bin/.

I like that as well. If we add them to the release later, they won’t clutter bin/.

Moved to libexec/. cc @TheCharlatan

Now is also a good time to move bitcoin-util there, because it’s only used by our test code (and for calibrating a new signet).

Done.

re #31161 (comment)

It would be good if multiprocess binaries bitcoin-node and bitcoin-gui could be built in libexec/ instead of than bin/

This makes sense to me if we’re going to go down the route of the wrapper executable, but I don’t think we should do that until users aren’t actually supposed to be launching them directly.

IMO it’s not a problem to move them after a release cycle or two if the expected user behavior changes, if nothing else that’s actually a good indication that their behavior should change.

The location of the bitcoin-node and bitcoin-gui reverted back to bin/.

Sjors commented at 9:24 am on January 13, 2025: member

Ok, so now we have bitcoin-util and bitcoin-chainstate in libexec, with bitcoin-gui and bitcoin-node back in bin - which can be moved by #31375. That seems fine to me.

When I do sudo cmake --install build on macOS 15.2 it puts the binaries in /usr/local/bin and /usr/local/lib (not /usr/local/libexec or /usr/libexec). Is that intended?

hebasto commented at 9:51 am on January 13, 2025: member

When I do sudo cmake --install build on macOS 15.2 it puts the binaries in /usr/local/bin and /usr/local/lib (not /usr/local/libexec or /usr/libexec). Is that intended?

Target output locations and their installation paths are independent properties. This PR modifies the former only. I’ll look into modifying the latter accordingly.

hebasto force-pushed on Jan 13, 2025

hebasto commented at 11:34 am on January 13, 2025: member

Reworked per recent feedback.

libexec is an installation path for targets whose output path is set to libexec.

fanquake commented at 1:32 pm on January 15, 2025: member

Why are bitcoin-chainstate and bitcoin-util being put into libexec? bitcoin-util is meant to be called by users directly. Not sure why bitcoin-chainstate would be put there either?

theuni commented at 3:17 pm on January 15, 2025: member

Why are bitcoin-chainstate and bitcoin-util being put into libexec? bitcoin-util is meant to be called by users directly. Not sure why bitcoin-chainstate would be put there either?

Agreed. The point of libexec is (implicitly) to make it HARDER to run binaries from there, because they don’t live in the user’s $PATH. They’re meant to be found by other binaries that do live in $PATH. So long as we’re recommending/relying on users running a binary by hand, it shouldn’t live in libexec.

So I propose dropping the libexec stuff entirely for this PR. I do think it’s worth moving the binaries there in the future when the execution model changes. Like I said above, IMO moving them later would actually be a good thing, as it will force users into the updated behavior.

hebasto force-pushed on Jan 15, 2025

hebasto commented at 3:57 pm on January 15, 2025: member

@fanquake @theuni

Thank you for your feedback!

So I propose dropping the libexec stuff entirely for this PR.

Dropped.

in src/CMakeLists.txt:17 in 9691848c29 outdated

28+# Subprojects
29+#=============================
30+# Subprojects include subdirectories that do or could have tests
31+# and/or benchmark binaries, such as all subtrees and univalue.
32+# The top-level target output locations for subprojects remain
33+# unmodified.

ryanofsky commented at 6:42 pm on January 16, 2025:

In commit “cmake: Set top-level target output locations” (9691848c29a772b5a8f0fc68855f8b2ba3c61921)

It wasn’t really clear to me what how this comment “The top-level target output locations for subprojects remain unmodified” is related to code it is commenting on.

Would maybe suggest changing to something more explanatory like “These need to be included before CMAKE_*_OUTPUT_DIRECTORY variables are set, so output locations of subproject tests and libraries are not overridden.”

hebasto commented at 9:48 pm on January 16, 2025:

Thanks! Updated.

in CMakeLists.txt:590 in 9691848c29 outdated

584@@ -585,9 +585,6 @@ set(CMAKE_SKIP_INSTALL_RPATH TRUE)
585 add_subdirectory(test)
586 add_subdirectory(doc)
587 
588-include(cmake/crc32c.cmake)
589-include(cmake/leveldb.cmake)
590-include(cmake/minisketch.cmake)

ryanofsky commented at 6:55 pm on January 16, 2025:

In commit “cmake: Set top-level target output locations” (9691848c29a772b5a8f0fc68855f8b2ba3c61921)

This seems ok, but I don’t understand why these include lines are moving from CMakeLists.txt to src/CMakeLists.txt. Unclear whether it’s necessary for this PR, or maybe it’s just nice to consolidate these together with univalue?

hebasto commented at 8:55 pm on January 16, 2025:

Unclear whether it’s necessary for this PR, or maybe it’s just nice to consolidate these together with univalue?

The latter.

theuni commented at 7:23 pm on January 17, 2025:

Not worth doing now, but in the future it’d be helpful to split up changes like this into separate commits so it’s more clear to reviewers what’s necessary for the described change vs what’s a helpful cleanup.

in src/CMakeLists.txt:20 in 9691848c29 outdated

31+# and/or benchmark binaries, such as all subtrees and univalue.
32+# The top-level target output locations for subprojects remain
33+# unmodified.
34+include(crc32c)
35+include(leveldb)
36+include(minisketch)

ryanofsky commented at 6:59 pm on January 16, 2025:

In commit “cmake: Set top-level target output locations” (9691848c29a772b5a8f0fc68855f8b2ba3c61921)

It seems like it might make things more obvious if included files were still referenced by path instead of by name: as ../cmake/crc32.cmake instead of crc32.

This would also avoid the need to change CMAKE_MODULE_PATH. And it might make sense conceptually because these files don’t act like typical cmake modules because they are adding targets rather than calling find commands or defining macros.

hebasto commented at 9:47 pm on January 16, 2025:

Thanks! Updated.

in src/CMakeLists.txt:22 in 9691848c29 outdated

32+# The top-level target output locations for subprojects remain
33+# unmodified.
34+include(crc32c)
35+include(leveldb)
36+include(minisketch)
37 add_subdirectory(univalue)

ryanofsky commented at 7:17 pm on January 16, 2025:

In commit “cmake: Set top-level target output locations” (9691848c29a772b5a8f0fc68855f8b2ba3c61921)

I was confused why univalue seemed to be handled differently than the other subprojects above.

Might be helpful to add a comment like “# The univalue subproject is different from the other subprojects because we actually use the CMakeLists.txt in the project subdirectory instead of defining separate cmake targets, so use add_subdirectory() instead of include() to include it.”

hebasto commented at 9:06 pm on January 16, 2025:

The included subprojects are subtrees managed with our custom CMake scripts. While there was an initial plan to switch to their upstream CMake scripts, this approach is currently considered questionable.

Conversely, univalue was promoted from a subtree to our own codebase, adopting the same per-directory approach.

in src/CMakeLists.txt:83 in 9691848c29 outdated

78+if(NOT CMAKE_ARCHIVE_OUTPUT_DIRECTORY)
79+  set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/lib)
80+endif()
81+
82+configure_file(${PROJECT_SOURCE_DIR}/cmake/bitcoin-build-config.h.in bitcoin-build-config.h USE_SOURCE_PERMISSIONS @ONLY)
83+include_directories(${CMAKE_CURRENT_BINARY_DIR} ${CMAKE_CURRENT_SOURCE_DIR})

ryanofsky commented at 7:20 pm on January 16, 2025:

In commit “cmake: Set top-level target output locations” (9691848c29a772b5a8f0fc68855f8b2ba3c61921)

These two lines seem to be duplicating lines at the top this file. I assume this is a copy and paste error and would suggest deleting one of the copies.

hebasto commented at 9:46 pm on January 16, 2025:

Thanks! Fixed.

ryanofsky approved

ryanofsky commented at 8:17 pm on January 16, 2025: contributor

Code review ACK e9edf28ba4ff4ebf02ae90874794c51c55ef789e. Nice changes! Again I think using consistent build locations that match install locations should make it is easier to see what binaries are available, simplify scripts and wrappers, and avoid bugs that only happen in developer builds but not final installs or vice-versa.

Since last review the main change seems to be that subproject binaries are not affected anymore by this PR, which is great.

I don’t understand the details of the different libexec pushes. It seems ok to postpone using libexec for now, but I disagree with @theuni “So long as we’re recommending/relying on users running a binary by hand, it shouldn’t live in libexec.”

IMO, it only makes sense for high level binaries intended to be used by normal users to be installed in bin/ and placed on the system PATH:

bitcoin-cli
bitcoind
bitcoin-qt
bitcoin-tx
bitcoin-util
bitcoin-wallet

I don’t think it makes sense for the following binaries which are not intended to by used by typical users to be installed in bin/ and put into the PATH:

bench_bitcoin
bitcoin-chainstate (*)
bitcoin-gui
bitcoin-node
fuzz (*)
test_bitcoin
test_bitcoin-qt

It seems better to either not install these binaries or to put them out of the way in libexec/ to avoid confusion. (It looks like the ones marked with an asterisk above are not currently installed, but the rest are.)

Would like to see more discussion about this and find out where the disagreement is so binaries don’t have to move twice, but this doesn’t need to block the PR.

DrahtBot requested review from Sjors on Jan 16, 2025

cmake: Set top-level target output locations

This change:
1. Collects build artifacts in dedicated locations.
2. Allows running bitcoin-chainstate.exe with bitcoinkernel.dll directly
from the build tree on Windows.

ee62f37b90

scripted-diff: Adjust documentation per top-level target output location

-BEGIN VERIFY SCRIPT-

ren() { sed -i "s|\<$1\>|$2|g" $( git grep -l "$1" :\(exclude\)./src/secp256k1 ) ; }

ren build/src/bench   build/bin
ren build/src/test    build/bin
ren build/src/qt/test build/bin
ren build/src/qt      build/bin
ren build/src         build/bin
ren build_fuzz/src/test/fuzz build_fuzz/bin

-END VERIFY SCRIPT-

186680acef

hebasto force-pushed on Jan 16, 2025

hebasto commented at 9:46 pm on January 16, 2025: member

Code review ACK e9edf28. Nice changes! Again I think using consistent build locations that match install locations should make it is easier to see what binaries are available, simplify scripts and wrappers, and avoid bugs that only happen in developer builds but not final installs or vice-versa. @ryanofsky

Thank you for your review! The branch has been updated per your feedback.

ryanofsky approved

ryanofsky commented at 2:59 pm on January 17, 2025: contributor

Code review ACK 186680acef661bd139bf3d16f494365ea55993b5. Since last review just reverted CMAKE_MODULE_PATH change, removed duplicate code and changed comment as suggested. Looks good!

ryanofsky commented at 5:08 pm on January 17, 2025: contributor

re: #31161#pullrequestreview-2556956641

It seems better to either not install these binaries or to put them out of the way in libexec/ to avoid confusion. […] Would like to see more discussion about this and find out where the disagreement is so binaries don’t have to move twice, but this doesn’t need to block the PR.

I opened #31679 to move internal binaries to libexec, since using libexec isn’t directly related to what this PR is trying to do. Would probably make sense for more discussion about libexec to happen in #31679 instead of here.

theuni approved

theuni commented at 8:00 pm on January 17, 2025: member

This is much simpler than the last time I looked. Thanks ryanofsky for all the feedback here. We can argue about libexec in the other PR :)

utACK 186680acef661bd139bf3d16f494365ea55993b5

cmake: Set top-level target output locations #31161

Code Coverage & Benchmarks

Reviews

Conflicts

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]