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
Concept ACK	TheCharlatan, theStack, theuni, BrandonOdiwuor

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:

#31332 (contrib: fix BUILDDIR in gen-bitcoin-conf script by MarnixCroes)
#31268 (cmake: add optional source files to bitcoin_crypto directly by purpleKarrot)
#29457 (doc: Fix gen-manpages to check build options by BrandonOdiwuor)

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…)

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.

3b6e0d0998

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-

06ff9429d3

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.

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]