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

More robust hdf5, along with more refined I/O from cmake #198

Merged
merged 4 commits into from
Dec 14, 2022

Conversation

mauneyc-LANL
Copy link
Collaborator

@mauneyc-LANL mauneyc-LANL commented Nov 30, 2022

PR Summary

Addressing issues @jdolence encountered with HDF5 resolution. Some small rearranging of dependency logic, and a less chatty cmake (by default, can be adjusted)

I had attempted to use module mode to resolve find_package(HDF5) with a distributed FindHDF5.cmake, but I think I made a mess of it and Josh hit an issue that arose from that. The large change here is to revert to a more "standard" approach that relies on the implementation of "official" Kitware CMake module (https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/FindHDF5.cmake).

For reference, it can be informative to expose some trace information on find_package when configuring

cmake --debug-find-pkg=HDF5 .. -DSINGULARITY_USE_HDF5=ON #...

Other notable changes:

  • Output has been reduced in the default log level. A more chatty output can be restored using --log-level=VERBOSE option: cmake --log-level=VERBOSE .. #...
  • some cleanup of singularity_import_* macros

@jonahm-LANL @dholladay00 @jhp-lanl

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file

@mauneyc-LANL mauneyc-LANL self-assigned this Nov 30, 2022
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to update changelog

CMakeLists.txt Show resolved Hide resolved
@Yurlungur Yurlungur requested a review from jdolence November 30, 2022 16:13
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. @jdolence can you double check this resolves the issues you were seeing?

@dholladay00
Copy link
Collaborator

@jdolence what is your timeline on testing this? I'd like to get this in soon.

@dholladay00
Copy link
Collaborator

@mauneyc-LANL can you resolve all incomplete items in the checklist? When completed, I'll merge.

I suspect downstream testing will be necessary.

@Yurlungur
Copy link
Collaborator

Yeah I think @jdolence is not going to test this. Assuming tests pass I'm fine with merging without waiting for him.

@dholladay00 dholladay00 merged commit 8b8acdd into main Dec 14, 2022
@dholladay00 dholladay00 deleted the mauneyc/change/robust_hdf5 branch December 14, 2022 18:45
@@ -285,22 +285,12 @@ if (SINGULARITY_USE_HDF5)
enable_language(C)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes; Indeed, we don't require to do this for spiner, and I'm working on a PR for singularity to follow that approach.

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.

4 participants