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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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.
The idea to 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:
src/crypto/sha256_arm_shani.cpp
src/crypto/sha256_avx2.cpp
src/crypto/sha256_sse41.cpp
src/crypto/sha256_x86_shani.cpp
src/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?
diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs
index d9950c3fba..fa5ac0a597 100644
--- a/test/lint/test_runner/src/main.rs
+++ b/test/lint/test_runner/src/main.rs
@@ -106,10 +106,16 @@ fn lint_includes_build_config() -> LintResult {
- let defines_regex = check_output(Command::new("grep").args(["undef ", "--", config_path]))
- .expect("grep failed")
- .lines()
- .map(|line| {
- line.split("undef ")
- .nth(1)
- .expect(&format!("Could not extract name in line: {line}"))
- })
- .collect::<Vec<_>>()
- .join("|");
+ let defines_regex = format!(
+ "^(?!//).*({})",
+ check_output(Command::new("grep").args(["undef ", "--", config_path]))
+ .expect("grep failed")
+ .lines()
+ .map(|line| {
+ line.split("undef ")
+ .nth(1)
+ .expect(&format!("Could not extract name in line: {line}"))
+ })
+ .collect::<Vec<_>>()
+ .join("|")
+ );
+
+ println!("{}", defines_regex);
+
@@ -120 +126 @@ fn lint_includes_build_config() -> LintResult {
- "--extended-regexp",
+ "--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 :(
Nice, done. TIL about regex lookahead assertions.
Concept ACK f4db3546c003822e485f1597701f278eef197619.
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:
crypto/sha256.cpp should remove these lines:
- #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?
Yeah, good point. So I guess the only way to remove unused includes would be to write another check in the linter. I'll drop the last commit and leave everything else for a follow-up.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 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.
<sub>Debug: https://github.com/bitcoin/bitcoin/runs/21683771354</sub>
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.
Added the check to error on redundant (harmless) includes as well, since it was trivial. Should be trivial to re-ACK.
Rebased, for silent merge conflict in 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.
ACK fa58ae74ea67485495dbc2d003adfbca68d6869b
Added milestone, since this test can catch bugs, and it was reviewed.
ACK fa58ae74ea67485495dbc2d003adfbca68d6869b, tested on Ubuntu 23.10 -- it catches bugs properly. I didn't review rust code changes.
Milestone
27.0