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

Experiment adding libhdf5 #157

Closed
wants to merge 11 commits into from
Closed

Conversation

willGraham01
Copy link
Collaborator

@willGraham01 willGraham01 commented Nov 2, 2022

Opening this PR so we can have an ongoing discussion and a record of it somewhere.

NOT INTENDED FOR REVIEW OR MERGING ANYTIME SOON

Tagging #181

@willGraham01
Copy link
Collaborator Author

Linux build fails, and this is replicable on my local machine.

Can't make much from the error message - looks like (for some reason) HDF5 is expecting a different version to what's installed? H5check_version() has some documentation - it's meant to be a consistency check for things that are already compiled.

@samcunliffe
Copy link
Member

Linux build fails, and this is replicable on my local machine.

Can't make much from the error message - looks like (for some reason) HDF5 is expecting a different version to what's installed? H5check_version() has some documentation - it's meant to be a consistency check for things that are already compiled.

That's indeed strange. We had it working in a little sandbox toy experiment, right? On your machine too? So it seems our CMakeLists.txt or FindHDF5.cmake is to blame.

@giordano
Copy link
Member

giordano commented Nov 4, 2022

What happened is that CMake correctly found system hdf5 during the configuration stage:

-- Found HDF5: /usr/lib/x86_64-linux-gnu/hdf5/serial/libhdf5.so;/usr/lib/x86_64-linux-gnu/libpthread.so;/usr/lib/x86_64-linux-gnu/libsz.so;/usr/lib/x86_64-linux-gnu/libz.so;/usr/lib/x86_64-linux-gnu/libdl.so;/usr/lib/x86_64-linux-gnu/libm.so (found version "1.10.4") 

but then when compiling the program the linker picks up matlab's own hdf5:

/usr/bin/c++ -Werror -O3 -DNDEBUG CMakeFiles/tdms.dir/src/openandorder.cpp.o CMakeFiles/tdms.dir/src/fields/base.cpp.o CMakeFiles/tdms.dir/src/fields/electric.cpp.o CMakeFiles/tdms.dir/src/fields/magnetic.cpp.o CMakeFiles/tdms.dir/src/fields/split.cpp.o CMakeFiles/tdms.dir/src/fields/td_field_exporter_2d.cpp.o CMakeFiles/tdms.dir/src/argument_parser.cpp.o CMakeFiles/tdms.dir/src/array_init.cpp.o CMakeFiles/tdms.dir/src/arrays.cpp.o CMakeFiles/tdms.dir/src/dimensions.cpp.o CMakeFiles/tdms.dir/src/fdtd_grid_initialiser.cpp.o CMakeFiles/tdms.dir/src/grid_labels.cpp.o CMakeFiles/tdms.dir/src/interface.cpp.o CMakeFiles/tdms.dir/src/interpolate.cpp.o CMakeFiles/tdms.dir/src/iterator.cpp.o CMakeFiles/tdms.dir/src/interpolation_methods.cpp.o CMakeFiles/tdms.dir/src/matrix_collection.cpp.o CMakeFiles/tdms.dir/src/mesh_base.cpp.o CMakeFiles/tdms.dir/src/numerical_derivative.cpp.o CMakeFiles/tdms.dir/src/shapes.cpp.o CMakeFiles/tdms.dir/src/simulation_parameters.cpp.o CMakeFiles/tdms.dir/src/source.cpp.o CMakeFiles/tdms.dir/src/timer.cpp.o CMakeFiles/tdms.dir/src/utils.cpp.o CMakeFiles/tdms.dir/matlabio/matlabio.cpp.o -o tdms   -L/usr/local/MATLAB/R2022b/bin/glnxa64  -Wl,-rpath,/home/runner/work/TDMS/build:/usr/local/MATLAB/R2022b/bin/glnxa64: libtdms_lib.so /usr/lib/x86_64-linux-gnu/libfftw3.so -lmex -lmx -lmat /usr/lib/gcc/x86_64-linux-gnu/9/libgomp.so /usr/lib/x86_64-linux-gnu/libpthread.so _deps/spdlog-build/libspdlog.a -lpthread 
/usr/bin/ld: CMakeFiles/tdms.dir/src/openandorder.cpp.o: undefined reference to symbol 'H5check_version@@MWHDF51.8.12'
/usr/bin/ld: /usr/local/MATLAB/R2022b/bin/glnxa64/libhdf5-1.8.so.8: error adding symbols: DSO missing from command line

There are two problems:

  • since you're using the C++ API, you need to require the C++ component:
    find_package(HDF5 REQUIRED COMPONENTS CXX)
  • the target tdms isn't told to explicitly link to the HDF5 library found during configuration, so the linker just picks up the first one it finds. Adding
    target_link_libraries(tdms ${HDF5_CXX_LIBRARIES})
    should do the trick

With these two changes I'm able to build tdms correctly on this branch.

@samcunliffe samcunliffe added enhancement New feature or request technical Technical and meta issues, not related to physics but infrastructure. labels Nov 14, 2022
Probably the MacOS build needs that.
Move the target_link_libraries command to targets.cmake (where all of
the others are). And explicitly link to our version of HDF5 when
building the tests and tdms_lib.
@@ -44,6 +45,7 @@ function(test_target)
${Matlab_MX_LIBRARY}
${Matlab_MAT_LIBRARY}
${LIBCXX_LIBRARY}
${HDF5_CXX_LIBRARIES}
Copy link
Member

Choose a reason for hiding this comment

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

  • the target tdms isn't told to explicitly link to the HDF5 library found during configuration, so the linker just picks up the first one it finds. Adding

      target_link_libraries(tdms ${HDF5_CXX_LIBRARIES})
    

    should do the trick

Mosè's fix is also needed for the dynlib target (at least on my ARM64 Mac which tries to link the wrong HDF5 libraries).

@samcunliffe
Copy link
Member

samcunliffe commented Nov 14, 2022

I am starting to actually develop (for #181) over on 181-hdf5-io.
And, in parallel, get HDF5 working in Windows on #188.

Let's keep this PR open for now.

@samcunliffe
Copy link
Member

Also closing this (c.f. #193) since it can just as happily live in the list of closed issues.
I'll need to revisit #193 (or something similar) given #215.

@willGraham01 willGraham01 deleted the experiment-adding-libhdf5 branch March 31, 2023 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request technical Technical and meta issues, not related to physics but infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants