build: Prevent system header fallback and include path pollution#34143
Merged
fanquake merged 4 commits intobitcoin:masterfrom Feb 11, 2026
Merged
build: Prevent system header fallback and include path pollution#34143fanquake merged 4 commits intobitcoin:masterfrom
fanquake merged 4 commits intobitcoin:masterfrom
Conversation
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34143. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Contributor
Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use] |
Contributor
|
my Guix Build Output Host architecture: 5eb1bd5a15708f4270eebbbb8f9e7f7f431da2c8d69603826f47ac18e58b31fd guix-build-2a6901529c73/output/aarch64-linux-gnu/SHA256SUMS.part
b46120b21bacb54cb999a7ee34906203b74a82a7a797adfaf5160ffe39df179f guix-build-2a6901529c73/output/aarch64-linux-gnu/bitcoin-2a6901529c73-aarch64-linux-gnu-debug.tar.gz
8d0ab4938c9b3dab75d0a3c75b97aca6ed35644fcdd42eedd95b5b6152892285 guix-build-2a6901529c73/output/aarch64-linux-gnu/bitcoin-2a6901529c73-aarch64-linux-gnu.tar.gz
d61e0f3bdb67fb09bd95f9457f2e4fa5949b88045a21ec5519291ee6bea77051 guix-build-2a6901529c73/output/arm-linux-gnueabihf/SHA256SUMS.part
b0b1ded64d6f62e4e42fd24e8429a616d372b3d1302a041f73737b2593588558 guix-build-2a6901529c73/output/arm-linux-gnueabihf/bitcoin-2a6901529c73-arm-linux-gnueabihf-debug.tar.gz
004b14d5513ce82262cc9510401714a97af37522eff3481a0d70db581f06cd3c guix-build-2a6901529c73/output/arm-linux-gnueabihf/bitcoin-2a6901529c73-arm-linux-gnueabihf.tar.gz
9dad3b1374d23171fa400dc286551ee61ac188bec14d576a038ed6de1820fd1a guix-build-2a6901529c73/output/arm64-apple-darwin/SHA256SUMS.part
e101af1b0fd333d5e498da19278b00d844e44f14e46678804577124bd1d9eaef guix-build-2a6901529c73/output/arm64-apple-darwin/bitcoin-2a6901529c73-arm64-apple-darwin-codesigning.tar.gz
b21d7e342bf3e3dbf4a30b547d0eab4c5482e6c48cafc7e52ed6ce84f274f771 guix-build-2a6901529c73/output/arm64-apple-darwin/bitcoin-2a6901529c73-arm64-apple-darwin-unsigned.tar.gz
57fe0760877802005946963d87c3a6b5849f3d03fd7ad412b2c55f345d0af163 guix-build-2a6901529c73/output/arm64-apple-darwin/bitcoin-2a6901529c73-arm64-apple-darwin-unsigned.zip
7f074c95551f7bbf9f23a85f0ad46aebf9a6069f17e3c89230257ae96b107338 guix-build-2a6901529c73/output/dist-archive/bitcoin-2a6901529c73.tar.gz
920dc7d726be94004b3003b7d0d1e7ac59ce1fe64275cc42712bb1fa63737e8d guix-build-2a6901529c73/output/powerpc64-linux-gnu/SHA256SUMS.part
d3f6f76cc2df64b263612224efdbc5cda29960d939ed7fef56fdb2547618f23e guix-build-2a6901529c73/output/powerpc64-linux-gnu/bitcoin-2a6901529c73-powerpc64-linux-gnu-debug.tar.gz
0970e286347486e9fef88f15e2409911d09fbbf2cf1d52e8a06a4cc247923e48 guix-build-2a6901529c73/output/powerpc64-linux-gnu/bitcoin-2a6901529c73-powerpc64-linux-gnu.tar.gz
1a9b469909c685d46aeea0668dff6f532570154e4f8abe7224d94c92ef28d1c5 guix-build-2a6901529c73/output/riscv64-linux-gnu/SHA256SUMS.part
c5a49c68ae6346d1510b497c483e25b04c1aac22f4b9580a15efb80c5590a3cb guix-build-2a6901529c73/output/riscv64-linux-gnu/bitcoin-2a6901529c73-riscv64-linux-gnu-debug.tar.gz
2bd07c73dc5a13ee33d27db52d903a7f4a71fac45db7b636c37b02c6d3fb4b5f guix-build-2a6901529c73/output/riscv64-linux-gnu/bitcoin-2a6901529c73-riscv64-linux-gnu.tar.gz
55b3a2b6258cb645547eb01c3ef46fcd97d4cb27ca8dd84c9c9a91ea1314cf81 guix-build-2a6901529c73/output/x86_64-apple-darwin/SHA256SUMS.part
8e52dc444786c392ac6010b6bb2c8d2536d8da4731a4ef87f211af548bf9dced guix-build-2a6901529c73/output/x86_64-apple-darwin/bitcoin-2a6901529c73-x86_64-apple-darwin-codesigning.tar.gz
373ceed73edcc41a343d2bce9b3f6d36634ca873b5d53959464f89435417f585 guix-build-2a6901529c73/output/x86_64-apple-darwin/bitcoin-2a6901529c73-x86_64-apple-darwin-unsigned.tar.gz
80e3980143fd4d5d8728e660ff01d029c1a8aab69f9718decd0b2149e287d88d guix-build-2a6901529c73/output/x86_64-apple-darwin/bitcoin-2a6901529c73-x86_64-apple-darwin-unsigned.zip
392c4d7911daf946bbf913ee27975bfda37693a992ca61794018e39c2fbad68f guix-build-2a6901529c73/output/x86_64-linux-gnu/SHA256SUMS.part
6bc8ce0727114c65006e98392795d255ed74e2f7a2ebf9ea7518c369fe577fe3 guix-build-2a6901529c73/output/x86_64-linux-gnu/bitcoin-2a6901529c73-x86_64-linux-gnu-debug.tar.gz
95207f982880b179ec329efd8f0c8cee89868874953d7c8125c2fde44c1edf0f guix-build-2a6901529c73/output/x86_64-linux-gnu/bitcoin-2a6901529c73-x86_64-linux-gnu.tar.gz
e269e1eae037b5c76f168945566c44d0695398f287ca6b131f789591ec88462c guix-build-2a6901529c73/output/x86_64-w64-mingw32/SHA256SUMS.part
65961e3ab4f98f24db751802a58e1bb1640d956f3b0ae5ee081cef4a8fe897f4 guix-build-2a6901529c73/output/x86_64-w64-mingw32/bitcoin-2a6901529c73-win64-codesigning.tar.gz
928be5ef461640e856bb9ded3e40a2e8ef131beb8423aa930f43cec34a50a973 guix-build-2a6901529c73/output/x86_64-w64-mingw32/bitcoin-2a6901529c73-win64-debug.zip
5521ed52afad02466930117496741114e7c62792bac25fc707869711c4a115c8 guix-build-2a6901529c73/output/x86_64-w64-mingw32/bitcoin-2a6901529c73-win64-setup-unsigned.exe
adc5553e1a162662ef668093dba297874a075ddb04992c9f15b58e4fe2131e43 guix-build-2a6901529c73/output/x86_64-w64-mingw32/bitcoin-2a6901529c73-win64-unsigned.zip |
Member
|
Nice. Concept ACK to moving, but I'd like to explore something else before making this boost-specific. Ideally, all packages would be installed to their own prefix so that no two include dirs are the same. As-is, we could (using your example) run into the same conflict between The reason depends uses a common prefix by default is that some depends packages depend on other depends packages (the qt/x11 ones, for example). Since those are the exception rather than the rule, perhaps in the future it would be worth switching to per-package prefixes, and requiring the deps to specify dependent package prefixes where necessary. For CMake packages, which is most these days (including boost), I wonder if we could do something like this instead: diff --git a/depends/funcs.mk b/depends/funcs.mk
index 60e7889a4ec..921464b37ba 100644
--- a/depends/funcs.mk
+++ b/depends/funcs.mk
@@ -223,6 +223,7 @@ $(1)_cmake=env CC="$$($(1)_cc)" \
-DCMAKE_NM=`which $$($(1)_nm)` \
-DCMAKE_RANLIB=`which $$($(1)_ranlib)` \
-DCMAKE_INSTALL_LIBDIR=lib/ \
+ -DCMAKE_INSTALL_INCLUDEDIR=$(1)/include \
-DCMAKE_POSITION_INDEPENDENT_CODE=ON \
-DCMAKE_VERBOSE_MAKEFILE:BOOL=$(V) \
-DCMAKE_EXPORT_NO_PACKAGE_REGISTRY:BOOL=TRUE \That way the package is still found in the depends root prefix, but each cmake dep gets its own include path. A quick test shows that this works for boost but it's broken for zmq (doesn't find the includedir), libevent (doesn't install to the includedir), and libmultiprocess (not sure what the problem is there). Am I missing something, or are all of those really misbehaving? |
Member
Author
This approach seems fragile to me, as Regarding |
Member
|
Ok, thanks for confirming. FWIW, this does seem like the right thing for us to be doing, but apparently with CMake it's unlikely that a custom As an alternative, according to their docs, Boost explicitly supports building with Either way, please add a comment about why we're setting the var. |
Member
|
Also, nice work on the fix for zmq :) |
hebasto
changed the title
boost package and add missed Boost::headers
Member
Author
|
Thank you for the review! Your feedback has been addressed. Additionally, I applied the same approach to the The PR description has been updated. |
hebasto
force-pushed
the
251223-boost-layout
branch
from
2a69015 to
2921491
Compare
Member
|
Grr, the systemtap issue kinda reinforces my point about using What do you think about:
Seems to me: worst case (like our libevent version) it's simply ignored, some cases it's broken so we fix and upstream, best case it actually catches things. Assuming carrying any necessary patches is trivial enough, I don't really see any downside? |
Member
|
Ugh, there are actually several libmultiprocess issues. First, capnproto needs a fix: diff --git a/c++/src/capnp/CMakeLists.txt b/c++/src/capnp/CMakeLists.txt
index 9980fde6..8b93617e 100644
--- a/c++/src/capnp/CMakeLists.txt
+++ b/c++/src/capnp/CMakeLists.txt
@@ -71,3 +71,3 @@ target_include_directories(capnp INTERFACE
$<BUILD_INTERFACE:${PARENT_DIR}>
- $<INSTALL_INTERFACE:include>
+ $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
)
diff --git a/c++/src/kj/CMakeLists.txt b/c++/src/kj/CMakeLists.txt
index 48c49b74..bf30b00b 100644
--- a/c++/src/kj/CMakeLists.txt
+++ b/c++/src/kj/CMakeLists.txt
@@ -91,3 +91,3 @@ target_include_directories(kj PUBLIC
$<BUILD_INTERFACE:${PARENT_DIR}>
- $<INSTALL_INTERFACE:include>
+ $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
)Then, for some reason (I suspect I'm missing something else in capnproto), libmultiprocess still needs help finding them.. diff --git a/src/ipc/libmultiprocess/CMakeLists.txt b/src/ipc/libmultiprocess/CMakeLists.txt
index a36023b1810..89351deaecd 100644
--- a/src/ipc/libmultiprocess/CMakeLists.txt
+++ b/src/ipc/libmultiprocess/CMakeLists.txt
@@ -144,3 +144,4 @@ target_include_directories(mputil PRIVATE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
- $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/include>)
+ $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/include>
+ $<BUILD_INTERFACE:${CAPNP_INCLUDE_DIRECTORY}>)
target_link_libraries(mputil PUBLIC CapnProto::kj)
@@ -184,2 +185,3 @@ target_include_directories(multiprocess PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
+ $<BUILD_INTERFACE:${CAPNP_INCLUDE_DIRECTORY}>
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>)THEN, there's the issue of mpgen hard-coding capnproto include paths and not respecting I do think those things are worth fixing, but there's no need for that to get in the way of the other fixes. |
theuni
approved these changes
Jan 22, 2026
Member
Author
cc @ryanofsky |
Member
Author
|
My Guix build: |
hebasto
added a commit
that referenced
this pull request
Feb 5, 2026
a50d0b6 build: don't pass on boost dependency to kernel consumers (Cory Fields) Pull request description: This is unnecessary now that the kernel now exports a (boost-less) API. Noticed while slimming down boost dependencies in #34495. ACKs for top commit: stickies-v: ACK a50d0b6 hebasto: ACK a50d0b6, I have reviewed the code and it looks OK. I tested it by applying the Boost-specifc commits from #34143 and building with depends. Tree-SHA512: e2d12356f41dd51dd729362121a33bd4f395821d53dd9a0bb0d5d6a53aba2ca2064e0709d9799dc6751b3d61ea576d2efc0e28296fdba26f2809dbcb0feabe44
hebasto
force-pushed
the
251223-boost-layout
branch
from
2921491 to
65134c7
Compare
Member
Author
|
Rebased. |
Member
Author
|
My Guix build: |
Member
|
Guix Build (x86_64): d763a6657762eb0c77192c722c48bdd57473f5aa9cd12c3ac4e88fe509fbf130 guix-build-65134c7e5f99/output/aarch64-linux-gnu/SHA256SUMS.part
cebab577dd7a29ed13e20c6f3996062fd51caa9af33b15170094b1708318e770 guix-build-65134c7e5f99/output/aarch64-linux-gnu/bitcoin-65134c7e5f99-aarch64-linux-gnu-debug.tar.gz
ac0bbeeba7cfcc3618fc8e0f745747cace8505d1d7d23568d5707bdd5911897e guix-build-65134c7e5f99/output/aarch64-linux-gnu/bitcoin-65134c7e5f99-aarch64-linux-gnu.tar.gz
553a97634cae34da03f4c2d4173b38cad8a40fc2562b777faffbfbb624dbe245 guix-build-65134c7e5f99/output/arm-linux-gnueabihf/SHA256SUMS.part
ea8826e63a9d8ecaf71249019a33932ab58497dd1b3c11a936cf94ee55727d51 guix-build-65134c7e5f99/output/arm-linux-gnueabihf/bitcoin-65134c7e5f99-arm-linux-gnueabihf-debug.tar.gz
689235f63a2d80642225963d70cb5b902664a80066e2d61131d9345d5cd83ab2 guix-build-65134c7e5f99/output/arm-linux-gnueabihf/bitcoin-65134c7e5f99-arm-linux-gnueabihf.tar.gz
037fb650bf5f5c727e9577e0ff5072e32bd2b448fac2fdc23dba34fbc539e6f0 guix-build-65134c7e5f99/output/arm64-apple-darwin/SHA256SUMS.part
eb29f6eb0f738a9a53d76811e2fb66b90c4fa8df4cb661c30c25527157106d7e guix-build-65134c7e5f99/output/arm64-apple-darwin/bitcoin-65134c7e5f99-arm64-apple-darwin-codesigning.tar.gz
b8ea38b5ebd7a115eb7b08ea1343dc936e45ff29db0b14aacdc8a9d38a5d0eb8 guix-build-65134c7e5f99/output/arm64-apple-darwin/bitcoin-65134c7e5f99-arm64-apple-darwin-unsigned.tar.gz
4bee904bebcc0cf5a02081497563661b6c3927219e60e010694a9f4b2abf9e38 guix-build-65134c7e5f99/output/arm64-apple-darwin/bitcoin-65134c7e5f99-arm64-apple-darwin-unsigned.zip
e051ec06ccde1de606888e5a4d13764ccdd2aa1e7d9441d68e114ea3592195fe guix-build-65134c7e5f99/output/dist-archive/bitcoin-65134c7e5f99.tar.gz
5b09df6197915666a387b68bf3a7755d0ec6e9472fc4cdd03405e44395d32d94 guix-build-65134c7e5f99/output/powerpc64-linux-gnu/SHA256SUMS.part
3a0febf69d84c2bbe3309f0fb5de5d03edbad06d02432e36efc9bda8a9b6b1ce guix-build-65134c7e5f99/output/powerpc64-linux-gnu/bitcoin-65134c7e5f99-powerpc64-linux-gnu-debug.tar.gz
8ebe707831982e17fa27a06af9347c0673c3e8d1e6e677a770cf899533270123 guix-build-65134c7e5f99/output/powerpc64-linux-gnu/bitcoin-65134c7e5f99-powerpc64-linux-gnu.tar.gz
53127892268ff5d9f477de376d8b571bab2cdb7bb36c5f164c243775fb79c349 guix-build-65134c7e5f99/output/riscv64-linux-gnu/SHA256SUMS.part
bbbb8912ebcdf3c1212757d1e45920e25f589cfc9063483cf304c6ee5c32782e guix-build-65134c7e5f99/output/riscv64-linux-gnu/bitcoin-65134c7e5f99-riscv64-linux-gnu-debug.tar.gz
2b243d05774a7ea5f25fe3b836062c0554756cbd35ce451dede48b547782f5a1 guix-build-65134c7e5f99/output/riscv64-linux-gnu/bitcoin-65134c7e5f99-riscv64-linux-gnu.tar.gz
66afccee5ff0632c287af0e1db159d444a46e46d4d95166c68c5926afb468179 guix-build-65134c7e5f99/output/x86_64-apple-darwin/SHA256SUMS.part
6e1e4adff095097df241a493bbdc4a590fd83a6157791ae39fb57d99f7479f15 guix-build-65134c7e5f99/output/x86_64-apple-darwin/bitcoin-65134c7e5f99-x86_64-apple-darwin-codesigning.tar.gz
c87beeb30256230c02569aa65c4623b6931b481e9b8a51adca89be6f2ea9e964 guix-build-65134c7e5f99/output/x86_64-apple-darwin/bitcoin-65134c7e5f99-x86_64-apple-darwin-unsigned.tar.gz
150b416cf9112afee5c8d8b8c399b3074f2c126a0f4d55d12df032ecf05db23e guix-build-65134c7e5f99/output/x86_64-apple-darwin/bitcoin-65134c7e5f99-x86_64-apple-darwin-unsigned.zip
2795f17576ffac773fa094e3a610663583080629cc2043e8a40a2c4cf7c83bec guix-build-65134c7e5f99/output/x86_64-linux-gnu/SHA256SUMS.part
2a9ef15547c143b4be2781e398f469a0a1c04822312e3d252bf5ced4a15e5a3d guix-build-65134c7e5f99/output/x86_64-linux-gnu/bitcoin-65134c7e5f99-x86_64-linux-gnu-debug.tar.gz
9e95a377d89ac10733678dd9482e230aba88fcff162f160f3f1da46dcb177f54 guix-build-65134c7e5f99/output/x86_64-linux-gnu/bitcoin-65134c7e5f99-x86_64-linux-gnu.tar.gz
e5b9f32aa1589cb56672d2e0ae20d8e396ab2bc54dcfe3fea3bb2ab2faf3b768 guix-build-65134c7e5f99/output/x86_64-w64-mingw32/SHA256SUMS.part
1eed3bf1fb9053b1a5fee4447fcbb5e999992d6f29ea7c518581cddc15c06e1f guix-build-65134c7e5f99/output/x86_64-w64-mingw32/bitcoin-65134c7e5f99-win64-codesigning.tar.gz
658b17a936192ac454bc0bba5c3de25b3d8aea335a8b810a19d661c7545e0ef4 guix-build-65134c7e5f99/output/x86_64-w64-mingw32/bitcoin-65134c7e5f99-win64-debug.zip
f0aa03aa664b3198092a7ff908dd326233daebc13ec1f74741e386aa8fba9c6a guix-build-65134c7e5f99/output/x86_64-w64-mingw32/bitcoin-65134c7e5f99-win64-setup-unsigned.exe
21e9d4a1e5528d0d0cdc34b0aabb54d887f4b3d3d2f092d5b0c516de0f4c5f9a guix-build-65134c7e5f99/output/x86_64-w64-mingw32/bitcoin-65134c7e5f99-win64-unsigned.zip |
Member
Member
|
Concept ACK. Just bumped into |
Contributor
|
my guix build on nixos 25.11 aarch64 crashes for: Detailstried this 3 times, 2 clean starts (restore vm) and 1 restart of the build. On this PR 3 other hosts builded before crash. This nixos instance builds other PR's fine. |
Member
This very much looks like resource exhaustion/OOM. Note that it's dying while compiling |
Contributor
Retested with increased available memory to ~16GB (+5GB) and it builds successful. Thanks |
Member
|
ACK 65134c7 |
hebasto
commented
Feb 12, 2026
| $(package)_config_opts += -DBUILD_TESTING=OFF | ||
| $(package)_config_opts += -DCMAKE_DISABLE_FIND_PACKAGE_ICU=ON | ||
| # Install to a unique path to prevent accidental inclusion via other dependencies' -I flags. | ||
| $(package)_config_opts += -DCMAKE_INSTALL_INCLUDEDIR=$(package)/include |
Member
Author
There was a problem hiding this comment.
Apparently, this breaks builds on NetBSD due to a hack introduced in 5a5ddbd.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, header-only dependencies in the depends subsystem are installed into the standard
include/subdirectory. This inadvertently exposes their headers to the compiler via-Iflags brought in by other dependencies (e.g.,libeventorsqlite). This "include path pollution" masks missing dependencies in the build configuration. While the build might succeed by accident due to this overlap, it creates a fragile state. If the overlapping library is removed, the build will break, or, worse, the compiler may silently fall back to the host system's default paths (e.g.,/usr/include).This PR improves build system security and hygiene by enforcing strict, distinguished include paths for header-only dependencies. The missing dependencies revealed by this change (
Boost::headers,USDT::headers) have been fixed in separate commits.