-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[cmake] fix condition for apple clang #2051
base: main
Are you sure you want to change the base?
Conversation
@ilya-lavrenov, CI results indicate that this is not the case for configuration we are testing. Which CMake version did you try? |
We use 3.13 as minimum required. |
Good question, we may have to bump the minimum required CMake version to at least 3.5 as the older versions have been deprecated in CMake 3.27. |
We are using CMake 2.8.12 as minimal supported version to ensure RHEL 7 compatibility. As RHEL 7 is now out of production support we could bump it. RHEL8 has 3.11.4 and Ubuntu 20.04 has 3.16.3. |
@@ -23,7 +23,7 @@ endif() | |||
set(OpenMP_cmake_included true) | |||
include("cmake/Threading.cmake") | |||
|
|||
if (APPLE AND CMAKE_CXX_COMPILER_ID STREQUAL "Clang") | |||
if (CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang") |
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.
Currently this check applied to all Clangs on macOS, after the change only to Apple provided Clang. If we want to retain the old logic, then MATCHES
would be a better option like: if (APPLE AND CMAKE_CXX_COMPILER_ID MATCHES "Clang")
. It should catch both Clangs.
Description
Cmake identifies Apple clang as "AppleClang"
Fixes # (github issue)
Checklist
General
make test
andmake test_benchdnn_*
) pass locally for each commit?Performance improvements
New features
Bug fixes
RFC PR