bitcoin
bitcoin copied to clipboard
bitcoin
build: Add missing USDT header dependency to kernel
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.
Code Coverage
For detailed information about the code coverage, see the test coverage report.
Reviews
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.
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #28690 (build: Introduce internal kernel library by TheCharlatan)
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.
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.