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

Update HDF5 detection #4708

Merged
merged 3 commits into from
Aug 24, 2023
Merged

Update HDF5 detection #4708

merged 3 commits into from
Aug 24, 2023

Conversation

prckent
Copy link
Contributor

@prckent prckent commented Aug 24, 2023

Proposed changes

Fix reported HDF5 detection problems. Closes #4705 .

Remove the version check from the find_package line, and enforce minimum version ourselves afterwards. HDF5 detection without the "minimum compatible version" of 1.10.0 is how we checked in QMCPACK v3.15.0, which is reported to still work reliably.

The fix solves the problem of HDF5 sometimes not being found because depending on how HDF5 was installed and is found (module vs config mode in find_package), the version check and compatibility analysis done by CMake work differently. This is described in the CMake docs but is subtle. Simplest solution is the one here; we only need to protect against 1.8.x, and even then this is old enough that I doubt it is very relevant today. 1.10.x, 1.12.x, 1.14.1 all appear to work with QMCPACK in my testing. I also verified that the FreeBSD 13.2 system HDF5 package now works.

What type(s) of changes does this code introduce?

  • Bugfix

Does this introduce a breaking change?

  • No

What systems has this change been tested on?

FreeBSD13.2, RHEL9, WSL2/Ubuntu with varous autotools, spack and cmake installs.

Checklist

  • Yes. This PR is up to date with current the current state of 'develop'

@prckent prckent marked this pull request as draft August 24, 2023 01:57
@prckent
Copy link
Contributor Author

prckent commented Aug 24, 2023

Test this please

@prckent prckent changed the title [WIP] Update HDF5 detection Update HDF5 detection Aug 24, 2023
@prckent prckent marked this pull request as ready for review August 24, 2023 13:40
@markdewing
Copy link
Contributor

It would be helpful to add a comment in the cmake file, for future readers wondering why the version is checked manually vs. using the built-in version check on find_package.

@prckent
Copy link
Contributor Author

prckent commented Aug 24, 2023

Added, good point Mark.

Hopefully we won't have to revisit this, since the question of whether different versions of HDF5 are compatible with each other does not have a simple answer.

@markdewing
Copy link
Contributor

Test this please

@quantumsteve
Copy link
Contributor

I think this also fixes #4447

@prckent
Copy link
Contributor Author

prckent commented Aug 24, 2023

@quantumsteve Note that when I checked 1.14.x it was for a serial build. Can you verify the MPI case?

@quantumsteve
Copy link
Contributor

@quantumsteve Note that when I checked 1.14.x it was for a serial build. Can you verify the MPI case?

Just built with MPI and 1.14.2. CMake configuration was successful and All deterministic tests pass.

@ye-luo ye-luo merged commit d8a8227 into QMCPACK:develop Aug 24, 2023
21 checks passed
@prckent
Copy link
Contributor Author

prckent commented Aug 24, 2023

Thanks Steve

@prckent prckent deleted the fixhdfdetection branch August 24, 2023 16:55
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.

[3.17.0] Fails to find hdf5: HDF5 not found. Set HDF5_ROOT
4 participants