Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add IWYU to CI #17078

Merged
merged 42 commits into from
Nov 8, 2024
Merged

Add IWYU to CI #17078

merged 42 commits into from
Nov 8, 2024

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Oct 14, 2024 •
edited
Loading

Description

This PR adds include-what-you-use to the CI job running clang-tidy. Like clang-tidy, IWYU runs via CMake integration and only runs on cpp files, not cu files. This should help us shrink binaries and reduce compilation times in cases where headers are being included unnecessarily, and it helps keep our include lists clean. The IWYU suggestions for additions are quite noisy and the team determined this to be unnecessary, so this PR instead post-filters the outputs to only show the removals. The final suggestions are uploaded to a file that is uploaded to the GHA page so that it can be downloaded, inspected, and easily applied locally.

Resolves #581.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@vyasr vyasr added feature request New feature or request non-breaking Non-breaking change labels Oct 14, 2024
@vyasr vyasr self-assigned this Oct 14, 2024
@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue labels Oct 14, 2024
@vyasr vyasr changed the title Test Add IWYU to CI Oct 14, 2024
@vyasr vyasr force-pushed the feat/iwyu branch 2 times, most recently from b8e68c6 to 8f1fc50 Compare October 19, 2024 00:48
vyasr
vyasr commented Oct 19, 2024
View reviewed changes
ci/clang_tidy.sh Outdated Show resolved Hide resolved
@vyasr vyasr mentioned this pull request Oct 24, 2024
Merged
3 tasks
rapids-bot bot pushed a commit that referenced this pull request Oct 28, 2024
@vyasr
Remove includes suggested by include-what-you-use (#17170)
1ad9fc1 
This PR cherry-picks out the suggestions from IWYU generated in #17078.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - David Wendt (https://github.com/davidwendt)

URL: #17170
@vyasr vyasr force-pushed the feat/iwyu branch 2 times, most recently from 461228a to d9384f6 Compare October 30, 2024 22:26
vyasr
vyasr commented Oct 30, 2024
View reviewed changes
cpp/CMakeLists.txt
function(enable_clang_tidy target)
set(_tidy_options)
function(enable_static_checkers target)
set(_tidy_options IWYU CLANG_TIDY)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need these options, or should we just always enable all linters?

vyasr
vyasr commented Oct 30, 2024
View reviewed changes
cpp/.clang-tidy Show resolved Hide resolved
vyasr
vyasr commented Oct 30, 2024
View reviewed changes
cpp/.clang-tidy Show resolved Hide resolved
vyasr
vyasr commented Oct 30, 2024
View reviewed changes
ci/cpp_linters.sh Outdated Show resolved Hide resolved
@vyasr vyasr marked this pull request as ready for review October 30, 2024 22:40
@vyasr vyasr requested review from a team as code owners October 30, 2024 22:40
@vyasr
Copy link
Contributor Author

vyasr commented Oct 30, 2024

In addition to the inline notes above, there are a couple other points that I anticipate coming up from reviewers:

  • Ideally we should probably rewrite the parsing script so that it can be run live on the CI logs (i.e. pipe cmake output directly to the script) and then it would print all unrelated lines back out to stdout so that we would still get live logging but with IWYU output removed. Then, at the end we could just print out (in addition to uploading) the final compiled IWYU suggestions with all the undesired lines removed. That will take some reworking of the script, though, which currently relies on reading the output all at once. I didn't think it was worth holding that up right now, but if reviewers think it is worthwhile we can work on getting that change into this PR.
  • IWYU does support pragmas that we might want to add so that it stops suggesting any repeated changes.

I'll leave it to reviewers to determine what to gate this PR on vs. doing in follow-ups.

harrism
harrism approved these changes Oct 30, 2024
View reviewed changes
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Couple of comments.

ci/cpp_linters.sh Show resolved Hide resolved
cpp/.clang-tidy Show resolved Hide resolved
@vyasr
Copy link
Contributor Author

vyasr commented Nov 6, 2024

OK this was bothering me so I went ahead and updated the script to filter live rather than do a batch process at the end, which should make parsing the CI output much easier.

jameslamb
jameslamb approved these changes Nov 7, 2024
View reviewed changes
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Giving you a ci-codeowners / packaging-codeowners approval. Left one very very tiny suggestion, do whatever you want with it.

ci/cpp_linters.sh Outdated Show resolved Hide resolved
@vyasr @jameslamb
Update ci/cpp_linters.sh
4f07b7d 
Co-authored-by: James Lamb <[email protected]>
karthikeyann
karthikeyann approved these changes Nov 7, 2024
View reviewed changes
Copy link
Contributor

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried IWYU years ago. But it was not suitable because of too many changes it suggested.
Good approach to "only include removals". 👍
Including "tests" would be nice because often some includes are not cleaned up after development/debug.

@vyasr
Copy link
Contributor Author

vyasr commented Nov 8, 2024

/merge

@rapids-bot rapids-bot bot merged commit 7b80a44 into rapidsai:branch-24.12 Nov 8, 2024
103 checks passed
@vyasr vyasr deleted the feat/iwyu branch November 12, 2024 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwendt davidwendt davidwendt approved these changes

@PointKernel PointKernel PointKernel approved these changes

@jameslamb jameslamb jameslamb approved these changes

@karthikeyann karthikeyann karthikeyann approved these changes

@harrism harrism harrism approved these changes

Assignees

@vyasr vyasr

Labels
CMake CMake build issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
cuDF/Dask/Numba/UCX
Status: Done
libcudf
Status: Landed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Compile times are growing significantly
6 participants
@vyasr @harrism @karthikeyann @jameslamb @PointKernel @davidwendt