CMake: Find MPI in HDF5 config #2400
Conversation
kwryankrattiger
requested review from
lrknox,
derobins,
byrnHDF,
fortnern,
jhendersonHDF,
qkoziol,
vchoi-hdfgroup,
bmribler and
raylu-hdf
as code owners
|
Hi @kwryankrattiger, I'm wondering, with API routines such as http://portal.hdfgroup.org/display/HDF5/H5P_SET_FAPL_MPIO, won't HDF5 need to keep MPI in the public link interface when built as a parallel-enabled library so that an application wishing to use that API will see MPI as a dependency? |
I was not aware of those, I thought HDF5 had completely abstracted parallel components in its interface. Since that is the case, then all of the previous versions since 1.4 have had a minor configuration bug! The correct solution then is likely to add a call to I pushed another fix for this that I tested with building sz. |
My plan too! |
|
We will need this same change in develop first. |
kwryankrattiger
force-pushed
the
hdf5_link_mpi_private
branch
from
e684bae to
afb5bf7
Compare
kwryankrattiger
changed the title
kwryankrattiger
force-pushed
the
hdf5_link_mpi_private
branch
from
afb5bf7 to
433126d
Compare
|
(unsolicited opinion) In my mind, it should still be linked privately. That way, when the users "link" they will be pointed to the appropriate MPI library, instead of the HDF5 implementation. |
|
@hmaarrfk Since HDF5 is exposing MPI symbols in the interface, publicly linking the MPI_C target is probably the correct thing to do. Since this patch is adding a It is possible to spoof an |
|
Fix for #2404. |
|
Ok, I think i lost track of the original suggestion. Looking through the code, I can only find: which is generally what I've found works when integrating libraries together. Thank you for your time explaining things Best, |
|
Interesting discussion. I did encounter the same problem when building the H5Z-ZFP filter using HDF5 1.14.0. This has been fixed by using find_package(MPI) in the H5Z-ZFP filter package itself. However, this pull request is a much better solution. Now I have one small question to the people of HDF group like @byrnHDF and @lrknox which HDF5 versions contains this CMake configuration mistake? Is it only 1.14.0? Personally, I would like to specialize the solution in H5Z-ZFP filter package such we make us of this solution when it has been released. |
Co-authored-by: kwryankrattiger <[email protected]>
|
@jwsblokland, HDF5 1.14.0 was the first HDF5 release that included an MPI::MPI_C dependency when PARALLEL was enabled, declared PUBLIC in src/CMakeLists.txt. Previous releases only had PRIVATE declarations of the MPI::MPI_C dependency. Pull requests to include the code in this pull request have been submitted for all active hdf5 development branches; all future releases will include this fix. |
|
I didn't see the PUBLIC linkage in But, generally, the C compiler, and dynamic linker, can get confused in terms of which binding to link to at run time. This can lead to surprising results. Generally, you always want to link privately when using shared libraries. Users will have to provide their own library at link to integrate with yours. I think adding the is good too. But you still want to point to the linker back to the original MPI library, not HDF5. |
Co-authored-by: kwryankrattiger <[email protected]>
Co-authored-by: kwryankrattiger <[email protected]>
|
@hmaarrfk Let me know if the following description answers your questions, I think maybe the terms public/private are crossing contexts so I am hoping this will make it more clear what is going on. 🙂
The context of private/public here is only in CMake and what gets added to be linked/included for dependent targets, it doesn't actually change how things are linked. What the public linking is saying here is a target named
And that is what happens. By putting If for some reason a package wanted to ensure that it used exactly the same MPI as the HDF5 build, it could use define it's own MPI::MPI_C and manually set An more concise example of what was causing the error this patch is addressing: find_package(hdf5)
find_package(MPI)
add_executable(my_target main.c)
target_link_libraries(my_target PRIVATE hdf5::hdf5 MPI::MPI_C)This was not working without the In the case of a serial application trying to use a parallel enabled HDF5: find_package(hdf5)
add_executable(my_target PRIVATE hdf5::hdf5)This example would likely fail for older versions of HDF5 as well. The H5public.h and H5pubconf.h is generate at configure time, so
Yeah, this is one of those things that really requires users to understand what they are doing and there isn't really a good way to avoid it other than bundling and setting runpaths (like spack) or making sure the MPIs you use are ABI compatible. But this is a separate issue from the CMake target link visibility. |
|
Truthfully, my skills at inspecting binaries are just not that good. I've tried to remove the lines that include build.sh#/usr/bin/env bash
set -ex
rm -rf build
mkdir build
cd build
PREFIX=${CONDA_PREFIX}
cmake \
${CMAKE_ARGS} \
-DONLY_SHARED_LIBS=ON \
-DBUILD_SHARED_LIBS=ON \
-DCMAKE_INSTALL_PREFIX=${PREFIX} \
-DHDF5_ENABLE_PARALLEL=ON \
..
make -j10Maybe I am missing something but I tried the following patch to the develop branch: diff --git a/config/cmake/hdf5-config.cmake.in b/config/cmake/hdf5-config.cmake.in
index 1a3fb7bbf2..35cee4f606 100644
--- a/config/cmake/hdf5-config.cmake.in
+++ b/config/cmake/hdf5-config.cmake.in
@@ -63,8 +63,6 @@ if (${HDF5_PACKAGE_NAME}_ENABLE_PARALLEL)
set (${HDF5_PACKAGE_NAME}_MPI_Fortran_INCLUDE_PATH "@MPI_Fortran_INCLUDE_DIRS@")
set (${HDF5_PACKAGE_NAME}_MPI_Fortran_LIBRARIES "@MPI_Fortran_LIBRARIES@")
endif ()
-
- find_package(MPI QUIET REQUIRED)
endif ()
if (${HDF5_PACKAGE_NAME}_BUILD_JAVA)
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 355881d72f..b010de458f 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -1245,7 +1245,7 @@ if (NOT ONLY_SHARED_LIBS)
TARGET_C_PROPERTIES (${HDF5_LIB_TARGET} STATIC)
target_link_libraries (${HDF5_LIB_TARGET}
PRIVATE ${LINK_LIBS} ${LINK_COMP_LIBS}
- PUBLIC $<$<NOT:$<PLATFORM_ID:Windows>>:${CMAKE_DL_LIBS}> "$<$<BOOL:${HDF5_ENABLE_PARALLEL}>:MPI::MPI_C>"
+ $<$<NOT:$<PLATFORM_ID:Windows>>:${CMAKE_DL_LIBS}> "$<$<BOOL:${HDF5_ENABLE_PARALLEL}>:MPI::MPI_C>"
)
if (NOT WIN32)
target_link_libraries (${HDF5_LIB_TARGET}
@@ -1289,7 +1289,7 @@ if (BUILD_SHARED_LIBS)
target_link_libraries (${HDF5_LIBSH_TARGET}
PRIVATE ${LINK_LIBS} ${LINK_COMP_LIBS}
$<$<OR:$<BOOL:${HDF5_ENABLE_THREADSAFE}>,$<BOOL:${HDF5_ENABLE_SUBFILING_VFD}>>:Threads::Threads>
- PUBLIC $<$<NOT:$<PLATFORM_ID:Windows>>:${CMAKE_DL_LIBS}> "$<$<BOOL:${HDF5_ENABLE_PARALLEL}>:MPI::MPI_C>"
+ $<$<NOT:$<PLATFORM_ID:Windows>>:${CMAKE_DL_LIBS}> "$<$<BOOL:${HDF5_ENABLE_PARALLEL}>:MPI::MPI_C>"
)
set_global_variable (HDF5_LIBRARIES_TO_EXPORT "${HDF5_LIBRARIES_TO_EXPORT};${HDF5_LIBSH_TARGET}")
H5_SET_LIB_OPTIONS (${HDF5_LIBSH_TARGET} ${HDF5_LIB_NAME} SHARED "LIB")When I build your little project example, somehow, MPI is still linked :/ I'll leave this conversation in the back of my head in case any problems come up. |
Co-authored-by: kwryankrattiger <[email protected]>
Co-authored-by: kwryankrattiger <[email protected]>
Downstream packages depending on HDF5 that don't perform
find_package(MPI)beforefind_package(hdf5)fail to configure. A possible fix to this is to move the MPI link dependency to a private scope.@lrknox Here is a patch that fixes the MPI linking issue observed in the spack CI. I tested this as a patch in spack and it fixes the issue for SZ and likely others.