[CI] Enable clang-tidy in precommit#21234
Conversation
I'm not sure if clang-tidy passes on all PRs, so for now introducing with continue-on-error (meaning it will be green even in case of failure).
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
sarnex
left a comment
sarnex
left a comment
There was a problem hiding this comment.
cool change, just a few comments
| if: steps.should_run_tidy.outputs.run == 'true' | ||
| run: | | ||
| # Remove commands containing "-D__INTEL_PREVIEW_BREAKING_CHANGES" to avoid running on the same file twice. | ||
| jq '[ .[] | select(.command | contains("-D__INTEL_PREVIEW_BREAKING_CHANGES") | not) ]' $GITHUB_WORKSPACE/build/compile_commands.json > $GITHUB_WORKSPACE/build/compile_commands.temp.json |
There was a problem hiding this comment.
Could we just pass -DSYCL_ENABLE_MAJOR_RELEASE_PREVIEW_LIB=Off instead of doing this?
There was a problem hiding this comment.
We might, but it looks like it requires some cmake file changes:
CMake Error at /home/sycl/llvm/sycl/cmake/modules/AddSYCLUnitTest.cmake:49 (get_target_property):
get_target_property() called with non-existent target "sycl-preview".
Call Stack (most recent call first):
/home/sycl/llvm/sycl/cmake/modules/AddSYCLUnitTest.cmake:166 (add_sycl_unittest_internal)
/home/sycl/llvm/sycl/unittests/ur/CMakeLists.txt:3 (add_sycl_unittest)
There was a problem hiding this comment.
I suggest we investigate this and apply as a follow-up. Left a TODO 088ab4f.
There was a problem hiding this comment.
Yeah seems like an existing cmake bug, this workaround is fine for now
There was a problem hiding this comment.
if you arent planning on investing the cmake bug yourself pls make a gh issue and assign it to me, thx
There was a problem hiding this comment.
I'll try to investigate this
There was a problem hiding this comment.
thanks, i expect we just need to check if the option is enabled before creating the cmake target or something
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| continue | ||
| # Check if any non-comment string was added. | ||
| for line in lines[4:]: | ||
| if line.startswith("+") and not line[1:].lstrip().startswith("//"): |
There was a problem hiding this comment.
in theory we also could consider block comments /* */ but those arent widely used and it's not like it's that bad if we run the job anyway
KornevNikita
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
Failures are unrelated. |
2 participants
I'm not sure if clang-tidy passes on all PRs, so for now introducing with continue-on-error (meaning it will be green even in case of failure).