Noticed while testing a branch that replaces boost::multi_index with a custom replacement.
Currently depends builds pick up usdt and boost from the same path, and because boost always exists, the usdt path is implicitly included. So without boost, USDT isn’t found.
An alternative to this would be to disable USDT for the kernel. I’d be open to either approach.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hebasto, fanquake |
| Concept ACK | TheCharlatan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
Concept ACK
Good find, I think just including it like you’ve done here is ok for now.
ACK ccd10fdb97f9b8268a5cd60d7461967cfe536f16, the diff looks correct.
Ideally, such changes should be enforced by a failure to compile:
0/home/hebasto/git/bitcoin/src/util/trace.h:12:10: fatal error: sys/sdt.h: No such file or directory
1 8 | #include <sys/sdt.h>
2 | ^~~~~~~~~~~
3compilation terminated.
However, when building with depends, the compiler is still able to include a system-wide header:
0$ sudo apt install systemtap-sdt-dev
1$ make -C depends NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_NATPMP=1 NO_UPNP=1
2$ cmake -B build --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake -DBUILD_KERNEL_LIB=ON
3$ rm -rf depends/x86_64-pc-linux-gnu/include/sys
4$ cmake --build build # no compiler errors
Currently depends builds pick up usdt and boost from the same path, and because boost always exists, the usdt path is implicitly included. So without boost, USDT isn’t found.
To clarify for myself, the build currently picks up all depends dependencies from the same path (i.e something like -isystem /root/ci_scratch/depends/x86_64-pc-linux-gnu/include, (which is what is introduced with the change here)), so using any dependency will implictly include all others. It’s just the case that the kernel only had two, and it was also possible that if the depends -isystem was missing, it could be masked by the fact that a system installed usdt, happens to exist alongside libc, and such will also always be included, so no compile failure.
Ideally, such changes should be enforced by a failure to compile: However, when building with depends, the compiler is still able to include a system-wide header:
Are you saying you think we shouldn’t be able to include system headers when building using depends? It’s not clear how that’d work, unless we started vendoring our own libc/c++ etc.
Are you saying you think we shouldn’t be able to include system headers when building using depends?
Yes, for headers-only packages built in depends, such as Boost and USDT. It’s for the same reason, why we got rid of the ALLOW_HOST_PACKAGES variable.
Yes, for headers-only packages built in depends, such as Boost and USDT.
Headers-only shouldn’t make any difference here, and this should already be the case for Boost. If it’s not, that should be fixed. See my point above re USDT, I don’t understand how you propose to do this, given that the USDT headers exist amongst the libc headers.