Add Travis check for duplicate includes #11878

practicalswift · 2017-12-12T16:44:32Z

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

promag · 2017-12-14T03:48:23Z

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 · 2017-12-14T05:06:12Z

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 · 2017-12-14T08:40:17Z

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

ryanofsky · 2017-12-14T12:07:43Z

@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 · 2018-01-29T15:11:39Z

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 · 2018-02-26T16:53:46Z

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

practicalswift · 2018-02-26T18:09:30Z

@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 · 2018-03-09T15:23:53Z

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

sipa · 2018-03-09T17:35:06Z

utACK. Why is this not included in Travis?

practicalswift · 2018-03-10T17:24:30Z

@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 :-)

maflcko · 2018-03-11T21:10:39Z

utACK 4dbfc1a83f79e2208890501b75253b85730e56cc

eklitzke

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.

eklitzke · 2018-03-13T06:46:26Z

contrib/devtools/lint-includes.sh

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


Why are you using redirection here?

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? :-)

eklitzke · 2018-03-13T06:47:00Z

contrib/devtools/lint-includes.sh

+    if [[ ! -e $CPP_FILE ]]; then
+        continue
+    fi
+    DUPLICATE_INCLUDES_IN_HEADER_AND_CPP_FILES=$(grep -hE '^#include ' <(sort -u < "${HEADER_FILE}") <(sort -u < "${CPP_FILE}") | grep -E '^#include ' | sort | uniq -d)


Same comment wrt redirection

eklitzke · 2018-03-13T06:47:37Z

contrib/devtools/lint-includes.sh

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


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

practicalswift · 2018-03-18T17:48:06Z

@eklitzke Please re-review :-)

eklitzke

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.

eklitzke · 2018-03-19T06:59:16Z

contrib/devtools/lint-includes.sh

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


eklitzke · 2018-03-19T07:03:12Z

contrib/devtools/lint-includes.sh

+# Check for duplicate includes.
+
+EXIT_CODE=0
+for HEADER_FILE in $(git ls-files | grep -E "^src/.*\.h$" | grep -Ev "/(leveldb|secp256k1|univalue)/"); do


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 ...

practicalswift · 2018-03-19T07:36:39Z

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

eklitzke · 2018-03-20T07:35:32Z

utACK 745513ecb7a825a8129104eda382cbc82d35c097

maflcko · 2018-04-09T01:27:38Z

Needs rebase if still relevant

@sipa

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

practicalswift · 2018-04-09T07:19:36Z

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

Please re-review :-)

@sipa

c36b720 Add Travis check for duplicate includes (practicalswift) 280023f Remove duplicate includes (practicalswift) Pull request description: 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 ``` Tree-SHA512: f653d23c58ebc024dfc5b1fb8570698fd3c515c75b60b5cabbc43595548c488fca92349fa4c8b64460edbe61c879ff1d24f37f959e18552e202a7342460ddbf1

practicalswift · 2018-04-09T12:08:46Z

@MarcoFalke Thanks for merging! :-)

@sipa

c36b720 Add Travis check for duplicate includes (practicalswift) 280023f Remove duplicate includes (practicalswift) Pull request description: This enforces parts of the project header include guidelines (added by @sipa in bitcoin#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 ``` Tree-SHA512: f653d23c58ebc024dfc5b1fb8570698fd3c515c75b60b5cabbc43595548c488fca92349fa4c8b64460edbe61c879ff1d24f37f959e18552e202a7342460ddbf1

@sipa

