[CMake] Fix clang-tidy integration for non-Docker builds #122
Conversation
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
sjain-stanford
left a comment
sjain-stanford
left a comment
There was a problem hiding this comment.
I did see this when building outside docker too (due to the Catch2 install like you mentioned). Glad you're fixing it.
|
|
||
| set_target_properties(${target} PROPERTIES | ||
| CXX_CLANG_TIDY | ||
| "clang-tidy;-warnings-as-errors=*;--config-file=${PROJECT_SOURCE_DIR}/.clang-tidy" |
| # Enable clang-tidy. | ||
| fusilli_enable_clang_tidy(libutils) |
There was a problem hiding this comment.
nit: move after target_link_libraries below
| QUIET | ||
| GIT_REPOSITORY https://github.com/catchorg/Catch2.git | ||
| GIT_TAG v3.11.0 | ||
| SYSTEM |
There was a problem hiding this comment.
Add a comment why we want this to be SYSTEM - is it to suppress warnings originating from Catch2?
| ) | ||
|
|
||
| # Enable clang-tidy. | ||
| fusilli_enable_clang_tidy(${_RULE_NAME}) |
There was a problem hiding this comment.
This gets applied to add_fusilli_samples, add_fusilli_tests and add_fusilli_lit_test. What about add_fusilli_benchmark?
| #ifndef WORKSPACE_SAMPLES_LAYERNORM_LAYERNORM_UTILS_H | ||
| #define WORKSPACE_SAMPLES_LAYERNORM_LAYERNORM_UTILS_H | ||
| #ifndef FUSILLI_SAMPLES_LAYERNORM_LAYERNORM_UTILS_H | ||
| #define FUSILLI_SAMPLES_LAYERNORM_LAYERNORM_UTILS_H |
There was a problem hiding this comment.
I think I know why this was using WORKSPACE_ prefix: FUSILLI_ prefix is used for the library sources inside include/. For things outside, I'm fine using FUSILLI_ too as long as it's done consistently. Did llvm-header-guard have issues with one or the other?
| -misc-no-recursion, | ||
| -misc-use-anonymous-namespace, | ||
| -llvm-else-after-return, | ||
| -llvm-header-guard, |
There was a problem hiding this comment.
Was the issue only for code outside include/fusilli/? If so, I wonder if there's a way to disable this only for targets other than libfusilli. Or just use the NOLINTNEXTLINE exclusion for the few guards outside libfusilli. This has been useful in catching header guard naming issues so want to see if we can keep it.
2 participants
When
-DFUSILLI_ENABLE_CLANG_TIDY=ONis used outside the docker setup, builds failed with clang-tidy errors from Catch2. This occurred becauseCMAKE_CXX_CLANG_TIDYwas set globally, causing clang-tidy to analyze all targets created after it was set, including third-party libraries added viaFetchContent. CI builds worked because Catch2 was pre-installed in the Docker image, so it wasn't built from source and clang-tidy never ran on it.Changes:
CMAKE_CXX_CLANG_TIDY, clang-tidy is now enabled explicitly on project-owned targets only using theCXX_CLANG_TIDYtarget propertyllvm-header-guardcheck: This check was causing errors in sample files and the check is only really intended to work with LLVM. See [Fusilli] Add clang-tidy checks to CI nod-ai/amd-shark-ai#2607 (comment) for details.samples/layernorm/layernorm_utils.handtests/hip_tests/hip_kernels.hto use consistentFUSILLI_prefix.