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.

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

build: migrate clang-tidy to LLVM 16 #3028

Closed
wants to merge 35 commits into from

Conversation

scarf005
Copy link
Member

@scarf005 scarf005 commented Jul 22, 2023

Summary

SUMMARY: Infrastructure "DDA Port: migrate clang-tidy plugin to LLVM 16"

Purpose of change

Make it easier to get custom clang-tidy plugin easier on local environment.

Describe the solution

ported following PRs:

all credits to: @BrettDong @jbytheway @Qrox

Describe alternatives you've considered

RIIR

Testing

image

confimed it was possible to use plugin built with local LLVM 16. CI may need to be adjusted a lot since i've patchworked a lot of files.

also lots of new checks are disabled as they were too time consuming and killed runners due to 6h limit. will be handled in separate PRs.

@scarf005 scarf005 force-pushed the modern-tidy branch 3 times, most recently from 9b37d03 to c239979 Compare July 22, 2023 13:05
@scarf005 scarf005 requested review from Coolthulhu and olanti-p July 22, 2023 13:38
@github-actions github-actions bot added src changes related to source code. tests changes related to tests labels Jul 22, 2023
@Qrox
Copy link
Contributor

Qrox commented Jul 22, 2023

You might want to also apply CleverRaven/Cataclysm-DDA#66315 and CleverRaven/Cataclysm-DDA#66538 which are follow-ups to CleverRaven/Cataclysm-DDA#65806.

@scarf005
Copy link
Member Author

thank you! will try tomorrow

Copy link
Contributor

@olanti-p olanti-p left a comment

Choose a reason for hiding this comment

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

Some checks definitely need to be disabled. The tidy runner seems to be timing out because of the sheer amount of new warnings.

build-scripts/build.sh Outdated Show resolved Hide resolved
.github/workflows/clang-tidy.yml Outdated Show resolved Hide resolved
.github/workflows/clang-tidy.yml Outdated Show resolved Hide resolved
build-scripts/clang-tidy.sh Outdated Show resolved Hide resolved
tools/clang-tidy-plugin/HeaderGuardCheck.cpp Outdated Show resolved Hide resolved
tools/clang-tidy-plugin/TranslateStringLiteralCheck.cpp Outdated Show resolved Hide resolved
tools/clang-tidy-plugin/TranslatorCommentsCheck.cpp Outdated Show resolved Hide resolved
tools/clang-tidy-plugin/TranslatorCommentsCheck.cpp Outdated Show resolved Hide resolved
tools/clang-tidy-plugin/test/translator-comments.cpp Outdated Show resolved Hide resolved
tools/clang-tidy-plugin/test/translator-comments.cpp Outdated Show resolved Hide resolved
@scarf005 scarf005 marked this pull request as draft August 5, 2023 04:23
@scarf005 scarf005 requested a review from olanti-p August 6, 2023 03:16
@scarf005 scarf005 marked this pull request as ready for review August 6, 2023 03:16
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Aug 6, 2023
originally added compiledb but removed them on rebase as it was unnecessary.

see: cataclysmbnteam#3028 (comment)
see: cataclysmbnteam#3028 (comment)

Co-authored-by: Olanti <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Aug 6, 2023
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Aug 6, 2023
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Aug 6, 2023
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Aug 6, 2023
@scarf005 scarf005 force-pushed the modern-tidy branch 2 times, most recently from b6e7964 to b9cf37e Compare August 6, 2023 07:47
@scarf005
Copy link
Member Author

scarf005 commented Oct 3, 2023

temporarily disabled 'check all affected files' for now (three header changed in this PR affects entire .cpp)

Copy link
Contributor

@olanti-p olanti-p left a comment

Choose a reason for hiding this comment

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

It doesn't seem to work at all now.

@scarf005
Copy link
Member Author

scarf005 commented Oct 5, 2023

aww, fixed. do you think it'd be ok to temporarily disable checking all affected source files and only run clang-tidy on changed files?

@olanti-p
Copy link
Contributor

olanti-p commented Oct 5, 2023

If you mean, disable checking .cpps if they were not changed but include a header that was changed, then no, it won't work this way.

C++ compilers (and clang-tidy checks by extension) need the entire preprocessed file to work with, and preprocessed file is created from .cpp and all its #included headers + defines passed to the compiler.

If that "affected files" check was disabled, then one PR could introduce error in header file that's only apparent when analyzing together with relevant .cpp, but CI would accept that, and then some other PR that happens to (correctly) modify that .cpp in a different place would trigger the check and get yelled at for no reason.

@olanti-p
Copy link
Contributor

olanti-p commented Oct 5, 2023

As I've mentioned before, you should start with fixing and/or disabling the checks added in this PR. If you check the build logs, the new checks spew billions upon billions of repeating errors, so it's quite possible the build times out simply because clang spends most of the time formatting error output.

BN builds 2-3 times faster than DDA, I say it's safe to expect our tidy could also run 2-3 times faster.

@scarf005
Copy link
Member Author

scarf005 commented Oct 5, 2023

image

a few days ago i tried disabling all new checks and still stumbled across 360m limit, as point.h, make_static.h, translations.h affected entire codebase. let's try again...

ref #3113: changed file detection relies on makefile includes target, will check if this could be moved to CMake as well, preferably using LLVM tooling in the following PR.

@scarf005 scarf005 requested a review from olanti-p October 7, 2023 11:50
@scarf005
Copy link
Member Author

scarf005 commented Oct 7, 2023

every new checks are disabled but it still hits time limit. this time limit hit is specifically caused due to changes in aforementioned 3 headers. which caused clang-tidy to run checks for every source files.

i think this PR should be merged despite that, as

@olanti-p
Copy link
Contributor

olanti-p commented Oct 7, 2023

I strongly disagree.

Tidy job works fine, while this PR in its current state breaks it completely.

Running tidy locally should not be a priority over running it in CI, because nobody runs it locally except when they're working on some tidy issue, but CI runs it for all C++ changes.

There's no immediate need to upgrade to tidy + LLVM 16, this PR is not a blocker for anything, it should be a self-contained upgrade, not downgrade.

If you claim you've disabled all the new checks and still it times out, takes more than 6 hours to complete, but at the current state of things tidy works and takes less than 2 hours for the entire run on all files, where does the >3 times increase come from?

@scarf005
Copy link
Member Author

keeping it draft until #2250 to reduce cognitive load

@scarf005 scarf005 marked this pull request as draft October 18, 2023 01:12
@github-actions github-actions bot added the docs PRs releated to docs page label Nov 16, 2023
@scarf005 scarf005 mentioned this pull request Aug 29, 2024
7 tasks
@scarf005 scarf005 mentioned this pull request Oct 3, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs PRs releated to docs page src changes related to source code. tests changes related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants