Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake: (fix) MPI parallel HDF5 1.14.0 #97

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

jwsblokland
Copy link
Contributor

  • The other day the HDF group has release HDF5 1.14.0. If the MPI parallel version of HDF5 1.14.0 is built using CMake then target MPI::MPI_C is required when using the target ${HDF5_LIBRARIES} in a CMake project like H5Z-ZFP. I have implemented finding the required MPI target in the case if HDF5 is parallel.

@jwsblokland
Copy link
Contributor Author

This pull request solves issue #98.

@markcmiller86 Apparently, the Ubuntu test fails because certain url does not longer exist. I am not sure how and if I can fix it.

- The other day the HDF group has release HDF5 1.14.0. If the
  MPI parallel version of HDF5 1.14.0 is built using CMake then
  target MPI::MPI_C is required when using the target ${HDF5_LIBRARIES}
  in a CMake project like H5Z-ZFP. I have implemented finding
  the required MPI target in the case if HDF5 is parallel.
@jwsblokland
Copy link
Contributor Author

@markcmiller86 I was able to figure it out the Ubuntu problem. Now everything works.

@markcmiller86
Copy link
Member

This pull request solves issue #98.

@markcmiller86 Apparently, the Ubuntu test fails because certain url does not longer exist. I am not sure how and if I can fix it.

I've got that fixed in #96 but haven't merged that yet and don't know if I actually will. You can go grab the fix there though.

@markcmiller86
Copy link
Member

Has this been tested agains older HDF5 versions? I can't recall..what would happen if HDF5_IS_PARALLEL is not defined? I assume it would treat it as a no answer instead of failing CMake but wanted to check for sure.

@jwsblokland
Copy link
Contributor Author

jwsblokland commented Jan 13, 2023

I tested both HDF5 1.12.1 and 1.14.0 and both of them work. Actually, HDF5 version 1.12.1 does not require MPI::MPI_C target. The current implementation always to try to find MPI::MPI_C target for parallel HDF5 regardless of the version number. If desired a more restrictive if-statement based on HDF5 version number can be implemented but this is going to be tricky because I assume that this CMake change of MPI::MPI_C has been backported to older versions of HDF5 which makes the more restrictive if-statement rather complicated. Furthermore, it requires a careful investigation which versions contains this CMake change.

Currently, the CMake build configuration of H5Z-ZFP requires CMake 3.9 as a minimum version. For this minimum version 3.9 the CMake variable HDF5_IS_PARALLEL is defined (see https://cmake.org/cmake/help/v3.9/module/FindHDF5.html). The same holds for MPI::MPI_C (see https://cmake.org/cmake/help/v3.9/module/FindMPI.htm).

Furthermore, I also checked the case if HDF5_IS_PARALLEL is not defined. In that case the variable will be treated as FALSE and CMake will not fail if the variable is undefined.

@markcmiller86 markcmiller86 merged commit 8fad85d into LLNL:master Jan 13, 2023
@brtnfld
Copy link
Collaborator

brtnfld commented Jan 18, 2023

For reference, see HDFGroup/hdf5#2404, HDFGroup/hdf5#2400

@jwsblokland jwsblokland deleted the bugfix/hdf5_1.14.0 branch January 19, 2023 08:41
@jwsblokland
Copy link
Contributor Author

Reading the discussion HDFGroup/hdf5#2404 and HDFGroup/hdf5#2400, mentioned by @brtnfld we may want to specialize the implemented solution by only applying it for HDF5 1.14.0. Of course assuming that the bug only occurs in 1.14.0 and that implemented solution in PR HDFGroup/hdf5#2400 will be part of HDF5 1.14.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants