[cmake][mpi] Use correct guard MPI

changed the description

HAVE_MPI is a DUNEism and should name C preprocessor macros. The HAVE_ variables exist because they make it easier to generate the config.h. Personally, I wouldn't use it inside CMake code. You are right that using MPI_FOUND is discouraged, but back then Markus and myself agreed on sticking to it, as it prevents using people from mistakenly using MPI_CXX_FOUND.
This old decision can be reconsidered. I just see no new argument.

We had a case were it seems like MPI_FOUND is true but MPI_C_FOUND is false. This leads to tests not being guarded, Dune is configured without MPI but the tests are still executed with mpiexec.

Arguably that might be some broken local configuration. On the other hand, we use an outdated and apparently possibly wrong variable MPI_FOUND although in the CMake script we actually explicitly check for MPI_C_FOUND.

Also in newer CMake versions MPI_FOUND has been reintroduced with the behaviour that I mentioned above (from the CMake doc 3.17: MPI_FOUND "Variable indicating that MPI settings for all requested languages have been found. If no components are specified, this is true if MPI settings for all enabled languages were detected.") . That means if MPI_C_FOUND is true but MPI_CXX_FOUND is false, the guarded tests will not be executed although they should be.

Seems inconsistent to me.

A third point: HAVE_MPI is a preprocessor macro and set to ENABLE_MPI by the CMake code. But there is also a CMake variable HAVE_MPI (set if MPI_C_FOUND) which seems to have nothing to do with the preprocessor variable. Maybe that should be removed then to not make it possible to use it in this context?

dune-istl uses both HAVE_MPI and MPI_FOUND as CMAKE_GUARD in https://gitlab.dune-project.org/core/dune-istl/-/blob/master/dune/istl/test/CMakeLists.txt. It at least doesn't seem to be clear to everyone, which one is the better option...

I agree on your last point. Mixing HAVE_MPI and MPI_FOUND inside of cmake is irritating and makes a transition to a new cmake build system more complicated. Multiple variables representing the same condition and both have to be set consistently is error-prone and might lead to hard-to-detect errors.

We have to decide how we want to do the tests and cmake guards for MPI in the future. The options are

Always use MPI_FOUND
Use MPI_FOUND inside of cmake and introduce HAVE_MPI for config.h file generation
Use MPI_C_FOUND inside of cmake. If MPI_FOUND => MPI_C_FOUND.
Switch to MPI_CXX_FOUND. This does not mean the MPI C++ API (removed in MPI-3), but refers to the MPI C API being usable from C++
Instead of any xxx_FOUND variable, test for existence of the mpi target, i.e. TARGET MPI::MPI_C or TARGET MPI::MPI_CXX

mentioned in issue #211 (closed)

After thinking about this again, I would now agree to use HAVE_MPI for CMAKE_GUARDS. It is a cmake variable that is always set whenever MPI is found in whatever way it is found. If we decide in the future to change to the CXX target and get MPI_CXX_FOUND this would also set HAVE_MPI=1 and thus nothing else need to be changed. Thus, we control by this variable when we consider MPI to be found and which variant we require.

I am okay with this, nobody objected. Timo just rebased his commit. Simon, do you mind merging or if I am going to merge?

added 510 commits

7f2e9621...a85c317c - 509 commits from branch master
b918491e - [cmake][mpi] Use correct guard MPI

Compare with previous version

mentioned in commit cafb9235

merged

mentioned in merge request !1054 (merged)

mentioned in merge request dune-grid!561 (merged)

[cmake][mpi] Use correct guard MPI

Merge request reports

Activity