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

Optionally Use Static Linking to HDF5 #823

Merged
merged 3 commits into from Sep 12, 2023

Conversation

HunterBelanger
Copy link
Contributor

I use HighFive and HDF5 in an HPC application. Usually, I try to link all of my dependencies statically to avoid problems on clusters. This PR adds a new CMake option, HIGHFIVE_STATIC_HDF5, which will then statically link to HDF5. The new option is used to set HDF5_USE_STATIC_LIBRARIES before calling find_package for HDF5.

@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (c987a51) 84.59% compared to head (9e27c3e) 84.59%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #823   +/-   ##
=======================================
  Coverage   84.59%   84.59%           
=======================================
  Files          68       68           
  Lines        4784     4784           
=======================================
  Hits         4047     4047           
  Misses        737      737           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@1uc
Copy link
Collaborator

1uc commented Sep 12, 2023

Thank you, this makes sense.

@1uc 1uc merged commit d5c756b into BlueBrain:master Sep 12, 2023
35 checks passed
@HunterBelanger HunterBelanger deleted the feature/hdf5_options branch September 24, 2023 01:15
@1uc
Copy link
Collaborator

1uc commented Dec 15, 2023

@HunterBelanger We're reviewing and rewriting our CMake. I'd like to remove the variable HIGHFIVE_STATIC_HDF5. Instead users would use the HDF5 variables.

The idea is that:

set(HDF5_USE_STATIC_LIBRARIES On)
find_package(HighFive REQUIRED)

target_link_libraries(foo PUBLIC HighFive)

should cause HighFive to link to the static HDF5 libraries. Essentially, because it will largely equivalent to:

set(HDF5_USE_STATIC_LIBRARIES On)
find_package(HDF5 REQUIRED)
target_link_libraries(foo PUBLIC HDF5::HDF5)
target_include_directory(foo PUBLIC ${HIGHFIVE_INCLUDE_DIR})

Do you remember if you tried using the HDF5 variables directly and if yes why it didn't work? It's very possible that some of HighFive's CMake code was preventing this from working correctly.

Please let us know if it's obvious to you that using the HDF5 CMake variables has a flaw.

@HunterBelanger
Copy link
Contributor Author

@1uc, you are right, this does seem to work and is likely a better solution ! Feel free to remove this option if needed, and I will be on the lookout so I can modify my CMake files.

@1uc
Copy link
Collaborator

1uc commented Dec 18, 2023

Thank you! If the change happens it'll be in 3.0.0.

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

2 participants