Skip to content

Conversation

@fjetter
Copy link
Contributor

@fjetter fjetter commented Aug 30, 2017

This adds iwyu make target to run include-what-you-use on all changed files of the current branch. As suggested in the ticket https://issues.apache.org/jira/browse/ARROW-1413 I took the code of apache-kudu as a starting point and added a few modifications.

While it works fine locally, I'm struggling to get it to work (i.e. build is failing) on travis. Somehow the check isn't performed without raising an error. I expect that clang doesn't have the iwyu properly installed but I couldn't verify it, yet.

Also, the tool requires a compile_commands.json to be present including all the files to be checked which is why I added the export CMAKE_EXPORT_COMPILE_COMMANDS=1 to the travis_env_common.sh
This may be an issue since we are partially compiling multiple times but never the full codebase which is why I included the make/ninja iwyu twice.

@wesm
Copy link
Member

wesm commented Aug 30, 2017

I don't think it's necessary to run this on every build in Travis as it will primarily inflate our build times, only that we do a manual cleanup periodically (e.g. a couple times a release cycle)

@wesm
Copy link
Member

wesm commented Aug 31, 2017

@fjetter can you capitalize the "Arrow" in the PR title (and add hyphen so it reads "ARROW-1413:")? I will take this for a spin when I can and then merge, sometime in the next couple days

@xhochy
Copy link
Member

xhochy commented Aug 31, 2017

I took this for a spin and I'm not really sure this produces the expected results. At least we need to tune the iwyu configuration a lot.

@wesm
Copy link
Member

wesm commented Aug 31, 2017

We definitely should not be applying automated changes. iwyu does require a ton of tuning

@fjetter fjetter changed the title Arrow 1413: [C++] Add include-what-you-use configuration ARROW-1413: [C++] Add include-what-you-use configuration Sep 1, 2017
@fjetter
Copy link
Contributor Author

fjetter commented Sep 1, 2017

@wesm I'd remove the travis stuff again or do you want to keep it? Currently it doesn't do anything.

@wesm
Copy link
Member

wesm commented Sep 1, 2017

Can you remove it ("ninja iwyu", etc.) from the Travis scripts for now?

@wesm
Copy link
Member

wesm commented Sep 1, 2017

Forgot I have push permissions on your fork, I can take it from here

wesm added 2 commits September 1, 2017 11:51
Change-Id: Ib6cc65a8e86968c4b60c25144b4def8225e1412c
Change-Id: Ic7b3968999571974619b25d75d600f2c7ddcb486
@wesm
Copy link
Member

wesm commented Sep 1, 2017

+1. I will wait for the build to pass

@fjetter
Copy link
Contributor Author

fjetter commented Sep 1, 2017

@wesm if you want to invoke it on the whole code base you can simply add cmake -DCMAKE_CXX_INCLUDE_WHAT_YOU_USE=iwyu <iwyu args> to the cmake arguments. It takes care for you of the compilation database and prints the output of iwyu to stdout

c.f. https://cmake.org/cmake/help/v3.9/prop_tgt/LANG_INCLUDE_WHAT_YOU_USE.html?highlight=include

For including it in the build chain, though, I would've preferred a partial check.

@wesm
Copy link
Member

wesm commented Sep 1, 2017

I see. I wanted an easy option to run with the filters and suppressions. The option to do a partial run as with Kudu is still there (omit the "all" option from iwyu.sh). We can continue to tweak this as we venture forward, this is by no means final. Just want to get us all in the habit of keeping our includes clean

@asfgit asfgit closed this in 75d1f61 Sep 1, 2017
@wesm wesm deleted the ARROW-1413 branch September 1, 2017 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants