-
Notifications
You must be signed in to change notification settings - Fork 170
[Github] [CIR] Add CI/CD for clang tidy [1/N] #1917
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to split the PR, first setup an action and then add changes incrementally always checking it passes the new action.
The first one should have tag [Github][CIR]
.github/workflows/clang-cir-lint.yml
Outdated
-DCMAKE_CXX_COMPILER_LAUNCHER=sccache \ | ||
-DCLANG_ENABLE_CIR=ON | ||
|
||
ninja -C build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we build the project, the only needed thing are just generated tablegen files maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL compile_commands.json doesn't need the full build to be generated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not seeing a clean way to generate all the necessary tablegen files for MLIR, except for this hack right here for now
ninja -C build_pilot $(ninja -C build_pilot -t targets all | grep IncGen | sed 's/:.*//') clang-tablegen-targets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're building 1k4 files with this approach so i'm not quite sure we need ccache or not here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a job that builds and run ninja check-clang-cir
, can't we run the clang-tidy after that and use the build artifacts from it? Not sure how easy is to connect these things, and it might not be the best to run it after we build (as opposed to faster bad formatting feedback), just dropping my 2 cents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i prefer keeping things separate as it's not adding on too much compile time (even w/o ccache) but I can't find any negatives about reusing artifacts except it's coupling CIs together more and slower formatting feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with the direction you are proposing
5c7b841
to
092ed9e
Compare
about the hacky For example, in
if we depend on the |
That was a great catch, fixing this dependency sounds like the way to go! |
Part of #1912
This PR only fixes 1 file. Also added a CI file to see if it'd run correctly