[CI] Build clang-tidy in sycl-linux-build.yml#21219
Conversation
To have clang-tidy as part of nightly container
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| run: | | ||
| cmake --build $GITHUB_WORKSPACE/build --target install-sycl-test-utilities | ||
| - name: Install clang-tidy | ||
| if: ${{ !cancelled() && steps.build.conclusion == 'success' && contains(inputs.build_configure_extra_args, '-DSYCL_BUILD_CLANG_TIDY=ON') }} |
There was a problem hiding this comment.
I don't really like that we're checking the configure args to pick what target to install, I would prefer we just install everything, but I guess that wasn't introduced in this PR, the below case does it too.
There was a problem hiding this comment.
We could pass some input like "build_clang_tidy: true" instead. I don't want to build&install clang-tidy always as it takes some time and space. I'd like to limit it to nightly builds as we only need it there.
There was a problem hiding this comment.
The build for it seems to be only 12 extra files and the executable is 5.8MB based on my testing (static linking). Unless you have a strong opinion I would prefer we just always install it. The containers can't be used by external people anyway
sycl/CMakeLists.txt
Outdated
There was a problem hiding this comment.
I don't think this is right, we should build whatever target is required for this in the compile command, which should include SYCL and whatever other targets are enabled.
| cmake --build $GITHUB_WORKSPACE/build --target install-sycl-test-utilities | ||
| - name: Install clang-tidy | ||
| if: ${{ !cancelled() && steps.build.conclusion == 'success' && contains(inputs.build_configure_extra_args, '-DSYCL_BUILD_CLANG_TIDY=ON') }} | ||
| run: cmake --build $GITHUB_WORKSPACE/build --target install-clang-tidy |
There was a problem hiding this comment.
do we still need the cmake change with this line? i would expect the install-clang-tidy target to require it's actually built
There was a problem hiding this comment.
Yeah I decided we can just build "sycl-toolchain clang-tidy" instead of adding a dependency. I believe it's faster to build both targets together and then install clang-tidy, than to build sycl-toolchain and install-clang-tidy sequentially.
Anyways, if you think it's not right, we can just limit this patch to calling install-clang-tidy if the build_clang_tid input is set.
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
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
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
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KornevNikita
changed the title
|
Do we care about windows? |
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
I guess it's enough to run clang-tidy on linux |
2 participants
To have clang-tidy as part of nightly container.
Tested here: https://github.com/intel/llvm/actions/runs/21686159301