Minimum cmake version for blt

[kennyweiss]

We should define the minimum cmake version allowed for blt-based projects.

This minimum version should be tested/checked within blt, and should be clearly indicated in our documentation.

One of our core macros used for transitive dependencies is using a cmake 3.3 feature:

if(... IN_LIST ...)

This feature also requires setting cmake policy CMP0057 to NEW (as pointed out by @zbeekman in PR #108).

Note: Setting a value for CMP0057 is only valid in Cmake versions above 3.3 and is required in versions above 3.3.2 when using if(.. IN_LIST ...).

See: https://cmake.org/cmake/help/v3.3/policy/CMP0057.html

[zbeekman]

Where would be the most appropriate place to set CMP0057 to NEW. I'd like to include that in my PR (#108).

[kennyweiss]

Thanks -- I'd think it should be around here -- https://github.com/LLNL/blt/blob/master/SetupBLT.cmake#L63 -- in SetupBLT.cmake.

It should be guarded by the version until we decide about adding a cmake_minimum_version to blt (and which version that should be), since earlier cmake versions will complain about unknown policies.
E.g.

if("${CMAKE_VERSION}" VERSION_GREATER 3.3)
    cmake_policy(SET CMP0057 NEW)
endif()

[zbeekman]

I agree, but how do you propose working around IN_LIST? Without setting CMP0057 to new, using IN_LIST is essentially a syntax error. I'd rather have the unknown policy complaint to warn me that something may break. The alternative is to proceed anyway in previous versions of CMake that don't support IN_LIST at all. In fact, it caused CI failures when I introduced IN_LIST in my PR, and I'm surprised the other instance of IN_LIST hasn't caused a problem yet.

…

On Mon, Oct 30, 2017 at 4:23 PM Kenny Weiss ***@***.***> wrote: Thanks -- I'd think it should be around here -- https://github.com/LLNL/blt/blob/master/SetupBLT.cmake#L63 -- in SetupBLT.cmake. It should be guarded by the version until we decide about adding a cmake_minimum_version to blt (and which version that should be), since earlier cmake versions will complain about unknown policies. E.g. if("${CMAKE_VERSION}" VERSION_GREATER 3.3) cmake_policy(SET CMP0057 NEW)endif() — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#109 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAREPGPAuOeTsv4g_wWQ8LPBMIz4KdKfks5sxjAogaJpZM4QLni-> .

[kennyweiss]

I think that it is a feature that is not yet in our blt tests.
It comes up when there is a transitive dependency in the DEPENDS_ON lists.

Perhaps we should avoid resolving the policy part of this problem within BLT until the cmake_minimum_version is decided. In the meantime, it is easy to add both a minimum version and the cmake policy part to your project's build system.

[zbeekman]

Perhaps we should avoid resolving the policy part of this problem within BLT until the cmake_minimum_version is decided. In the meantime, it is easy to add both a minimum version and the cmake policy part to your project's build system.

OK, I'll hold off adding the policy in my PR and leave it as the more portable/flexible MATCHES variant.

[kennyweiss]

Thanks @zbeekman.

It looks like we will explicit require cmake-3.3+ for basic blt functionality by testing against ${CMAKE_VERSION} in blt and throwing a FATAL_ERROR for earlier versions.

We will most likely not be adding the cmake_minimum_version to blt since this macro has side effects, and is better left to the user projects that brings in blt.

It is also worth mentioning that some of the CUDA functionality requires a significantly newer version of cmake (e.g. cmake-3.8.x), but since this is an optional feature of blt, we will not make this a requirement for all blt users.

We will be adding documentation and internal checks for these in the near future.

[zbeekman]

Sounds like a good plan. FYI 3.7 or 3.8 introduces support for Fortean submodules too. Do you want me to include any of this in the PR I have open? I.e. `cmake_policy(SET CMP0057 NEW)` or checking `CMAKE_VERSION`?

…

On Mon, Oct 30, 2017 at 6:22 PM Kenny Weiss ***@***.***> wrote: Thanks @zbeekman <https://github.com/zbeekman>. It looks like we will explicit require cmake-3.3+ for basic blt functionality by testing against ${CMAKE_VERSION} in blt and throwing a FATAL_ERROR for earlier versions. We will most likely not be adding the cmake_minimum_version to blt since this macro has side effects, and is better left to the user projects that brings in blt. It is also worth mentioning that some of the CUDA functionality requires a significantly newer version of cmake (e.g. cmake-3.8.x), but since this is an optional feature of blt, we will not make this a requirement for all blt users. We will be adding documentation and internal checks for these in the near future. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#109 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAREPFvlArlTkJTutoZZ2xrlQYx3LNlsks5sxkw_gaJpZM4QLni-> .

[kennyweiss]

Let's make it a different PR to keep it separate from the fortran fixes.

[zbeekman]

Sure thing. They are somewhat related if we want to use IN_LIST, but that can be fixed later

…

On Mon, Oct 30, 2017 at 8:09 PM Kenny Weiss ***@***.***> wrote: Let's make is a separate PR to keep it separate from the fortran fixes. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#109 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAREPIi2Xm7Z3JhCg42qjAIUsH2JfE12ks5sxmU7gaJpZM4QLni-> .

[white238]

This maybe caused by gmock/gtest overriding the cmake minimum in their code. It is at the top level CMakeLists.txt.

cmake_minimum_required(VERSION 2.6.2)

This should probably be commented out in our code.

[white238]

cmake_minimum_required calls were removed and a check was added in BLT to warn if you are below CMake 3.8.