Simplify enforcement of required c++ standard in cmake

Summary

Setting the minimal c++ standard in cmake is now done by a cmake feature-requirement cxx_std_17 on the dunecommon library target. This requirement is propagated to all other modules by linking against dunecommon. The cmake options CXX_MAX_STANDARD, CXX_MAX_SUPPORTED_STANDARD and DISABLE_CXX_VERSION_CHECK are removed. The cmake function dune_require_cxx_standard() is now deprecated.

Details

Enforcing a c++-standard is different between compilers, the flags might have different names or are not necessary at all. Sometimes the user needs a higher c++ standard than defined for dune-common, maybe just on a single target. The "cmake-way" of setting the c++-standard requirement is either a target property or a target feature. By using these, the highest required standard is passed to the compiler and there is not mixup with flags and standard requirements.

With this MR, the dunecommon library target gets the compile-feature cxx_std_17 that enforces at least c++-language standard 17 to be supported by the compiler. It might be, though, that not all c++-17 library feature are already implemented by the corresponding standard library. This has to be tested separately using individual feature tests.

The change has an implication on other dune modules and targets. The dunecommon target-properties are only propagated, if a downstream target links against it. The c++-standard is not a global property / global compiler flag anymore.

FAQ

• How do downstream modules/libraries inherit the c++-standard requirement? Simply by linking against the dunecommon target.
• How to enforce in a downstream module a different (higher) c++-standard for a specific target than is set in dune-common? Mixing c++ standards in dependent libraries is undefined behaviour, and thus not recommended. But a call of target_compile_features(<target> <PRIVATE|PUBLIC|INTERFACE> cxx_std_[17|20|...]) on the target with name <target> sets an arbitrary standard requirement for the code. This should be combined with a corresponding c++ standard used for all dune modules.
• How to increase the c++ standard for all dune modules? The cmake variable CMAKE_CXX_STANDARD is used for all targets in a cmake project as default c++ standard. This can only be increased using a cxx_std_XXX feature requirement. Thus, one simply has to set this CMAKE_CXX_STANDARD to the highest standard requested by the dune modules.

TODO

• Fix compiler-feature detection

Closes #217 (closed),#307 (closed) Requires !896 (merged)

mentioned in merge request extensions/dune-alugrid!103 (merged)

The pipeline fails in the ENABLE_PYTHONBINDINGS toolchain. This needs some investigations, but I don't know much about the python stuff in dune.

Great, another piece of code following more the CMake way!

Can you add some documentation and add a changelog entry covering this change?

Having to add the library from dune-common for so many tests, does not look nice. But for modules outside of the core modules I expect that to be expected anyways.

added 1 commit

• dd160a7d - add CHANGELOG entry

Compare with previous version

... maybe just on a single target. The "cmake-way" of setting the c++-standard requirement is either a target property or a target feature ...

Does this mean, that we will allow to change the standard for downstream module targets? If yes I consider this very dangerous because the standard is not consistent. It's e.g. possible, that compiling the library uses C++14 which activates some Dune-fallbacks for standard utilities while the downstream does not which leads to two different implementations of the same feature getting linked. Regardless of standard specific stuff in Dune it's AFAIK not guaranteed that mixing standards is OK.

changed the description

mentioned in merge request !864 (closed)

added 1 commit

• 0b4338a2 - link pybind11 module explicitly against dunecommon

Compare with previous version

changed the description

How to proceed with this MR? In general, I think that enforcing a c++ standard by a cmake compiler feature is the way to go, but we change the current dune behaviour in that we do not activate the maximal available standard, but the fixed requested standard. It should be combined with a way to guarantee that all (feature-) flags set by our cmake modules are fixed and cannot be changed in a downstream module. A Proposal for this is in !864 (closed). But this proposal assumes that we set the c++ standard using a compiler feature. (-> circular dependency, maybe should be combined into one MR)

