build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD

151henry151 commented at 10:35 pm on August 23, 2025: none

Remove deprecated CMake RPATH settings (CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH) that are no longer needed after reordering Guix build script to perform binary checks after installation.

Remove RPATH workarounds from CMakeLists.txt and src/CMakeLists.txt Reorder Guix script to check installed binaries instead of build tree binaries Update TODO comments

Builds successfully on Linux Full Guix build in progress

Addresses TODO items in CMakeLists.txt:620 and src/CMakeLists.txt:412

This is my first attempt at contributing to bitcoin core so please, provide me any feedback that you might have. Thanks!

build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings

Remove deprecated CMake settings that are no longer needed after
reordering Guix script commands to perform binary checks after the
installation step.

Changes:
- Remove CMAKE_SKIP_BUILD_RPATH TRUE setting from main CMakeLists.txt
- Remove SKIP_BUILD_RPATH OFF property setting from src/CMakeLists.txt
- Update TODO comments to reflect that these settings have been removed

The NetBSD-specific logic for CMAKE_INSTALL_RPATH_USE_LINK_PATH is
preserved as it's still needed for that platform.

This addresses the TODO items mentioned in:
- CMakeLists.txt:620
- src/CMakeLists.txt:412

Relevant discussions:
- https://github.com/hebasto/bitcoin/pull/236#issuecomment-2183120953
- https://github.com/bitcoin/bitcoin/pull/30312#issuecomment-2191235833

7f312e4d7e

DrahtBot added the label Build system on Aug 23, 2025

DrahtBot commented at 10:35 pm on August 23, 2025: contributor

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

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32380 (Modernize use of UTF-8 in Windows code by hebasto)

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

DrahtBot added the label CI failed on Aug 23, 2025

DrahtBot removed the label CI failed on Aug 23, 2025

hebasto commented at 7:15 am on August 24, 2025: member

This is my first attempt at contributing to bitcoin core so please, provide me any feedback that you might have. Thanks!

Thank you for your interest in contributing to this project!

The changes you are referring to as completed, i.e. “reordering Guix script commands to perform binary checks after the installation step”, have not actually been done yet. See contrib/guix/libexec/build.sh, from line 251 onwards.

151henry151 commented at 2:14 pm on August 24, 2025: none

This is my first attempt at contributing to bitcoin core so please, provide me any feedback that you might have. Thanks!

Thank you for your interest in contributing to this project!

The changes you are referring to as completed, i.e. “reordering Guix script commands to perform binary checks after the installation step”, have not actually been done yet. See contrib/guix/libexec/build.sh, from line 251 onwards.

Thanks for your helpful reply @hebasto. I’ve pushed another commit attempting to perform the actual reordering of the Guix script commands to perform those checks after the installation step; I tested it locally and it seemed to build successfully, hope I got the right idea here.

hebasto commented at 2:17 pm on August 24, 2025: member

In a02723427f5f0b142a6ef97b92df093e00ad6004, all checks are still performed on the binaries in the build tree, whereas they are supposed to be performed on the binaries in the installation location.

hebasto commented at 2:22 pm on August 24, 2025: member

… I tested it locally and it seemed to build successfully…

When a developer tests changes in the Guix scripts locally, they usually posts the resulting binary hashes. For example, as in #33217#pullrequestreview-3142128074.

151henry151 commented at 2:26 pm on August 24, 2025: none

Thanks for the feedback hebasto, I’ll continue working on this

Conflicts

Reviewers, this pull request conflicts with the following ones:
* [#32380](https://github.com/bitcoin/bitcoin/pull/32380) (Modernize use of UTF-8 in Windows code by hebasto)

The conflict resolves easily by keeping both changes.

janb84 commented at 3:18 pm on August 24, 2025: contributor

This is my first attempt at contributing to bitcoin core so please, provide me any feedback that you might have. Thanks!

Welcome, please also change this message for a description of the changes made in this PR. Helpful tips are in creating-the-pull-request

151henry151 commented at 3:23 pm on August 24, 2025: none

In a027234, all checks are still performed on the binaries in the build tree, whereas they are supposed to be performed on the binaries in the installation location.

Thanks for the feedback.

The previous change moved checks after installation but was still using cmake –build targets which check build tree binaries. I’ve now fixed this to directly call the security and symbol check scripts on the installed binaries in ${INSTALLPATH}/bin/, ensuring checks are performed on the final installed binaries with proper RPATHs.

When a developer tests changes in the Guix scripts locally, they usually posts the resulting binary hashes. For example, as in #33217#pullrequestreview-3142128074.

I’m currently running a full Guix build locally to generate the binary hashes. The build is in progress and I’ll post the resulting hashes once it completes.

151henry151 commented at 1:10 am on August 25, 2025: none

My Guix build resulting binary hashes:

075d4eb10b95a64c595ee1ddf31d3c1e0086958a1ae3d64f9e2bbc4930cd88366  dist-archive/bitcoin-e922e27a2ecd.tar.gz
1
2f7d2d6cd820561229ab5fa476c06484ced81a1fa8b90d2fbbd7a82fa915aa956  x86_64-linux-gnu/bitcoin-e922e27a2ecd-x86_64-linux-gnu-debug.tar.gz
3
4df24c2269cf96dbf011692fbc0b21dab7786abd43dbb82e4351f0117e95169e5  x86_64-linux-gnu/bitcoin-e922e27a2ecd-x86_64-linux-gnu.tar.gz

in CMakeLists.txt:621 in e922e27a2e outdated

617@@ -618,17 +618,16 @@ if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.29)
618   set(CMAKE_SKIP_TEST_ALL_DEPENDENCY FALSE)
619 endif()
620 
621-# TODO: The `CMAKE_SKIP_BUILD_RPATH` variable setting can be deleted
622-#       in the future after reordering Guix script commands to
623-#       perform binary checks after the installation step.
624+# TODO: The `CMAKE_SKIP_BUILD_RPATH` variable setting has been removed