c36b720 Add Travis check for duplicate includes (practicalswift) 280023f Remove duplicate includes (practicalswift) Pull request description: This enforces parts of the project header include guidelines (added by @sipa in bitcoin#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 ``` Tree-SHA512: f653d23c58ebc024dfc5b1fb8570698fd3c515c75b60b5cabbc43595548c488fca92349fa4c8b64460edbe61c879ff1d24f37f959e18552e202a7342460ddbf1

ad5717d Lint: Add lint-includes.sh (Fuzzbawls) faba3f6 [tests] Use FastRandomContext in scheduler_tests.cpp (practicalswift) 2034bf6 Remove unused boost includes in reverselock_tests.cpp (Fuzzbawls) 3256d16 bench: Use non-throwing ParseDouble() (Fuzzbawls) 3c984d1 Remove duplicate includes (Fuzzbawls) b54e396 Declare TorReply parsing functions in torcontrol_tests (Fuzzbawls) Pull request description: This brings in the `lint-includes.sh` shell script from upstream (first introduced in bitcoin#11878) to automatically check for duplicate includes and expanded in bitcoin#13301 and bitcoin#13385 to check for the inclusion of `.cpp` files and the introduction of new boost includes, respectively. The check for enforcing bracket include syntax (bitcoin#13230) is also included, but currently disabled, as we have yet to systematically switch to that syntax preference. Three other upstream PRs are backported here as they are directly related to the removal of some boost dependencies and are very straight forward: - bitcoin#10547 - bitcoin#13291 - bitcoin#13383 **NOTE: #2711 removes the dependency on `boost/tuple/tuple.hpp`, so it makes sense to merge that first. submitting this now so the general concept/functionality can be reviewed prior to that PR being merged** ACKs for top commit: furszy: good ACK ad5717d random-zebra: utACK ad5717d and merging... Tree-SHA512: d9d32d6d5122dac52c9601cda75612d87c4e027c6f196a2382206b227fcfd2bb61d4f72df7cbf5e572d94150ad8ca6db6301bd99b0da647b9627fe342b66873f

@sipa

c36b720 Add Travis check for duplicate includes (practicalswift) 280023f Remove duplicate includes (practicalswift) Pull request description: This enforces parts of the project header include guidelines (added by @sipa in bitcoin#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 ``` Tree-SHA512: f653d23c58ebc024dfc5b1fb8570698fd3c515c75b60b5cabbc43595548c488fca92349fa4c8b64460edbe61c879ff1d24f37f959e18552e202a7342460ddbf1

laanwj added the Build system label Dec 13, 2017

practicalswift force-pushed the lint-includes branch 2 times, most recently from c614e4c to b1b9c43 Compare December 13, 2017 09:27

practicalswift changed the title ~~Add Travis check for duplicate/redundant includes~~ Add Travis check for duplicateincludes Dec 13, 2017

practicalswift changed the title ~~Add Travis check for duplicateincludes~~ Add Travis check for duplicate includes Dec 13, 2017

practicalswift force-pushed the lint-includes branch from b1b9c43 to 4dbfc1a Compare December 13, 2017 09:39

maflcko mentioned this pull request Dec 13, 2017

Remove unused include in hash.cpp #11884

Merged

maflcko added Tests and removed Build system labels Dec 13, 2017

practicalswift mentioned this pull request Mar 11, 2018

Remove redundant includes. Conform to header include guidelines. #12542

Merged

eklitzke reviewed Mar 13, 2018

View reviewed changes

practicalswift force-pushed the lint-includes branch from 4dbfc1a to b1ef6bd Compare March 14, 2018 06:41

eklitzke reviewed Mar 19, 2018

View reviewed changes

practicalswift force-pushed the lint-includes branch from b1ef6bd to eb8ba7a Compare March 19, 2018 07:35

practicalswift force-pushed the lint-includes branch from eb8ba7a to 745513e Compare March 19, 2018 12:47

practicalswift added 2 commits April 9, 2018 09:18

Remove duplicate includes

280023f

Add Travis check for duplicate includes

c36b720

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

practicalswift force-pushed the lint-includes branch from 745513e to c36b720 Compare April 9, 2018 07:18

maflcko merged commit c36b720 into bitcoin:master Apr 9, 2018

maflcko mentioned this pull request Apr 11, 2018

doc: Refine header include policy #12933

Merged

ryanofsky mentioned this pull request Apr 12, 2018

doc: Revert to previous header include policy #12960

Closed

ryanofsky mentioned this pull request Feb 5, 2019

Remove wallet -> node global function calls #15288

Merged

maflcko mentioned this pull request Feb 19, 2019

Running iwyu on Bitcoin Core #15442

Closed

UdjinM6 mentioned this pull request Apr 16, 2020

Backports 0.16 pr17 dashpay/dash#3407

Merged

str4d mentioned this pull request Oct 27, 2020

Backport useful lints from upstream zcash/zcash#4829

Merged

practicalswift deleted the lint-includes branch April 10, 2021 19:34

Fuzzbawls mentioned this pull request Feb 7, 2022

[Lint] Introduce lint-includes.sh script PIVX-Project/PIVX#2740

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Add Travis check for duplicate includes #11878

Add Travis check for duplicate includes #11878

Uh oh!

Conversation

practicalswift commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Dec 14, 2017

Uh oh!

ryanofsky commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Dec 14, 2017

Uh oh!

practicalswift commented Jan 29, 2018

Uh oh!

luke-jr commented Feb 26, 2018

Uh oh!

practicalswift commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Mar 9, 2018

Uh oh!

sipa commented Mar 9, 2018

Uh oh!

practicalswift commented Mar 10, 2018

Uh oh!

maflcko commented Mar 11, 2018

Uh oh!

eklitzke left a comment

Choose a reason for hiding this comment

Uh oh!

eklitzke Mar 13, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift Mar 14, 2018

Choose a reason for hiding this comment

Uh oh!

eklitzke Mar 13, 2018

Choose a reason for hiding this comment

Uh oh!

eklitzke Mar 13, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Mar 18, 2018

Uh oh!

eklitzke left a comment

Choose a reason for hiding this comment

Uh oh!

eklitzke Mar 19, 2018

Choose a reason for hiding this comment

Uh oh!

eklitzke Mar 19, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Mar 19, 2018

Uh oh!

eklitzke commented Mar 20, 2018

Uh oh!

maflcko commented Apr 9, 2018

Uh oh!

practicalswift commented Apr 9, 2018

Uh oh!

practicalswift commented Apr 9, 2018

Uh oh!

Uh oh!

practicalswift commented Dec 12, 2017 •

edited

Loading

ryanofsky commented Dec 14, 2017 •

edited

Loading

practicalswift commented Dec 14, 2017 •

edited

Loading

practicalswift commented Feb 26, 2018 •

edited

Loading