Another question has to be answered: Is it necessary to fix a c++ standard to a specific version different from cxx_std_17, e.g. for test runs? How to solve this? Is it possible to globally set the CMAKE_CXX_STANDARD variable and how does this interact with the compiler feature? This needs to be investigated.

Another problem raised by @carsten.graeser is: Is the change of a c++ standard a defined behaviour in c++ at all? I.e. is it allowed to compile a library with c++ standard A and include the header and link against this library with c++ standard B? Could it be that the translation units are influenced by the choice of the standard? We probably need ABI compatibility (this was a problem with gcc < 5 and newer versions). Maybe this is already enough.

marked as a Work In Progress

added 1 commit

• b62c656c - add cmake-check for equal c++ standard of targets

Compare with previous version

added 39 commits

• b62c656c...e51f2a7e - 33 commits from branch core:master
• af4ca65f - remove CXX_MAX_STANDARD and CXX_MAX_SUPPORTED_STANDARD and deprecate dune_require_cxx_standard
• ece4dc19 - add CHANGELOG entry
• 61a7f292 - link pybind11 module explicitly against dunecommon
• cf5b9dac - add cmake-check for equal c++ standard of targets
• 79f48683 - add CXX_STANDARD to feature test macros
• 6af35a55 - update the feature checks

Compare with previous version

changed the description

marked the checklist item Fix compiler-feature detection as completed

added 1 commit

• 8d339b75 - update the check_compatible_cxx_standard macro

Compare with previous version

added 2 commits

• aec2827c - correct parametis test
• f2b5c490 - Merge branch 'feature/find_parmetis' into feature/cxx_standard_requirement

Compare with previous version

added 1 commit

• 0b0684c8 - make the c++ standard detection more robust

Compare with previous version

added 1 commit

• ece24346 - make the c++ standard detection more robust

Compare with previous version

added 1 commit

• b51cc68c - fix detection of c++ standard in cxx flags

Compare with previous version

I am not an expert and the mentioning of possible ABI incompatibilities makes me feel really uneasy.

The answer in https://stackoverflow.com/questions/46746878/is-it-safe-to-link-c17-c14-and-c11-objects seems to indicate that there is no reason (for gcc) to force a standard for the whole toolchain as long as the standard (or subset) used is fully supported. Problems start when different compiler versions with incomplete standard implementations are used.

But was there ever a real issue? I am pretty sure that I used mixed modules across compiler vendors, like Dune core modules built with GCC and DuMux with Clang. How hypothetically is this whole discussion? This is also true for your C++ libraries from your distribution. Virtually never you will use the same compiler, compiler version, and C++ standard. Still, most of the time it is working flawlessly.

Mixing compilers is in general dangerous. For example we had issues for a long time with the C++ bindings of MPI, if they were compiled with a different compiler (smetimes even with a different version of g++), because they used different name mangling. Nowadays icc, g++ and clang use the same mangling on linux, but how is this on windows, or mac?

There have been issues (e.g. ABI problems with MPI C++ bindings). That they are rare is part of the problem and means they are hard to detect and tend to eat up man-months. Currently we are seeing problems on clusters that might or might not be due to this: https://github.com/trilinos/Trilinos/issues/8087 https://github.com/OPM/opm-grid/pull/488

The problem is users and developers can hardly figure this out.

Hence the lesser chances of something going wrong, the better.

mentioned in merge request !848 (closed)

mentioned in issue #211 (closed)

Lets summarize the discussion since it became a little bit off focus on the subject:

Status quo:

• we have our own cmake function dune_require_cxx_standard to manually set the c++ standard for all dune modules, by simply testing whether the compiler supports a list of std flags.
• the highest tested standard could be changed by setting the CXX_MAX_STANDARD variable. This can even be done in downstream modules.
• Each dune module could in principle change the c++ standard requirement to higher or lower by simply appending its own flag, using cmake functions like set_property(... CXX_STANDARD ...) or target_compile_feature(... cxx_std_123) or by manually appending compile flags.

