ci: Add IWYU job #33810
ci: Add IWYU job #33810
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33810. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
How can we improve the quality of the feedback (#33779 (comment))? 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 |
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. |
hebasto
force-pushed
the
251106-force-iwyu-ci
branch
from
f72b2f7 to
9bb013d
Compare
hebasto
force-pushed
the
251106-force-iwyu-ci
branch
from
9bb013d to
9b50213
Compare
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. |
Some basic instructions could be added after the |
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.
hebasto
force-pushed
the
251106-force-iwyu-ci
branch
from
9b50213 to
3ee4105
Compare
|
Rebased on top of the merged #33818. |
hebasto
force-pushed
the
251106-force-iwyu-ci
branch
from
3ee4105 to
bd48a10
Compare
|
The TSAN failure is #33318. |
| fi | ||
|
|
||
| if [ "${RUN_IWYU}" == "true" ]; then | ||
| # TODO: Consider enforcing IWYU across the entire codebase. |
There was a problem hiding this comment.
Maybe:
echo "^^^ ⚠️ Failure generated from IWYU"
echo
echo "Some adjustments to the diff may be needed:"
echo
echo "* Use the C++ headers: E.g. <cerrno> over <errno.h>"
echo '* Use #include <__.h> over #include "__.h" for all headers'
echo "* Sort the headers, so that they are in the right pre-existing section, if"
echo " there are any."The change in `src/crypto/hex_base.cpp` is because GCC 14 is not affected by an IWYU bug. See: include-what-you-use/include-what-you-use#1763.
hebasto
force-pushed
the
251106-force-iwyu-ci
branch
from
bd48a10 to
1f55824
Compare
5 participants
This PR seeks to address feedback raised in the discussion of #33779, specifically from this comment:
Based on ideas from #32953.