buildsystem: relax requirement on cmake version

[yann-morin-1998]

Commit 73cc508 (Using target_compile_features to specify C++ 11
standard) bumped the required cmake version, from 3.0 to 3.8, so
as to get the definition of target_compile_features().

However, target_compile_features() was introduced in cmake-3.1:
https://cmake.org/cmake/help/v3.1/command/target_compile_features.html

And using cmake-3.1 is indeed sufficient to properly build.

As such, relax the minimum required version down to cmake-3.1,
so we can build on oldish, entreprise-grade distributions that
only have cmake-3.1 (or at least, don't have up to cmake-3.8).

Reported-by: Thomas Petazzoni thomas.petazzoni@bootlin.com
Signed-off-by: "Yann E. MORIN" yann.morin.1998@free.fr

[coveralls]

Coverage remained the same at 100.0% when pulling e8b6b7a on yann-morin-1998:yem/cmake-version into c682b98 on nlohmann:develop.

[nlohmann]

Thanks for the PR! I’ll check it soon.

[nlohmann]

Looks good to me.

[nlohmann]

Thanks!

[yann-morin-1998]

@nlohmann Thanks for the quick handling of the MR! :-)

Happy New Year and best wishes!

[pboettch]

Sorry to be late, but I think this PR will actually break things for old CMake-versions. Indeed target_compile_features() has been added to CMake 3.1. However, the requested feature by this library (cxx_std_11) has only been added in CMake 3.8. See the changelog of cmake 3.8

One possible solution before 3.8 to solve a dependency to C++11 with target_compile_features was requesting a feature which implied C++11-support for compiler such as cxx_range_for.

[nlohmann]

@pboettch Thanks for reporting!

[yann-morin-1998]

@pboettch Indeed, cxx_std_11 was added only with 3.8.0. I only
checked for the addition of target_compile_features(). My bad, sorry.

However, I've actually been able to build with just cmake-3.1.

[pboettch]

GCC from version 5 on enables C++11 by default, however, cmake should moan about the unknown feature.

[pboettch]

Error on my side: even gcc 5.4 does NOT (edit) build c++11 by default.

[pboettch]

I just tried on a Ubuntu 14.04 platform which uses cmake 3.5.1 the following:

add_executable(t t.cpp)
target_compile_features(t PRIVATE cxx_std_11)

And I got

CMake Error at CMakeLists.txt:4 (target_compile_features):
  target_compile_features specified unknown feature "cxx_std_11" for target
  "t".

[yann-morin-1998]

@pboettch Arg.. I had a more recent version of cmake lying around, that was
taking precedence. I just retried, and indeed cmake-3.1 errors out too.

Shall I send a revert, or will you do?

(lesson learnt: next time, do not append to PATH, but prefix to it...)

[pboettch]

@yann-morin-1998 If you change it to target_compile_features( ... cxx_range_for) in the CMakeLists.txt of this library, does it it work for you?

[yann-morin-1998]

@pboettch So, replacing cxx_std_11 with cxx_range_for does work with cmake-3.1, yes.

[nlohmann]

Just to nitpick: are there compilers that support range fors but not the rest?

[pboettch]

Maybe some versions before GCC 4.8. IOW versions of compilers which had partial support of some C++11 features. Normally you should already not support these compilers.

AFAIK, CMake cxx_range_for pulls in C++11 support on GCC, Clang and VS.

[yann-morin-1998]

@nlohmann @pboettch Here's what it got me:

[  2%] Building CXX object test/CMakeFiles/catch_main.dir/src/unit.cpp.o
cd /home/ymorin/tmp/json/json/build-me/test && /usr/bin/c++    -I/home/ymorin/tmp/json/json/test/thirdparty/catch    -std=gnu++11 -o CMakeFiles/catch_main.dir/src/unit.cpp.o -c /home/ymorin/tmp/json/json/test/src/unit.cpp

So it is using the gnu++11 suport.

[pboettch]

That's CMake's default for GCC when C++11 is required.

[nlohmann]

Would someone volunteer for a PR on this issue?

[yann-morin-1998]

@nlohmann I'll do something over the WE.

If I understood @pboettch correctly, we "just" have to use cxx_range_for to
get the same behaviour as we previously had with cxx_std_11 (sorry, but
C++ is not my cup of tea...;-))

Again, I'm sorry I broke the thing to start with. :-(

[yann-morin-1998]

@nlohmann In the meantime, until I actually come up with a PR, you should just revert the merge, no?

[nlohmann]

No, do not revert the merge. There is no need to rush here. But it would be great to get back to the earlier CMake prerequisite with the next release.

[nlohmann]

So basically this would work, right?

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()

[yann-morin-1998]

@nlohmann I've just opened #1428 with code even simpler than your proposal.

I'd like @pboettch to review it, though, since that was their idea. I'll ping them
directly from the PR.