This PR seeks to address feedback raised in the discussion of #33779, specifically from this comment:
Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.
Based on ideas from #32953.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33810.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | w0xlt |
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.
7@@ -8,7 +8,6 @@
8 #include <cassert>
9 #include <cstring>
10 #include <string>
11-#include <tuple>
I think both CI tasks should use the same config (debian:trixie). Otherwise, if someone tries to reproduce the CI config, they two configs will contradict each other, which doesn’t seem helpful?
Also, I find it confusing that one CI task is printing the iwyu errors and the other is printing the warnings. What is the goal here? It seems clearer to just print the errors and warnings in one task, like it was done before.
As mentioned previously, iwyu takes 9 minutes (https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3491592008) and in this pull it takes 30 seconds and you claim that it is faster. However, if the goal is to eventually run it on more stuff, it will take 9 minutes again. So I’d say we should also combine iwyu errors and warnings into one task, to be honest and accurate about the expected long-term runtime of the task.
providing convenient feedback to developers.
How can we improve the quality of the feedback (https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3496969502)? Getting it faster is nice, but if it can’t be taken and applied directly without fixing the sorting, and the formatting, and in the worse case, changing headers entirely for modernize-deprecated-headers, then it doesn’t seem that much more useful.
but if it can’t be taken and applied directly without fixing the sorting, and the formatting, and in the worse case, changing headers entirely for
modernize-deprecated-headers, then it doesn’t seem that much more useful.
I disagree. In practice, when a developer works on changes that modify includes, the diff in the includes would typically be just a few lines. Using the diff from the CI job as a hint seems entirely reasonable. I don’t see a strong need to increase the script’s complexity just to produce a diff that can be immediately applied.
43@@ -44,7 +44,7 @@ echo "=== END env ==="
44 # Don't apply patches in the tidy job, because it relies on the `git diff`
0# Don't apply patches in the iwyu job, because it relies on the `git diff`
nit, if you re-touch
just to produce a diff that can be immediately applied.
The diff can’t be applied at all, how wrong it is varies, and there are no instructions for devs on how to fix it.
instructions
Some basic instructions could be added after the echo "^^^ ⚠️ Failure generated from IWYU", if needed.
instructions
Some basic instructions could be added after the
echo "^^^ ⚠️ Failure generated from IWYU", if needed.
Happy to add them once someone suggests good wording.
The executable target run by ctest is `mptest`.
This change removes unnecessary steps when building the `codegen`
target.
The change in `src/crypto/hex_base.cpp` is because GCC 14 is not
affected by an IWYU bug.
See: https://github.com/include-what-you-use/include-what-you-use/issues/1763.
216@@ -213,7 +217,9 @@ if [ "${RUN_TIDY}" = "true" ]; then
217 echo "^^^ ⚠️ Failure generated from clang-tidy"
218 false
219 fi
220+fi
221
222+if [ "${RUN_IWYU}" == "true" ]; then
223 # TODO: Consider enforcing IWYU across the entire codebase.
Maybe:
0echo "^^^ ⚠️ Failure generated from IWYU"
1echo
2echo "Some adjustments to the diff may be needed:"
3echo
4echo "* Use the C++ headers: E.g. <cerrno> over <errno.h>"
5echo '* Use #include <__.h> over #include "__.h" for all headers'
6echo "* Sort the headers, so that they are in the right pre-existing section, if"
7echo " there are any."