Missing bitcoin-config.h includes are problematic, because the build could silently pass, but produce an unintended result. For example, a slower fallback algorithm could be picked, even though bitcoin-config.h indicates that a faster feature is available and should be used.
As the build succeeds silently, this problem is not possible to detect with iwyu.
Thus, fix this by using a linter based on grepping the source code.
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 | theuni, TheCharlatan, hebasto |
| Concept ACK | epiccurious |
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.
grep was taken from theuni (https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1930911069)
Concept ACK.
The linter output matches my list, with the additions of:
0src/crypto/sha256_arm_shani.cpp
1src/crypto/sha256_avx2.cpp
2src/crypto/sha256_sse41.cpp
3src/crypto/sha256_x86_shani.cpp
4src/interfaces/wallet.h
The crypto defines actually come from our Makefile. Those have always been wonky, maybe now is a good time to do something about those.
src/interfaces/wallet.h however is a false-positive :(
src/interfaces/wallet.hhowever is a false-positive :(
Yeah, not sure how to solve this. Maybe rewrite everything as a clang-tidy plugin?
101+ assert!(Command::new("./autogen.sh")
102+ .status()
103+ .expect("command error")
104+ .success());
105+ }
106+ let defines_regex = check_output(Command::new("grep").args(["undef ", "--", config_path]))
How about this to at least filter out the easy commented lines?
0diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs
1index d9950c3fba..fa5ac0a597 100644
2--- a/test/lint/test_runner/src/main.rs
3+++ b/test/lint/test_runner/src/main.rs
4@@ -106,10 +106,16 @@ fn lint_includes_build_config() -> LintResult {
5- let defines_regex = check_output(Command::new("grep").args(["undef ", "--", config_path]))
6- .expect("grep failed")
7- .lines()
8- .map(|line| {
9- line.split("undef ")
10- .nth(1)
11- .expect(&format!("Could not extract name in line: {line}"))
12- })
13- .collect::<Vec<_>>()
14- .join("|");
15+ let defines_regex = format!(
16+ "^(?!//).*({})",
17+ check_output(Command::new("grep").args(["undef ", "--", config_path]))
18+ .expect("grep failed")
19+ .lines()
20+ .map(|line| {
21+ line.split("undef ")
22+ .nth(1)
23+ .expect(&format!("Could not extract name in line: {line}"))
24+ })
25+ .collect::<Vec<_>>()
26+ .join("|")
27+ );
28+
29+ println!("{}", defines_regex);
30+
31@@ -120 +126 @@ fn lint_includes_build_config() -> LintResult {
32- "--extended-regexp",
33+ "--perl-regexp",
I think this is a reasonable enough escape hatch. Given that we only have 1 currently, requiring a // FOO rather than a /* FOO */ seems ok to me.
I don’t see how we could do it as a clang-tidy plugin unfortunately. Its preprocessor stuff kinda sucks, and I believe we’d also have to keep a hard-coded list because any actual bitcoin-config.h would be guaranteed to leave some things undef’d. Hmm, I suppose the plugin itself could read the .h.in for each invocation. That’s pretty ugly though :(
Ok, this (The signed, previous to last commit, fa9c71aa914c683a6f4ee6aa91b8673e8eefd2e5) is ready for review.
I’ll drop the last commit (unsigned merge commit) once and if it is merged.
Tested 48562917172accd911e4c86f1032e4282fd321f5 on Ubuntu 23.10.
Missing #include <config/bitcoin-config.h> are reported correctly.
What are we supposed to do to automatically avoid redundant / unneeded includes?
What are we supposed to do to automatically avoid redundant / unneeded includes?
Iwyu is a tool to detect redundant and unneeded includes, so it can be used. However, there may be false warnings in regard to this specific header. Thus, an iwyu pragma can be used to avoid incorrect removal of needed includes. Should I push a commit here?
What are we supposed to do to automatically avoid redundant / unneeded includes?
Iwyu is a tool to detect redundant and unneeded includes, so it can be used. However, there may be false warnings in regard to this specific header. Thus, an iwyu pragma can be used to avoid incorrect removal of needed includes. Should I push a commit here?
I assume that both macros HAVE_TIMINGSAFE_BCMP and HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR are commented out in config/bitcoin-config.h in the CI environment.
I’m pretty sure that the same happens to the CHAR_EQUALS_INT8 macro as well. However, IWYU does not suggest to drop #include <config/bitcoin-config.h> from the serialize.h. That seems fragile as IWYU behavior might change in the future..
I suggest to add // IWYU pragma: keep everywhere and put this rule to the Developer Notes.
https://api.cirrus-ci.com/v1/task/5330278671974400/logs/ci.log:
0crypto/sha256.cpp should remove these lines:
1- #include <config/bitcoin-config.h> // lines 6-6
More // IWYU pragma: keep?
It’s not clear to me what the benefit of using IWYU is here. The missing defines will (by definition) vary per-platform, so I’m not sure what we’re chasing here exactly.
Or is it more-so just to keep IWYU usable without warnings for our c-i configs specifically?
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is 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.
Leave a comment here, if you need help tracking down a confusing failure.
Or is it more-so just to keep IWYU usable without warnings for our c-i configs specifically?
Yes. At some point in the future we might want to treat IWYU warnings as errors.
This is needed for a future change.
This will print a backtrace when an internal code error happened.
src/bench/wallet_ismine.cpp. I guess this immediately shows that the check is useful :)
It was included indirectly via src/wallet/test/util.h, however it is
better to include what you use.
Weak ACK fa58ae74ea67485495dbc2d003adfbca68d6869b.
My python-fu is weak, but I think this is a necessary check and it seems to work as advertised.