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

Introduce cmake variable MIOPEN_USE_CLANG_TIDY #3171

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

trixirt
Copy link

@trixirt trixirt commented Jul 29, 2024

While clang-tidy is useful for development to find issues, it is not always appropriate. When the clang-tidy used to build does not match the clang-tidy used in development the build can fail if the clang-tidy is not the same version as what development used. It also roughly doubles the build.

Introduce the cmake variable MIOPEN_USE_CLANG_TIDY to give the user the choice to use it our not. Set the default to ON to match the current behavior.

While clang-tidy is useful for development to find issues, it
is not always appropriate.  When the clang-tidy used to build does
not match the clang-tidy used in development the build can fail
if the clang-tidy is not the same version as what development used.
It also roughly doubles the build.

Introduce the cmake variable MIOPEN_USE_CLANG_TIDY to give the
user the choice to use it our not.  Set the default to ON to match the
current behavior.

Signed-off-by: Tom Rix <[email protected]>
@DrizztDoUrden
Copy link
Contributor

DrizztDoUrden commented Aug 2, 2024

If you want to do check except tidy you can do make cppcheck test, ninja cppcheck test, cmake --build . --target cppcheck && cmake --build . --target test or something like that
Is there some specific target that you have to build where tidy is unavoidable?
Plus, you can install any clang-tidy version alongside another and use it just in MIOpen
AFAIK tidy is not ran on any build, but maybe my environment is somehow different. Anyhow it is not intended to be.

@trixirt
Copy link
Author

trixirt commented Aug 2, 2024

I am building miopen as a package for Fedora, by the time it gets to me, all the development / compiler fixes should have been done. And Fedora's clang-tidy is will generally not the same clang-tidy used in development as warning/errors are thrown.

@averinevg
Copy link
Collaborator

@trixirt clang-tidy is not needed to build MIOpen. We use it during development. Please describe the steps to reproduce your problem.

@trixirt
Copy link
Author

trixirt commented Aug 16, 2024

The development environment needs to account for multiple versions of clang-tidy,
On Fedora rawhide it is
$ clang-tidy --version
LLVM (http://llvm.org/):
LLVM version 19.1.0
Optimized build.
On Fedora 40 it is
$ clang-tidy --version
LLVM (http://llvm.org/):
LLVM version 18.1.6
Optimized build.
On Fedora 39 it will 17.x etc.
So clang-tidy can not be generally used unless there is logic for specific versions of clang tidy.
Without this change, I would have to uninstall clang-tidy to build and then reinstall after the build.

@averinevg
Copy link
Collaborator

@trixirt I still don't get it. We have a separate CMake target for clang-tidy. You don't need to disable clang-tidy if you don't need it as it is not a dependency for other targets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants