cmake: Set top-level target output locations #31161

1. 
   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.

2. hebasto added the label Build system on Oct 26, 2024
3. 
   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)
   • #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.

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

   Concept ACK
5. hebasto force-pushed on Oct 26, 2024
6. DrahtBot added the label CI failed on Oct 26, 2024
7. DrahtBot removed the label CI failed on Oct 26, 2024
8. 
   theStack commented at 9:03 pm on October 26, 2024: contributor

   Concept ACK
9. 
   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).

10. 
    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:

    1. Copying/moving build artifacts into a single directory. This can be done using an add_custom_command(TARGET ... POST_BUILD ...) command.
    2. Adding a path to the DLLs location to the PATH environment variable.
11. 
    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.

12. fanquake added this to the milestone 29.0 on Oct 29, 2024
13. 
    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.
14. DrahtBot added the label Needs rebase on Oct 30, 2024
15. hebasto force-pushed on Oct 31, 2024
16. 
    hebasto commented at 10:36 am on October 31, 2024: member

    Rebased due to the conflict with the merged bitcoin/bitcoin#31015.
17. DrahtBot removed the label Needs rebase on Oct 31, 2024
18. 
    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.

19. 
    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.

20. DrahtBot added the label Needs rebase on Nov 5, 2024
21. hebasto force-pushed on Nov 5, 2024
22. 
    hebasto commented at 2:16 pm on November 5, 2024: member

    Rebased due to the conflict with the merged bitcoin/bitcoin#31191.
23. DrahtBot removed the label Needs rebase on Nov 5, 2024
24. maflcko added the label DrahtBot Guix build requested on Nov 5, 2024
25. 
    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...

26. DrahtBot removed the label DrahtBot Guix build requested on Nov 10, 2024
27. DrahtBot added the label Needs rebase on Nov 11, 2024
28. 
    BrandonOdiwuor commented at 3:04 pm on November 12, 2024: contributor

    Concept ACK
29. 
    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.

30. hebasto force-pushed on Nov 16, 2024
31. 
    hebasto commented at 6:00 pm on November 16, 2024: member

    Rebased due to the conflict with the merged #26593.
32. DrahtBot removed the label Needs rebase on Nov 16, 2024
33. 
    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…)

34. 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
35. 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
36. hebasto force-pushed on Nov 19, 2024
37. 
    hebasto commented at 2:13 pm on November 19, 2024: member

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

Contributors
hebasto DrahtBot TheCharlatan theStack fanquake theuni BrandonOdiwuor

Labels
Build system

Milestone
29.0