janb84 commented at 7:53 am on August 25, 2025:

If the TODO is solved than there is no more TO DO. Please remove the todo or reword it what is still left to do.

151henry151 commented at 12:45 pm on August 25, 2025:

I clicked commit suggestion too quickly – It looks a bit like a comment was cut in half there. Would we want to remove the additional lines of the TODO as well, or rephrase them as comments?

151henry151 commented at 2:13 pm on August 25, 2025:

Looking at this more closely, the answer was clear and so I removed the additional lines of the TODO as well.

maflcko commented at 10:59 am on August 25, 2025: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

151henry151 commented at 10:59 am on August 25, 2025: none

Would it be better to leave the comments there and simply remove the TODO tag? It looks a bit like a comment was cut in half there, but perhaps adding back in the The `CMAKE_SKIP_BUILD_RPATH` variable setting has been removed Would be better than removing the rest of that information. What do you think?

151henry151 force-pushed on Aug 25, 2025

maflcko added the label DrahtBot Guix build requested on Aug 28, 2025

contrib: Reorder Guix build script to perform binary checks after installation

Move the binary security and symbol checks to occur after installation instead of before.

Binary checks are performed on the final
installed binaries rather than build artifacts.

This fix directly calls the security and symbol check scripts on the installed binaries
in the INSTALLPATH/bin/ directory.

This ensures that the checks are performed on the final installed binaries
with proper RPATHs pointing to final locations, not temporary build paths.
This eliminates the need for RPATH workarounds.

49c468860f

151henry151 force-pushed on Aug 29, 2025

151henry151 commented at 5:34 am on August 29, 2025: none

There was a TODO still in the src/CMakeLists.txt file; I cleaned it up and squashed the commits just now and think that it is now all correct.

DrahtBot commented at 6:56 am on August 29, 2025: contributor

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

File	commit 6ca6f3b37b992591726bd13b494369bee3bd6468(master)	commit 1ac12bc409e0570cefc640fcac1fe13edfa98a92(pull/33247/merge)
*-aarch64-linux-gnu-debug.tar.gz	`abcc0baac5e7b752...`	`8ce308bbc579ad49...`
*-aarch64-linux-gnu.tar.gz	`4a8653178ccbdddb...`	`ed54dfe44406b72a...`
*-arm-linux-gnueabihf-debug.tar.gz	`95d3d74d5682c24a...`	`86d72da5723c7b7d...`
*-arm-linux-gnueabihf.tar.gz	`e25759641901b072...`	`d53eee042815730f...`
*-arm64-apple-darwin-codesigning.tar.gz	`366f30df86e875e8...`	`9aee1e5a66c0b9d5...`
*-arm64-apple-darwin-unsigned.tar.gz	`1c83672238b93111...`	`e07f9aec2228734e...`
*-arm64-apple-darwin-unsigned.zip	`ad51645f12fb5dc7...`	`87aa37bf869b5b55...`
*-powerpc64-linux-gnu-debug.tar.gz	`df430f763352d69c...`	`39f3f3ebb73204d8...`
*-powerpc64-linux-gnu.tar.gz	`7dd5cbe718652deb...`	`0e6ef9dea9eeafd2...`
*-riscv64-linux-gnu-debug.tar.gz	`d258d893caa4d9b7...`	`48ad366e15a9c433...`
*-riscv64-linux-gnu.tar.gz	`3f182eee5fd9c395...`	`48cf7092c537d8cd...`
*-x86_64-apple-darwin-codesigning.tar.gz	`3998753c28bfa555...`	`7397d894c9cabe8c...`
*-x86_64-apple-darwin-unsigned.tar.gz	`b4474a38f90d8d72...`	`147d8db165c37ccd...`
*-x86_64-apple-darwin-unsigned.zip	`6028f331e23a3f0f...`	`60a73a9684ce8e1d...`
*-x86_64-linux-gnu-debug.tar.gz	`881ca6a16e6f2ac5...`	`b55f8fb6ec93d54b...`
*-x86_64-linux-gnu.tar.gz	`01fc4a45eb400557...`	`0aac741f62f5299c...`
*.tar.gz	`0711f5eb93cb4d47...`	`0191779babb2b28f...`
SHA256SUMS.part	`1801e58d5cf74ae8...`	`beb6ef46d809ac3d...`
guix_build.log	`cbdad833bb02e563...`	`b87aee0c1d625b91...`
guix_build.log.diff		`b3dfe2faa87aaa23...`

DrahtBot removed the label DrahtBot Guix build requested on Aug 29, 2025

151henry151 commented at 3:53 pm on September 1, 2025: none

Is there anything further required here? I understand that review for pull requests can take a long time (especially for non-critical things such as this one) but just wanted to check in and see if there’s further action needed from me, or anything about this pull request that I could improve.

build: Remove unused CMake maintenance targets ef5dd15309

build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings #33247

Code Coverage & Benchmarks

Reviews

Conflicts

Conflicts

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