Running iwyu on Bitcoin Core

MarcoFalke commented at 3:19 pm on February 19, 2019: member

There seems to be a lot of time wasted on discussing what headers to include and what headers not to include, so it would be nice to just have a tool that does that for us. iwyu is probably the closest to what I am looking for, but it is not straightforward to set up (see instructions below for Ubuntu). Let me know if the workflow could be simplified, so that is actually easily usable (maybe also on different distros).

iwyu makes a lot of changes, so it’s best to run it manually and selectively on individual source files, rather than in some automated place like a travis script.

To install on Ubuntu/Debian: apt install iwyu.

Steps to install:

0sudo apt install libclang-dev
1cd ~/src
2git clone github:include-what-you-use/include-what-you-use.git iwyu
3cd iwyu
4git checkout clang_3.8
5mkdir build && cd build
6cmake -DIWYU_LLVM_ROOT_PATH=/usr/lib/llvm-3.8 ..
7make
8sudo cp -aiv ~/src/iwyu/build/include-what-you-use /usr/lib/llvm-3.8/bin/

Steps to run:

0cd ~/src/bitcoin/src
1make
2rm -v interface/libbitcoin_util_a-chain.o # run iwyu on interface/chain.cpp
3make -k CXX="iwyu -std=c++17" 2>&1 | tee /tmp/iwyu
4~/src/iwyu/fix_includes.py < /tmp/iwyu

Taken from #11878 (comment)

MarcoFalke added the label Brainstorming on Feb 19, 2019

MarcoFalke added the label Docs on Feb 19, 2019

MarcoFalke added the label Build system on Feb 19, 2019

MarcoFalke commented at 3:24 pm on February 19, 2019: member

See also our section on #includes in the dev notes: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#source-code-organization

practicalswift commented at 3:56 pm on February 19, 2019: contributor

Concept ACK on introducing a mechanical tiebreaker mechanism (such as iwyu). Ideally automatically enforced. That way we’ll never have to think about this ever again :-)

Sjors commented at 5:32 pm on February 19, 2019: member

Concept ACK. Even having this only on the Travis linter instance helps.

I’ve ran into some pretty confusing bugs because of #include mistakes. The last one involved a #ifdef SOMETHING getting skipped because I forgot to include the file that set it, whereas in a different file I was working on #ifdef SOMETHING was set, because it coincidentally did include that file via two or three steps.

cc @promag

promag commented at 0:36 am on March 23, 2019: member

Is the idea to check that touched files have all “iwyu” includes?

Or is the idea to prevent adding unnecessary includes (not really used)?

elichai commented at 1:05 pm on January 2, 2020: contributor

I don’t think we can add IWYU to CI because: A. it sometimes generate false negatives. B. it does the same for boost includes which then fail building because they aren’t meant to be used that way (ie only use high level headers and not more “internal” ones)

but. I do think that a single PR for sorting this out is important(will open soon for feedback) and then trying to notice in review the following things:

New includes obviously, might be unnecessary with a simple forward declaration.
New types/functions used in the file that might require new includes even if it compile (IWYU).

Another option that can work is adding the necessary comments to “fix” IWYU on the project but idk how much people will like this.

MarcoFalke commented at 3:13 pm on January 2, 2020: member

Oh, could you actually get useful output? Last time I tried it was a mess and not really useful or applicable to our codebase.

elichai commented at 3:16 pm on January 2, 2020: contributor

Oh, could you actually get useful output? Last time I tried it was a mess and not really useful or applicable to our codebase.

it is a mess. you still need to manually go over all the files and change things, i’ll open a PR but I don’t think even as is after the change people will like it much (validation gets dozens of includes and it kinda bypasses things like fs.h) but we can trim it done to an acceptable change :)

elichai commented at 6:25 pm on May 5, 2020: contributor

If anyone’s interested I looked into this, and tried running iwyu on core and changing the code until it satisfies, I encountered 2 things: A. By default there’s a lot of false positives and false negatives, it can sometimes ask you to include a stl or boost internal header, which might not even compile by itself. Because of that the “autofix” leaves you with code that doesn’t compile and doesn’t even make sense some times, so you must “fix” these manually. This leads me to B B. I Haven’t finished the codebase (but I think I did most) and I already got to: 457 files changed, 4191 insertions(+), 1663 deletions(-).

So B makes a hard bargain without any performance advantages, and A makes B unmaintainable (there’s no simple CI script we can run that will help us keep this because of the amount of false negatives/positives)

These are my findings in the matter :) (I can push those changes somewhere if anyone wants to look/see)

ryanofsky commented at 7:06 pm on May 5, 2020: member

A makes B unmaintainable (there’s no simple CI script we can run that will help us keep this because of the amount of false negatives/positives)

I used to use IWYU a lot and I’m not sure what problems there are with iwyu + bitcoin + boost/stl, but presumably these are fixable and the place to fix them is the upstream mappings, e.g.: https://github.com/include-what-you-use/include-what-you-use/blob/master/boost-all.imp

For special cases iwyu has pragmas that sometimes make sense for overriding default behavior https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md

So B makes a hard bargain

If diffs actually make sense and are a one-time change that doesn’t have to be repeated, I don’t think they should be a hard bargain just because they touch a lot of #include lines. Different story of course if the changes are nonsensical or not easily maintainable

hebasto commented at 12:13 pm on September 14, 2021: member

Concept ACK.

vasild commented at 8:16 am on September 15, 2021: member

Concept ACK

MarcoFalke commented at 5:53 pm on December 9, 2021: member

From IRC (no context):

0[09:37] <_aj_> fwiw, i've found:    make -k CXX="iwyu -Xiwyu --max_line_length=300 -Xiwyu --transitive_includes_only -Xiwyu --no_fwd_decls -std=c++17" libbitcoin_server_a-net_processing.o     to work okay (as long as you remember to touch the .cpp or rm the .o first so make doesn't no-op it); still have to check it manually though

MarcoFalke commented at 1:48 pm on December 10, 2021: member

On Ubuntu/Debian, install can be as simple as apt install iwyu.

MarcoFalke commented at 3:09 pm on December 10, 2021: member

I doubt it will be possible to use this unsupervised any time soon, so closing for now

MarcoFalke closed this on Dec 10, 2021

DrahtBot locked this on Dec 10, 2022

Running iwyu on Bitcoin Core #15442