Draft: Optional CMake target dependencies

Convert target dependencies to cmake generator expressions. If a downstream project does not find a dependency, the generator expression will generate an empty string. If the dependency is found, the generator expression will fill the corresponding dependencies for that target. This has the effect to make dependencies optional from the point of view of the upstream module. Notice that these generator expressions include the corresponding compiler definitions, thus, removing most of the #define problems we have with #234 (closed).

Note: this is an experiment and is not yet fully tested!

cmake/modules/AddBLASLapackFlags.cmake

@@ -24,26 +24,35 @@ set_package_properties("LAPACK" PROPERTIES
 set(HAVE_BLAS ${BLAS_FOUND})
 set(HAVE_LAPACK ${LAPACK_FOUND})
 
-# register Lapack library as dune package
-if(HAVE_LAPACK)
-  dune_register_package_flags(LIBRARIES "${LAPACK_LIBRARIES}")
-
+unset(LAPACK_COMPILE_OPTIONS_FLAGS)
+  if(HAVE_LAPACK)
+    set(LAPACK_COMPILE_OPTIONS_FLAGS "-DHAVE_LAPACK=1")
+  endif()
   include(CMakePushCheckState)
   cmake_push_check_state()
   set(CMAKE_REQUIRED_LIBRARIES ${LAPACK_LIBRARIES})
   check_function_exists("dsyev_" LAPACK_NEEDS_UNDERLINE)
   cmake_pop_check_state()
-elseif(HAVE_BLAS)
-  dune_register_package_flags(LIBRARIES "${BLAS_LIBRARIES}")
+  if(LAPACK_NEEDS_UNDERLINE)
+  list(APPEND LAPACK_COMPILE_OPTIONS_FLAGS "-DLAPACK_NEEDS_UNDERLINE")
 endif()
 
+# register Lapack library as dune package
+dune_register_package_flags(
+  LIBRARIES           "$<$<BOOL:${LAPACK_FOUND}>:${LAPACK_LIBRARIES}>"
+  COMPILE_OPTIONS     "$<$<BOOL:${LAPACK_FOUND}>:${LAPACK_COMPILE_OPTIONS_FLAGS}>")
+
+dune_register_package_flags(
+  LIBRARIES           "$<$<BOOL:${BLAS_FOUND}>:${BLAS_LIBRARIES}>"
+  COMPILE_DEFINITIONS $<$<BOOL:${BLAS_FOUND}>:HAVE_BLAS=1>)
+
 # add function to link against the BLAS/Lapack library
 function(add_dune_blas_lapack_flags _targets)
   foreach(_target ${_targets})
-    if(LAPACK_FOUND)
-      target_link_libraries(${_target} PUBLIC ${LAPACK_LIBRARIES})
-    elseif(BLAS_FOUND)
-      target_link_libraries(${_target} PUBLIC ${BLAS_LIBRARIES})
-    endif()
-  endforeach(_target)
+    target_link_libraries(${_target}      PUBLIC "$<$<BOOL:${LAPACK_FOUND}>:${LAPACK_LIBRARIES}>")
+    target_compile_options(${_target}     PUBLIC "$<$<BOOL:${LAPACK_FOUND}>:${LAPACK_COMPILE_OPTIONS_FLAGS}>")
+
+    target_link_libraries(${_target}      PUBLIC "$<$<BOOL:${BLAS_FOUND}>:${BLAS_LIBRARIES}>")
+    target_compile_definitions(${_target} PUBLIC $<$<BOOL:${BLAS_FOUND}>:HAVE_BLAS=1>)
+  endforeach()
 endfunction(add_dune_blas_lapack_flags)

[Simon Praetorius]

Since LAPACK_LIBRARIES might be a list of libraries, this generator expression cannot be used. Try the following:

set(LAPACK_LIBRARIES_GENERATORS "")
foreach (lib ${LAPACK_LIBRARIES})
  list(APPEND LAPACK_LIBRARIES_GENERATORS "$<$<BOOL:${LAPACK_FOUND}>:${lib}>")
endforeach(lib)
# ...
dune_register_package_flags(
  LIBRARIES          ${LAPACK_LIBRARIES_GENERATORS}
  COMPILE_OPTIONS     "$<$<BOOL:${LAPACK_FOUND}>:${LAPACK_COMPILE_OPTIONS_FLAGS}>")

Not so nice, though.

[Simon Praetorius]

I think, it might be related to the escape of the ";" symbol, it seems that the string with multiple libraries inside the generator expression is somewhere interpreted as a list with two string:

The global property ALL_PKG_LIBS contains

[...];$<$<BOOL:TRUE>:/usr/lib/x86_64-linux-gnu/libblas.so>;$<$<BOOL:TRUE>:/usr/lib/x86_64-linux-gnu/liblapack.so;/usr/lib/x86_64-linux-gnu/libblas.so>;

So, it is a list of libraries, but the LAPACK library is a generator expression containing a list. The <...> symbols in cmake do not protect from list separation.

One solution could be to store the ALL_PKG_LIBS not as a regular list, but our own "list" implementation with a custom element separator. This will be difficult. In the long run, I would really love to get rid of these global properties.

[Simon Praetorius]

Could be related to https://gitlab.kitware.com/cmake/cmake/-/issues/14353

[Simon Praetorius]

But it is difficult to reproduce in a minimal example.

[Santiago Ospina De Los Ríos]

Yes, I realized about this problem with generators too. I think the solution might be to make an INTERFACE target with all of the dependencies of that library. That way one does not have to deal with lists of flags/definitions/includes. So ideally, ALL_PKG_INCS/ALL_PKG_DEFS/ALL_PKG_OPTS would be empty and ALL_PKG_LIBS would be a list of targets. But I have to experiment a bit with that.

[Simon Praetorius]

Maybe something like a global accessible interface library, e.g., dune-all-packages that is simply filled with all flags, options, and libraries, and then the user can simply link against this target.

[Santiago Ospina De Los Ríos]

I tried this the other day but it does not entirely work because such library needs to be installed (because the main library may depend on it). The problem comes in downstream modules when you also want a library with the same name, and possible the same alias. Then it will conflict with whatever is defined upstream. But maybe there is a way to solve that.

[Simon Praetorius]

how about dune-common-all-packages. The name does probably not solve all the problems. We need something like a forwarding target that does not show up as dependency itself. But I think this would only be possible if it would just define link libraries (via imported targets)

[Simon Praetorius]

we could maybe collect all packages in imported library that contain also the HAVE_CXY flags and then just collect these.

[Simon Praetorius]

Maybe !1207 (merged) can help. I want to get rid of the automatic linking of all packages. This requires the global ALL_PKG_LIBS properties and is the source of the above issue. If we make this more explicit, we don't have this problem anymore.

[Santiago Ospina De Los Ríos]

It would be great if we could get rid of these global states at least for the core modules. All of these experiments try to go on that direction. One concern I have is that I don't want to install targets that are only internal to a library and do not necessarily need to be exported. So there is a compromise to make there.

[Simon Praetorius]

Now I see the problem. When I link dunecommon against my dune-common-all-packages INTERFACE target I get the cmake error export called with target "dunecommon" which requires target "dune-common-all-packages" that is not in any export set.

[Simon Praetorius]

Maybe we just to have to some extra quotation marks somewhere, e.g. in the add_dune_all_flags or dune_target_enable_all_packages functions around the extracted global properties, e.g. link libraries.

[Santiago Ospina De Los Ríos]

Can you push your experiment with dune-common-all-packages? I want to check it out. If it works when installing the target, it could be a good compromise while we get rid of global states/libraries.

[Santiago Ospina De Los Ríos]

NVM I found it.