buildsystem: fix support for cmake older than 3.8 #1428
Conversation
In e8b6b7a, we relaxed the cmake version check down to 3.1, the first version to expose target_compile_features(). However, an error while testing that change improperly concluded that the change was OK. While target_compile_features() was indeed intriduced in cmake 3.1, the actual feature we use it to test, cxx_std_11, was really introduced only with cmake-3.8, which explained the actual version that was requested before e8b6b7a. As Patrick noted, however, we can still convince cmake-3.1 to test for C++11, by testing for an actual feature introduced in C++11, like testing for cxx_range_for, which will instruct cmake to set the appropriate C++11 options for the current compiler, which for gcc would be adding -std=gnu++11. Reported-by: Patrick Boettcher <patrick.boettcher@posteo.de> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
|
@pboettch Could you please give your opinion on this? Thanks! ;-) |
|
I'd also like to know whether @pboettch is fine with the change. I am just concerned about the different semantics:
We could switch to if (${CMAKE_VERSION} VERSION_LESS "3.8.0")
target_compile_features(catch_main PUBLIC cxx_range_for)
else()
target_compile_features(catch_main PUBLIC cxx_std_11)
endif()to use |
nlohmann
added
the
state: please discuss
|
In the meantime I had a problem with another project with The conclusion there was to force CMake 3.8 and leave
In addition setting Hmm. Maybe for this library using There is also the property set_property(TARGET <tgt> PROPERTY CXX_STANDARD 11)
set_property(TARGET <tgt> PROPERTY CXX_STANDARD_REQUIRED ON) |
|
@pboettch @nlohmann Sorry guys, but this is straddling into C++ and cmake If it were me, I would just revert my previous change to at least restore the |
|
I agree - we should revert to the status quo, requiring 3.8.0 and using |
|
@nlohmann Sure, I'll send it in a moment. |
|
I would really like to be able to use a cmake version that's less recent. Some of us are still on Ubuntu 16.04, which ships with CMake 3.5.1. Using this library on Xenial requires modifying the CMakeLists just to change that feature requirement on the target. I've filed #1441, which has the |
|
Closed #1441 because of the point raised by @pboettch that the if statement will cause different behaviour depending on the CMake version, which is undesirable. I presume in this case that we weren't setting
Instead of picking |
|
As I said in your PR. There is no difference for CMake whether you put |
|
Oh, now I see what you mean. You are right, If deemed acceptable I'd be happy to file a PR that does: cmake_minimum_required(VERSION 3.1)
#...
target_compile_features(${NLOHMANN_JSON_TARGET_NAME} INTERFACE cxx_range_for) |
|
Fixed with #1441. |
nlohmann
added
solution: proposed fix
In e8b6b7a, we relaxed the cmake version check down to 3.1, the first
version to expose target_compile_features().
However, an error while testing that change improperly concluded that
the change was OK. While target_compile_features() was indeed intriduced
in cmake 3.1, the actual feature we use it to test, cxx_std_11, was
really introduced only with cmake-3.8, which explained the actual
version that was requested before e8b6b7a.
As Patrick noted, however, we can still convince cmake-3.1 to test
for C++11, by testing for an actual feature introduced in C++11, like
testing for cxx_range_for, which will instruct cmake to set the
appropriate C++11 options for the current compiler, which for gcc would
be adding -std=gnu++11.
Reported-by: Patrick Boettcher patrick.boettcher@posteo.de
Signed-off-by: "Yann E. MORIN" yann.morin.1998@free.fr