Revert "buildsystem: relax requirement on cmake version"

[yann-morin-1998]

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 introduced
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.

Coming up with a working test is not as trivial as initially thought, so
a better solution will have to be devised in the future. In the mean
time, revert to the previously-working situation.

This reverts commit e8b6b7a.

Reported-by: Patrick Boettcher [email protected]
Signed-off-by: "Yann E. MORIN" [email protected]

[coveralls]

Coverage remained the same at 100.0% when pulling 9763333 on yann-morin-1998:yem/revert-cmake-version-requirement into dffae10 on nlohmann:develop.

[nlohmann]

As I wrote in #1441 (comment):

• We should require the older CMake version, 3.1 or even 3.0.
• In case we detect CMake >=3.8, we can use target_compile_features with cxx_std_11.
• Otherwise, we use the code that set the compile flag explicitly, cf. the "before" state in 73cc508.

Does this make sense?

[iwanders]

@nlohmann , the first two bullet points in your response are exactly what I implemented under #1441, however as @pboettch noted this is quite a big difference, to quote from the comment:

• cxx_range_for makes CMake choose the compiler's C++-default-standard, yet at least C++11.
• cxx_std_11 sets C++11 even if a newer version is supported and default for the compiler.

This means that cxx_std_11 will force any target downstream to be compiled in c++11 only. Which is obviously undesirable.

I think the best option is what @pboettch proposes in bold in this comment. I've commented about this on the original PR thread and would happily create a PR that implements that solution if we are all in agreement that that's the best way forward.

Update; See comment below, it only overwrites the implicitly set version by the compiler.

[iwanders]

I made a test of all of this. And believe we were incorrect in our assumption that cxx_std_11 forces the downgrade. It doesn't do this, but if the target compile features are lacking downstream it appears to be the case.

With gcc 7.3, the default C++ version is 14, which means that if you pass no flags that explicitly specify the version you get C++14. If a transitive dependency sets cxx_std_11, this flag gets added to the compiler flags, basically dropping you down from the compiler's implicit c++14 version to c++11. If you manually specifiy cxx_std_14 on a target that depends on a cxx_std_11 target, it works and you get c++14.

That means that if downstream projects from this rely on the implicit assumption their compiler will use c++14 or c++17, their builds will break, because the c++11 will propagate down from this package. However, if their cmakes are correct and they specify their target_compile_features as they should it will keep working and use the highest required version.

Based on this I do again believe that #1441 is an appropriate fix for this.

[pboettch]

@iwanders are you sure about your tests?

I used cxx_std_11 to force c++11 even on newer compilers with newer defaults. It worked as expected.

And it is transitive to downstream targets if and only if declared as PUBLIC:

target_compile_features(<tgt> PUBLIC cxx_std_11) 

if declared as PRIVATE nothing happens.

[iwanders]

@pboettch , I linked the test in the previous comment. I could have been more clear though in my comment. With cxx_std_11 we propagate setting a C++ version down the dependency chain. This overwrites the compilers' implicitly set c++ version. I'd say this is acceptable as projects downstream should not rely on an implicit version from the compiler, but instead they should set target_compile_features on their own project, and setting cxx_std_14 overwrites a cxx_std_11 from the transitive dependencies. This situation is present in the test I linked and with that line commented out it files, if I uncomment it it compiles without problems on Ubuntu 18.04.

So yes, setting cxx_std_11 may break projects that rely on the compiler's implicitly set c++ version if their compiler defaults to 14 or 17. But it will not force projects downstream to use c++11. Which I think is acceptable and I'd say #1441 is a good fix, alternatively we can just pre-empt this debate and just use cxx_range_for for all cmake versions and be done with it. I'm fine with either, as long as it works with older cmake versions ;)

[pboettch]

OK, I see.

Totally agree now. Downstream projects need to set their modern c++-standard explicitly otherwise they don't know which version they use. If they don't set it, they don't care, thus this library can impose a standard. Great.

Thanks for insisting. Can you re-open your PR. I'll approve it. @nlohmann could merge it, IMHO.

[iwanders]

Downstream projects need to set their modern c++-standard explicitly otherwise they don't know which version they use. If they don't set it, they don't care, thus this library can impose a standard. Great.

Yes, definitely.

Thanks for insisting. Can you re-open your PR. I'll approve it.

No problem :) I've reopened the PR based on this discussion.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[nlohmann]

Fixed with #1441.