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:
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.
Warning: This PR changes build locations of newly built executables like bitcoind and test_bitcoin from src/ to bin/ without deleting previously built executables. A clean build is recommended to avoid accidentally running old binaries.
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.
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:
#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
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]
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:
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 ACK06ff9429d32943aa4debec38ea105c8af4283aec. 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)
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
Code review ACK06ff942. 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 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
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
hebasto force-pushed
on Jan 10, 2025
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
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
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
So I propose dropping the libexec stuff entirely for this PR.
Dropped.
in
src/CMakeLists.txt:17
in
9691848c29outdated
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.”
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?
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
9691848c29outdated
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.
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.”
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.
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.
ryanofsky
commented at 8:17 pm on January 16, 2025:
contributor
Code review ACKe9edf28ba4ff4ebf02ae90874794c51c55ef789e. 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
hebasto force-pushed
on Jan 16, 2025
hebasto
commented at 9:46 pm on January 16, 2025:
member
Code review ACKe9edf28. 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 ACK186680acef661bd139bf3d16f494365ea55993b5. 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
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 :)
utACK186680acef661bd139bf3d16f494365ea55993b5
theStack approved
theStack
commented at 3:06 pm on January 28, 2025:
contributor
Nice! Tested on Ubuntu 22.04.3 LTS with a fresh build using the dev-mode preset and verifying the build/{bin,lib} folder contents. Also checked that the only remaining binaries and libraries in build/src are from subtree projects (which makes sense to not mix it). Only skimmed quickly over the actual build system changes, as I’m not familiar with the ins and outs of CMake.
achow101
commented at 8:56 pm on January 29, 2025:
member
~0
I’ve read through the conversation but I still don’t understand why this change is beneficial?
This is also a major breaking change and would break a significant amount of external tooling that rely on the current location of the binaries.
theuni
commented at 9:07 pm on January 29, 2025:
member
@achow101 This is one of the changes I was referring to in last week’s IRC meeting. We’d rather not move these around after v29 (with the exception of some moves to libexec) so that nobody has to migrate to CMake and then adapt again.
Not sure what you mean about breaking existing tooling though? This affects the location in the CMake build tree. It’s not related to installation locations. So afaik the only tooling that needs to be updated has been changed in this PR. Presumably anyone else relying on the old autotools locations will be adapting for CMake anyway, and imo this makes migration even easier as now everything’s in one place.
ryanofsky
commented at 9:56 pm on January 29, 2025:
contributor
~0
I’ve read through the conversation but I still don’t understand why this change is beneficial?
The main thing I like about this change is that makes it easier to write scripts and navigate the build. Examples:
When you run make WHATEVER, WHATEVER will be built in bin/ if it is an executable lib/ if it is a binary. You don’t have to go looking all over the tree for the WHATEVER you just built in src/ or src/bench/ or src/test/ or src/qt/ or /src/qt/test or whatever random subdirectory the add_executable or add_library call is placed in. Currently add_executable and add_library calls are not placed in consistent locations because some subdirectories have CMakeLists.txt files and other don’t and are built by their parents, and there is usually some reasoning behing this but as a build system user the placement seems arbitrary and random.
This makes builds match releases, so for example if you want to write a script that calls binaries you don’t have to go hunting around different locations and hardcoding multiple paths to make it work in a both a build directory and a release. You can just point the script at the bin/ directory, not have hardcoded build/src/ paths.
It makes the build easier to navigate as a new user or developer. You can extract the source, run make, run ls -l bin/ and see all the tools that have been built and are available to use. You don’t have to hunt through the build/src/ directory and sift through object and makefile cruft to see tools that are available.
Even though we aren’t really using shared libraries I think this change would especially make sense in case we did provide more shared libraries in the future. It is useful to be able to relocate builds and run them with LD_LIBRARY_PATH, and if all libraries are built in the same lib/ that is easy to do. On windows there are no rpaths, so shared libraries actually need to be placed in the same folder (bin/) to work together easily. They can’t be scattered in different build directories and just work.
This is also a major breaking change and would break a significant amount of external tooling that rely on the current location of the binaries.
I think it would break internal tooling not external tooling since this PR does not affect install or release locations. Earlier I suggested this could create symlinks in old locations for convenience and to prevent breakage. I don’t think too many tools should actually break, but I do think this change could be annoying to developers, for example if you just update git and rebuild, and try to run the old binary because you don’t know the new binary has a different location.
achow101
commented at 11:26 pm on January 29, 2025:
member
Not sure what you mean about breaking existing tooling though?
I think it would break internal tooling not external tooling since this PR does not affect install or release locations. Earlier I suggested this could create symlinks in old locations for convenience and to prevent breakage. I don’t think too many tools should actually break, but I do think this change could be annoying to developers, for example if you just update git and rebuild, and try to run the old binary because you don’t know the new binary has a different location.
Yes, the tooling for developers is what I meant.
I have several scripts which aid me in building and testing prs. These already have to figure out when to use autotools and when to use cmake. With this PR, these would now all need to figure out where the binaries live, and that seems to be significantly less trivial to do than detecting autotools or cmake.
While I’m not opposed to this change, I’m not thrilled by it either. It feels a lot like a “it’d be nice if we did that” kind of change, but one that also ends up breaking a bunch of developer workflows while doing it. I would’ve preferred that this was done with cmake, or immediately after it, than now where we’re a few weeks before feature freeze for 29.0.
ryanofsky
commented at 1:58 am on January 30, 2025:
contributor
I have several scripts which aid me in building and testing prs. These already have to figure out when to use autotools and when to use cmake. With this PR, these would now all need to figure out where the binaries live, and that seems to be significantly less trivial to do than detecting autotools or cmake.
I think it would be nice not just for tools but also for developers to just create symlinks from the old to new locations. Here is a commit which does that, mostly written by chatgpt (if that is not obvious): 433aa99d238740bbdcb07b53b17f639170bd326d
hebasto
commented at 10:31 am on January 30, 2025:
member
With this PR, these would now all need to figure out where the binaries live, and that seems to be significantly less trivial to do than detecting autotools or cmake.
I would’ve preferred that this was done with cmake, or immediately after it, than now…
I agree with you in hindsight.
… where we’re a few weeks before feature freeze for 29.0.
The point is to avoid breaking changes for the (external) users.
I think it would be nice not just for tools but also for developers to just create symlinks from the old to new locations. Here is a commit which does that, mostly written by chatgpt (if that is not obvious): 433aa99
I’d refrain form adding the code, which will effectively become dead in a couple of weeks or so. Also, the suggested commit might not work on Windows, where symbolic links are not enabled by default.
ryanofsky
commented at 7:43 pm on February 3, 2025:
contributor
I think if this PR is going to be merged without providing backward compatibility, something should be done to warn developers, like adding a tag [BREAKING] or [INCOMPATIBLE] or [!] or emoji ‼️ or ⚠️ to the subject and a clear warning in the description and maybe saying something in IRC channel too.
In its current form, this change seems likely to cause confusion and wasted time because when upgrading or downgrading around the change, builds will appear to succeed but old binaries will be left in place so it will be easy to accidentally wind up testing with binaries you think contain changes that aren’t there.
IMO, there is isn’t a good reason to break compatibility here at all, if a shim that just adds one new line and one new file to the build #31161 (comment) can provide it.
As long as we’re going to customize output locations, putting symlinks in the default locations pointing to our custom locations, seems like a sensible thing to do. This would be useful for developers upgrading in near future, as well as developers testing or bisecting later on, and for developers who may be familiar with cmake but not familiar with our particular build and looking for outputs in the default cmake locations.
(Sorry for repeating this suggestion again. Just wanted to be clear about reasons I think it would be worthwhile.)
hebasto
commented at 7:49 pm on February 3, 2025:
member
IMO, there is isn’t a good reason to break compatibility here at all, if a shim that just adds one new line and one new file to the build #31161 (comment) can provide it.
Thanks! Applied.
in
cmake/module/GenerateSymlinks.cmake:29
in
433aa99d23outdated
I wonder if this should be an add_custom_command() with a POST_BUILD event instead? Presumably this version (I haven’t checked) creates the symlink even if the link fails, whereas I would expect a POST_BUILDcommand to not have that problem.
ryanofsky
commented at 9:41 pm on February 6, 2025:
What problem do you see this solving? Creating a symlink pointing at a target that does not exist should be fine. It will still point to the right place.
theuni approved
theuni
commented at 7:50 pm on February 6, 2025:
member
utACK433aa99d238740bbdcb07b53b17f639170bd326d
Only change since my last review is the symlink script, which I don’t feel strongly about either way. I left a comment about a fixup for it, but it’s not a blocker for me.
DrahtBot requested review from theStack
on Feb 6, 2025
DrahtBot requested review from ryanofsky
on Feb 6, 2025
fanquake
commented at 8:52 pm on February 6, 2025:
member
When would we remove these symlinks? This seems like overkill, and just deferring breakage until later? CMake is already a huge disruption, and it’s not clear that we need to add even more workarounds to accomodate a few developers before we’ve even managed to actually fully make the changeover. Some infra/script/fuzzing etc will need to be updated, but this all seems maangeable.
Also, the suggested commit might not work on Windows, where symbolic links are not enabled by default.
Did anyone test this on Windows?
theuni
commented at 8:59 pm on February 6, 2025:
member
When would we remove these symlinks? This seems like overkill, and just deferring breakage until later? CMake is already a huge disruption, and it’s not clear that we need to add even more workarounds to accomodate a few developers before we’ve even managed to actually fully make the changeover. Some infra/script/fuzzing etc will need to be updated, but this all seems maangeable.
I agree with this. We’ve not really attempted any autotools compatibility, I’m not sure why this would be an exception.
I just didn’t want to see this PR blocked forever.
ryanofsky
commented at 9:33 pm on February 6, 2025:
contributor
When would we remove these symlinks? This seems like overkill, and just deferring breakage until later?
We’ve not really attempted any autotools compatibility
I don’t see any reason to delete the symlinks in the future. And they are not created for compatibility with autotools but for compatibility with cmake defaults and compatibility with our own build.
Also, if we did decide to delete the symlinks in the future it would not be deferring the breakage but replacing silent breakage (i.e. you rebuild with change and your changes have no effect because you are running an old binary in there is no indication about what could be wrong) with obvious breakage (i.e. the location you were expecting the binary to exist does not exist at all).
IMO, there’s no reason to a inflict silent breakage like this on developers if you are maintaining a build system. It should be a point of pride if you work on a build system for your system to just work and not waste other developers time.
ryanofsky
commented at 9:44 pm on February 6, 2025:
contributor
The success of the cmake -E create_symlink command on Windows depends on the “Create symbolic links” privilege (SeCreateSymbolicLinkPrivilege), which is disabled by default. There are a few ways to enabled it. It is enabled in GitHub Actions images.
With SeCreateSymbolicLinkPrivilege disabled, the build fails:
0 Creating symlink from C:/Users/hebasto/bitcoin/build/src to C:/Users/hebasto/bitcoin/build/bin
1CUSTOMBUILD : CMake error : failed to create symbolic link 'C:/Users/hebasto/bitcoin/build/src/bitcoin-cli.exe': A required privilege is not held by the client. [C:\Users\hebasto\bitcoin\build\src\bitcoin-cli_create_symlink.vcxproj]
hebasto
commented at 9:05 am on February 8, 2025:
member
Additionally, 433aa99d238740bbdcb07b53b17f639170bd326d does not work for any multi-config generators.
For example:
0$ cmake -B build -G "Ninja Multi-Config"
1$ cmake --build build
2$ file build/src/bitcoind
3build/src/bitcoind: broken symbolic link to /home/hebasto/git/bitcoin/build/bin/bitcoind
hebasto force-pushed
on Feb 11, 2025
hebasto
commented at 10:52 am on February 11, 2025:
member
IMO, there is isn’t a good reason to break compatibility here at all, if a shim that just adds one new line and one new file to the build #31161 (comment) can provide it.
Thanks! Applied.
Reverted, as the GenerateSymlinks module was not sufficiently robust.
In its current form, this change seems likely to cause confusion and wasted time because when upgrading or downgrading around the change, builds will appear to succeed but old binaries will be left in place so it will be easy to accidentally wind up testing with binaries you think contain changes that aren’t there.
It can be easily mitigated by starting a build with a clean build tree.
sipa
commented at 4:44 pm on February 13, 2025:
member
Mild concept ACK. I think having all binaries in one place is a nice, but not critical improvement. If we’re doing it, we should do it now, though.
TheCharlatan
commented at 1:07 pm on February 18, 2025:
contributor
Thought a guix build might be prudent since we are changing a bunch of paths (built on aarch64):
Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
hebasto force-pushed
on Feb 18, 2025
DrahtBot removed the label
Needs rebase
on Feb 18, 2025
hebasto
commented at 4:06 pm on February 18, 2025:
member
TheCharlatan
commented at 6:15 pm on February 18, 2025:
Nit: These headers in the comments seem a bit inconsistent at the moment. What makes the secp subtree special enough to get its own header? Where do these sections end? Why don’t the other components get their own headers?
hebasto
commented at 1:24 pm on February 20, 2025:
What makes the secp subtree special enough to get its own header?
For other subtrees, it has not yet been decided whether their own build scripts will be used.
fanquake
commented at 3:03 pm on February 20, 2025:
it has not yet been decided whether their own build scripts will be used.
The current decision is to not use them (i.e #30905). As far as I’m aware, no one is working on the changes that would be required for us to start using them, so I’m not sure why we’d start using them.
TheCharlatan approved
TheCharlatan
commented at 6:28 pm on February 18, 2025:
contributor
ACKf7d8a44ce42cce0b3010a7f7d14ec180e959f3e4
DrahtBot requested review from theuni
on Feb 18, 2025
DrahtBot requested review from sipa
on Feb 18, 2025
DrahtBot removed the label
CI failed
on Feb 18, 2025
theStack approved
theStack
commented at 4:16 pm on February 20, 2025:
contributor
(retested as described in #31161#pullrequestreview-2578563294 before)
glozow assigned fanquake
on Feb 20, 2025
DrahtBot added the label
Needs rebase
on Feb 20, 2025
hebasto force-pushed
on Feb 20, 2025
hebasto
commented at 5:23 pm on February 20, 2025:
member
Rebased due to the conflict with the merged bitcoin/bitcoin#31884.
ryanofsky approved
ryanofsky
commented at 6:13 pm on February 20, 2025:
contributor
Code review ACK879569cab4e5b400350f3b95d7bee71b49636591 (no changes since last review other than rebase), but again I think this PR should be prefixed with [BREAKING] or [INCOMPATIBLE] or [!] or ‼️ or ⚠️ and have a clear message to developers about what it does like “Warning: This PR changes build locations of newly built executables like bitcoind and test_bitcoin from src/ to bin/ without deleting previously built executables. A clean build is recommended to avoid accidentally running old binaries.”
Reverted, as the GenerateSymlinks module was not sufficiently robust.
I still think a change like 433aa99d238740bbdcb07b53b17f639170bd326d would be a strict improvement to avoid creating confusion and silent failures when updating. And to be compatible with cmake default build locations. If symlinks are disabled on windows bitcoin builds or in multi-config ninja bitcoin builds that seems harmless because I don’t think we have any developers regularly using these.
In its current form, this change seems likely to cause confusion and wasted time because when upgrading or downgrading around the change, builds will appear to succeed but old binaries will be left in place so it will be easy to accidentally wind up testing with binaries you think contain changes that aren’t there.
It can be easily mitigated by starting a build with a clean build tree.
The point is that failures can be silent, so you don’t know there is anything to mitigate. For example, you make a change that causes a segfault but you don’t know it causes a segfault because you are running a stale binary. Or you are trying to fix a bug and your fixes do not work because you do not know you are a running a stale binary.
I don’t think this breakage should block the PR if others are ok with it. I’m only arguing against it because it seems unnecessary.
DrahtBot requested review from TheCharlatan
on Feb 20, 2025
DrahtBot requested review from theStack
on Feb 20, 2025
hebasto
commented at 6:53 pm on February 20, 2025:
member
Code review ACK879569c (no changes since last review other than rebase), but again I think this PR should be prefixed with [BREAKING] or [INCOMPATIBLE] or [!] or ‼️ or ⚠️ and have a clear message to developers about what it does like “Warning: This PR changes build locations of newly built executables like bitcoind and test_bitcoin from src/ to bin/ without deleting previously built executables. A clean build is recommended to avoid accidentally running old binaries.”
Done.
DrahtBot removed the label
Needs rebase
on Feb 20, 2025
DrahtBot added the label
Needs rebase
on Feb 20, 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.
026bb226e9
scripted-diff: Adjust documentation per top-level target output location
hebasto
commented at 10:22 pm on February 20, 2025:
member
Rebased due to the conflict with the merged bitcoin/bitcoin#31742.
DrahtBot removed the label
Needs rebase
on Feb 21, 2025
maflcko
commented at 11:09 am on February 21, 2025:
member
I don’t think it is a supported use-case to leave the cmake build dir stale and append builds on it, without fully deleting it first. Not deleting the build dir is a well-known footgun unrelated to this pull, where devs ran into it already. So I think dropping the symlink commit is correct to not imply this is a supported use-case. (If someone wanted to close that footgun completely and permanantly, maybe cmake could error the build if the build directory already exists?) Unrelated from the footgun, if someone likes the old locations, they can call ln ... themselves after each build (either manually, or automatically).
Other than that, I don’t have a strong opinion and I haven’t reviewed or tested this, but weak concept ack.
ryanofsky
commented at 12:01 pm on February 21, 2025:
contributor
I don’t think it is a supported use-case to leave the cmake build dir stale and append builds on it, without fully deleting it first.
What are you talking about? I am doing this constantly many times every day, when I am checking out different PRs and branches and running make and expecting binaries to be up to date when it finishes. If other developers are happy to break this assumption I’m also happy though. Just trying to point out alternatives and tradeoffs here.
Sjors
commented at 12:28 pm on February 21, 2025:
member
I am doing this constantly many times every day, when I am checking out different PRs
Same, but I’ve sometimes found it unreliable, just as with autotools. For example on this branch, if you first do -DENABLE_WALLET=OFF and then -DENABLE_WALLET=ON if will fail with “Wallet functionality requested but no BDB or SQLite support available.”
maybe cmake could error the build if the build directory already exists?)
That seems overkill, but emitting a warning would be a good reminder and debugging hint.
No strong opinion on the symlinks.
Lightly tested 568fcdddaec2cc8decba5a098257f31729cc1caa on macOS 15.3.1 (Apple Silicon) and macOS 13.7.4 (Intel).
DrahtBot requested review from ryanofsky
on Feb 21, 2025
maflcko
commented at 1:09 pm on February 21, 2025:
member
What are you talking about?
The set of binaries that are built can be picked by setting configure options, so the most obvious way to end up with stale binaries is to make a build will all binaries and then another build with a subset of the binaries (intentionally or accidentally) without clearing the build dir. This is one example. Another example would be a stale cmake cache, which influences the build result (intentionally or accidentally). I am sure there are other examples.
All of this silently passes and then people go out and report bugs against Bitcoin Core, because after some wasted time they notice that something is slightly off. Then, developers waste their time trying to reproduce only to tell the user to do a clean build. (https://github.com/bitcoin/bitcoin/issues/23125#issuecomment-929873287, #27808 (comment), …)
Similarly with cmake, all the bug reports of this kind will be wasting time that could have been saved by just doing a clean and reproducible build. Just today there was a cmake build issue reported due to a stale folder and I am sure there will be more issues once cmake is in a release.
If unclean builds work for you in all cases reliably, then that is great, by my impression is that this isn’t true for everyone. At least to me, the benefit of saving a few seconds of build time only to spend minutes or hours debugging afterwards doesn’t seem worth it.
hebasto
commented at 3:15 pm on February 21, 2025:
member
ryanofsky
commented at 2:09 pm on February 26, 2025:
contributor
Code review ACK568fcdddaec2cc8decba5a098257f31729cc1caa. Only change since last review was rebasing. I’m ok with this PR in its current form if other developers are happy with it. I just personally think it is inappropriate to *silently* break an everyday developer workflow like git pull; make bitcoind. I wouldn’t have a problem with this PR if it triggered an explicit error, or if the problem was limited to less common workflows like changing cmake options in an existing build.
I just think the silent failures this PR causes are unnecessary given (1) how easy it it to provide compatibility with a shim that doesn’t touch the rest of the build, (2) how the shim provides useful functionality on its own and (3) how the shim can be safely removed later since removing it would not trigger silent failures.
This is just my opinion though. If it is not convincing I am ok with current approach.
TheCharlatan approved
TheCharlatan
commented at 2:53 pm on March 4, 2025:
contributor
Re-ACK568fcdddaec2cc8decba5a098257f31729cc1caa
Sjors
commented at 9:54 am on March 7, 2025:
member
If I understand the remaining criticism correctly, it seems best to merge this before branch-off and improve things later (perhaps immediately after branch-off).
The number of users that will be confused if we delay this change until after branch-off is potentially much larger than the number of “everyday developer” folks impacted by the silent breakage here. For people who build the release from source, it’s better if we educate them about the cmake changes and the new binary locations at the same time.
the silent failures this PR causes are unnecessary given (1) how easy it it to provide compatibility with a shim that doesn’t touch the rest of the build
To not lose the existing review work here, maybe someone can PR this right after branch-off?
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-03-10 03:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
