add support for appleclang and update cmake#582
add support for appleclang and update cmake#582gold-development wants to merge 1 commit intoapache:trunkfrom
Conversation
|
Mac os build seems broken . any plan to commit this ? |
|
Apologies, I've been distracted by too many things. @yifan-c can you take a look at this one as well? |
absurdfarce
left a comment
absurdfarce
left a comment
There was a problem hiding this comment.
A few questions but I'll defer to @yifan-c on the details since I know he's been working much more directly in this space.
| @@ -1,4 +1,4 @@ | |||
| cmake_minimum_required(VERSION 3.10) | |||
| cmake_minimum_required(VERSION 3.5) | |||
There was a problem hiding this comment.
Can you elaborate on a couple things here?
First: why is it necessary to change the cmake minimum version at all in order to fix this?
Second: if it is necessary to change it why are we tweaking it only in three CMakeLists files rather than uniformly throughout the project?
|
|
||
| if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang") | ||
| if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang" OR | ||
| "${CMAKE_CXX_COMPILER_ID}" STREQUAL "AppleClang") |
There was a problem hiding this comment.
I'll let him speak on this for sure but I believe @yifan-c was able to get this working more generally using regex matching:
if("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang$"
That's probably more desirable as it covers a broader range of clang derivatives. cmake supports a number of such variants so as long as the flags in question apply to clang generally (i.e. isn't some custom flag implemented for a specific clang variant) the more general syntax seems preferrable.
3 participants
Added support for AppleClang and update cmake version