Add Travis check for duplicate includes #11878

practicalswift commented at 4:44 PM on December 12, 2017: contributor

This enforces parts of the project header include guidelines (added by @sipa in #10575).

Example run:

$ git diff
diff --git a/src/warnings.cpp b/src/warnings.cpp
index c52a1fd..d8994dd 100644
--- a/src/warnings.cpp
+++ b/src/warnings.cpp
@@ -5,6 +5,8 @@

 #include <sync.h>
 #include <clientversion.h>
+#include <string>
 #include <util.h>
 #include <warnings.h>
+#include <util.h>

diff --git a/src/warnings.h b/src/warnings.h
index e8e982c..8d2252e 100644
--- a/src/warnings.h
+++ b/src/warnings.h
@@ -7,6 +7,7 @@
 #define BITCOIN_WARNINGS_H

 #include <stdlib.h>
 #include <string>
+#include <stdlib.h>

 void SetMiscWarning(const std::string& strWarning);
$ contrib/devtools/lint-includes.sh
Duplicate include(s) in src/warnings.h:
#include <stdlib.h>

Include(s) from src/warnings.h duplicated in src/warnings.cpp:
#include <string>

Duplicate include(s) in src/warnings.cpp:
#include <util.h>
$ echo $?
1
$ git checkout .
$ contrib/devtools/lint-includes.sh
$ echo $?
0

laanwj added the label Build system on Dec 13, 2017

practicalswift force-pushed on Dec 13, 2017

practicalswift renamed this:
~~Add Travis check for duplicate/redundant includes~~
Add Travis check for duplicateincludes
on Dec 13, 2017

practicalswift renamed this:
~~Add Travis check for duplicateincludes~~
Add Travis check for duplicate includes
on Dec 13, 2017

practicalswift force-pushed on Dec 13, 2017

MarcoFalke added the label Tests on Dec 13, 2017

MarcoFalke removed the label Build system on Dec 13, 2017

promag commented at 3:48 AM on December 14, 2017: member

I just want to point that sometimes the include in the header is unnecessary if a forward declaration is possible. With this check the natural fix is to remove the include from the source. @practicalswift have you checked Iwyu as referenced in #10575?

ryanofsky commented at 5:06 AM on December 14, 2017: member

have you checked Iwyu as referenced in #10575?

I've used iwyu with bitcoin a few times before, though not since we switched from "" to <> style includes. It 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.

Steps to install:

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

Steps to run:

cd ~/src/bitcoin/src
make
rm -v interface/libbitcoin_util_a-chain.o # run iwyu on interface/chain.cpp
make -k CXX="/usr/lib/llvm-3.8/bin/include-what-you-use -std=c++11" 2>&1 | tee /tmp/iwyu
~/src/iwyu/fix_includes.py < /tmp/iwyu

practicalswift commented at 8:40 AM on December 14, 2017: contributor

@promag Yes, iwyu is nice but I'm afraid it could be too slow to run as part of a Travis session?

ryanofsky commented at 12:07 PM on December 14, 2017: member

@promag Yes, iwyu is nice but I'm afraid it could be too slow to run as part of a Travis session?

Even if it were fast, output isn't really useful or stable enough to run in an automated way. IWYU is most useful as a manual tool to remove unused includes after you add a new source file or split up a existing one. In any case, as instructions above indicate it is awkward to run, and it IMO would have to be a lot easier to run locally to even consider running it on travis.

practicalswift commented at 3:11 PM on January 29, 2018: contributor

Any chance of getting this reviewed? Let me know if it is a NACK and I'll close :-)

FWIW, these types of duplicate includes have been recurring and this check is very quick so the costs of introducing it is hopefully near zero :-)

luke-jr commented at 4:53 PM on February 26, 2018: member

What? This contradicts the guidelines. If warnings.h references std::string and warnings.cpp uses std::string, both of them should include <string>

practicalswift commented at 6:09 PM on February 26, 2018: contributor

@luke-jr From the header include guideline:

One exception is that a .cpp file does not need to re-include the includes already included in its corresponding .h file.

practicalswift commented at 3:23 PM on March 9, 2018: contributor

@MarcoFalke Would you be willing to review (context: I saw that you mentioned this PR in #11884)? :-)

sipa commented at 5:35 PM on March 9, 2018: member

utACK. Why is this not included in Travis?

practicalswift commented at 5:24 PM on March 10, 2018: contributor

@sipa contrib/devtools/lint-all.sh which is included in Travis runs contrib/devtools/lint-*.sh, so no need to include this script explicitly in the Travis conf :-)

MarcoFalke commented at 9:10 PM on March 11, 2018: member

utACK 4dbfc1a83f79e2208890501b75253b85730e56cc

in contrib/devtools/lint-includes.sh:11 in 4dbfc1a83f outdated

   6 | +#
   7 | +# Check for duplicate includes.
   8 | +
   9 | +EXIT_CODE=0
  10 | +for HEADER_FILE in $(git ls-files | grep -E '^src/.*\.h$' | grep -Ev '/(leveldb|secp256k1|univalue)/'); do
  11 | +    DUPLICATE_INCLUDES_IN_HEADER_FILE=$(grep -E '^#include ' < "${HEADER_FILE}" | sort | uniq -d)

eklitzke commented at 6:46 AM on March 13, 2018:

Why are you using redirection here?

practicalswift commented at 6:37 AM on March 14, 2018:

Do you mean why I'm storing the output in DUPLICATE_INCLUDES_IN_HEADER_FILE? :-) That is the only redirection taking place here, right? :-)

I need the output to print it below. I'm guessing that's not what you meant? :-)

in contrib/devtools/lint-includes.sh:22 in 4dbfc1a83f outdated

  17 | +    fi
  18 | +    CPP_FILE=${HEADER_FILE/%\.h/.cpp}
  19 | +    if [[ ! -e $CPP_FILE ]]; then
  20 | +        continue
  21 | +    fi
  22 | +    DUPLICATE_INCLUDES_IN_HEADER_AND_CPP_FILES=$(grep -hE '^#include ' <(sort -u < "${HEADER_FILE}") <(sort -u < "${CPP_FILE}") | grep -E '^#include ' | sort | uniq -d)

eklitzke commented at 6:47 AM on March 13, 2018:

Same comment wrt redirection

in contrib/devtools/lint-includes.sh:36 in 4dbfc1a83f outdated

  27 | +        EXIT_CODE=1
  28 | +    fi
  29 | +done
  30 | +for CPP_FILE in $(git ls-files | grep -E '^src/.*\.cpp$' | grep -Ev '/(leveldb|secp256k1|univalue)/'); do
  31 | +    DUPLICATE_INCLUDES_IN_CPP_FILE=$(grep -E '^#include ' < "${CPP_FILE}" | sort | uniq -d)
  32 | +    if [[ ${DUPLICATE_INCLUDES_IN_CPP_FILE} != "" ]]; then

eklitzke commented at 6:47 AM on March 13, 2018:

You're using '' in some places and "" in others

eklitzke commented at 6:52 AM on March 13, 2018: contributor

Besides the pedantic shell comments I gave, I don't think this is right because you're not going to be able to handle preprocessor statements adequately with this approach.

practicalswift force-pushed on Mar 14, 2018

practicalswift commented at 5:48 PM on March 18, 2018: contributor

@eklitzke Please re-review :-)

in contrib/devtools/lint-includes.sh:3 in b1ef6bdcd4 outdated

   0 | @@ -0,0 +1,39 @@
   1 | +#!/bin/bash
   2 | +#
   3 | +# Copyright (c) 2017 The Bitcoin Core developers

eklitzke commented at 6:59 AM on March 19, 2018:

nit: 2018

in contrib/devtools/lint-includes.sh:10 in b1ef6bdcd4 outdated

   5 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   6 | +#
   7 | +# Check for duplicate includes.
   8 | +
   9 | +EXIT_CODE=0
  10 | +for HEADER_FILE in $(git ls-files | grep -E "^src/.*\.h$" | grep -Ev "/(leveldb|secp256k1|univalue)/"); do

eklitzke commented at 7:03 AM on March 19, 2018:

nit: you could do this and the change below as a function, and pass in the .h or .cpp'ness in a bash function, e.g.

filter_suffix() { git ls-files | grep -E "^src/.*\.${1}"'$' | grep -Ev "/(leveldb|secp256k1|univalue)/"; }

And then below would become:

for HEADER_FILE in $(filter_suffix h); do
...

for CPP_FILE in $(filter_suffix cpp); do
...

eklitzke commented at 7:11 AM on March 19, 2018: contributor

Honestly looks good, sorry for the bash nits. For the nit-feedbackness also feel free to ping me on freenode my handle is eklitzke. Sometimes GitHub code reviews can get queued up, but for minor changes pinging on IRC works.

practicalswift force-pushed on Mar 19, 2018

practicalswift commented at 7:36 AM on March 19, 2018: contributor

@eklitzke Good points. Now updated. Please re-review :-)

practicalswift force-pushed on Mar 19, 2018

eklitzke commented at 7:35 AM on March 20, 2018: contributor

utACK 745513ecb7a825a8129104eda382cbc82d35c097

MarcoFalke commented at 1:27 AM on April 9, 2018: member

Needs rebase if still relevant

Remove duplicate includes 280023f31d

Add Travis check for duplicate includes

This enforces parts of the project header include guidelines (added by @sipa in #10575).

c36b720d00

practicalswift force-pushed on Apr 9, 2018

practicalswift commented at 7:19 AM on April 9, 2018: contributor

Rebased and removed 5+ few recently introduced duplicate includes. Still very much relevant :-)

Please re-review :-)

MarcoFalke merged this on Apr 9, 2018

MarcoFalke closed this on Apr 9, 2018

MarcoFalke referenced this in commit a04440feb9 on Apr 9, 2018

practicalswift commented at 12:08 PM on April 9, 2018: contributor

@MarcoFalke Thanks for merging! :-)

PastaPastaPasta referenced this in commit 949d1685e3 on Jun 17, 2020

PastaPastaPasta referenced this in commit 005d241fea on Jul 2, 2020

zkbot referenced this in commit 43ac2062f9 on Oct 28, 2020

zkbot referenced this in commit 84a5830aaa on Nov 9, 2020

practicalswift deleted the branch on Apr 10, 2021

random-zebra referenced this in commit 0e757ad2c9 on Feb 9, 2022

gades referenced this in commit 629cc7dc53 on Feb 21, 2022

DrahtBot locked this on Aug 16, 2022