Proposal:

• Do not write an own functions for setting the standard, but rely on existing cmake functions
• Remove all the additional variables to influence the standard selection, since there are cmake variables for this
• Major difference: std flags are now propagated by linking against the dune-common library target an is not global property any more
• (optionally) Add a warning in the dune_add_library macro, if the c++ standard is changed in the created library compared to the upstream dune modules.

backward compatibility:

• Most of the dune modules already link against dune-common
• Cmake variables are removed (not used any more), so it doesn't matter whether someone uses them or not. They will get automatically a warning by cmake about an unused flag
• the dune_require_cxx_standard function is marked deprecated and does simply nothing
• There are just additional warning if a downstream module does something that seems to be undefined behaviour.
• dune-common just enforces the minimal required standard. For e.g. the test suite this could (globally) be increased by setting the cmake option CMAKE_CXX_STANDARD to something higher. So, also if an application or downstream module requires a higher c++ standard by default, it has to update its config file. This is compatible with the old dune build system.

The discussion about whether changing the c++ standard between modules, or changing the c++ compiler or any compiler flag seems to be unrelated to this MR, since it was possible before in multiple ways and is not forbidden by this MR either. The only relation is the (optional) warning about this behaviour.

unmarked as a Work In Progress

added 26 commits

• b51cc68c...11ee9e0e - 25 commits from branch core:master
• 9ba1d3bd - remove CXX_MAX_STANDARD and CXX_MAX_SUPPORTED_STANDARD and deprecate dune_require_cxx_standard

Compare with previous version

changed the description

added 5 commits

• 9ba1d3bd...f53e761c - 2 commits from branch core:master
• ead5b933 - remove CXX_MAX_STANDARD and CXX_MAX_SUPPORTED_STANDARD and deprecate dune_require_cxx_standard
• a549ddd8 - use relative path for add_subdirectory in python generator
• 770f118e - Merge branch 'issue/python_generated_cmake_relpath' into feature/cxx_standard_requirement

Compare with previous version

added 1 commit

• 74379141 - remove c++ standard related unnecessary parameters in gitlabci.yml

Compare with previous version

added 2 commits

• 1bf00884 - use generated_dir_rel variable
• 5ed5a1a8 - Merge branch 'issue/python_generated_cmake_relpath' into feature/cxx_standard_requirement

Compare with previous version

added 1 commit

• e83a1e55 - cleanup cmake macros

Compare with previous version

resolved all threads

added 1 commit

• c3c9fadd - Cleanup CheckCXXFeatures and add documentation of dune_check_cxx_source_compiles macro

Compare with previous version

How should we proceed? I follow Simon with his reasoning; other do not. To me, all arguments are share and I don't expect new insights that would change our minds.
Should we have a formal vote? Or should we discuss this in a meeting and decide then?

added 5 commits

• c3c9fadd...5a2735c5 - 4 commits from branch core:master
• c15bbf76 - remove CXX_MAX_STANDARD and CXX_MAX_SUPPORTED_STANDARD and deprecate dune_require_cxx_standard

Compare with previous version

mentioned in merge request !944 (closed)

mentioned in merge request !924 (closed)

added 223 commits

• c15bbf76...d94e923e - 221 commits from branch core:master
• e249d6d7 - remove CXX_MAX_STANDARD and CXX_MAX_SUPPORTED_STANDARD and deprecate dune_require_cxx_standard
• cff67491 - Prefix new cmake functions with dune_

Compare with previous version

resolved all threads

changed title from remove CXX_MAX_STANDARD and CXX_MAX_SUPPORTED_STANDARD and deprecate dune_require{-cxx-}standard to Simplify enforcement of required c++ standard in cmake

added buildsystem label

added 1 commit

• 8cfb3298 - Add missing include of CMakePushCheckState

Compare with previous version

I think this should be merged. As there were concerns from @markus.blatt and @carsten.graeser, are you still concerned? Or can we merge anyway?