tbeu / matio

MATLAB MAT File I/O Library
https://matio.sourceforge.io
BSD 2-Clause "Simplified" License
330 stars 97 forks source link

CMake improvements #151

Closed MaartenBent closed 3 years ago

MaartenBent commented 3 years ago

This fixes using the hdf5-config.cmake provided by the HDF5 package. This didn't work correctly because it checked for an exact version match.

I now automatically select static HDF5 (and zlib/szip) libraries when building static matio. ~Another idea is adding an option MATIO_HDF5_LIBRARY_TYPE with choices default, shared and static. What do you think?~ A user can specify HDF5_USE_STATIC_LIBRARIES themselves to override it.

coveralls commented 3 years ago

Coverage Status

Coverage remained the same at 82.574% when pulling 68d5c24b3bf6e06c35d8455ab5f59aa0b61d1b91 on MaartenBent:cmake-fixes into 1f56cabf096d6d7084a2910cd42ec00a78a4ffc1 on tbeu:master.

tbeu commented 3 years ago

Thanks again. I was constantly failing in configuring the conan recipe to find the proper hdf5 dependency. With these fixes (and my above change setting H5_BUILT_AS_DYNAMIC_LIB as pre-processor define it is finally working, no matter if shared/static matio links shared/static hdf or whatsoever.

tbeu commented 3 years ago

What I observe in VS is that -DH5_BUILT_AS_DYNAMIC_LIB is defined instead of H5_BUILT_AS_DYNAMIC_LIB.

image

MaartenBent commented 3 years ago

Have you tried a clean build? For me it is just H5_BUILT_AS_DYNAMIC_LIB.

tbeu commented 3 years ago

Have you tried a clean build? For me it is just H5_BUILT_AS_DYNAMIC_LIB.

Yes, deleted all and started a fresh build with same result. CMake is 3.18.2 and Conan is 1.28.1.

MaartenBent commented 3 years ago

Maybe it is an issue with Conan. Can you add the following at line 54 in thirdParties.cmake to see what definitions you get from the HDF5 library:

get_target_property(definitions hdf5::hdf5-shared INTERFACE_COMPILE_DEFINITIONS)
message("${definitions}")

For me it prints: H5_BUILT_AS_DYNAMIC_LIB

tbeu commented 3 years ago

It always uses the else branch:

        # results from CMake FindHDF5
        set_target_properties(HDF5::HDF5 PROPERTIES
            INTERFACE_INCLUDE_DIRECTORIES "${HDF5_INCLUDE_DIRS}"
            INTERFACE_LINK_LIBRARIES "${HDF5_LIBRARIES}"
            INTERFACE_COMPILE_DEFINITIONS "${HDF5_DEFINITIONS}"
        )

Maybe it is due to Conan not distributing the CMake settings, but only include files, libs and executables.

MaartenBent commented 3 years ago

Indeed, it does not seem to have the cmake config files. But I could reproduce it and hopefully fix it.

tbeu commented 3 years ago

Indeed, it does not seem to have the cmake config files. But I could reproduce it and hopefully fix it.

Confirmed. It is working now. Thanks a lot!

tbeu commented 3 years ago

Argh, too early.

There is a warning in https://travis-ci.org/github/tbeu/matio/jobs/724596042

CMake Warning:

  Manually-specified variables were not used by the project:

    ZLIB_ROOT

and later when testing

+./tools/matdump -d ./MSL/Modelica/Resources/Data/Tables/test_v7.mat s
Compressed variable found in "./MSL/Modelica/Resources/Data/Tables/test_v7.mat", but matio was built without zlib support
An error occurred in reading the MAT file
Couldn't find variable s in the MAT file

Thus, it does not seem to correctly build with zlib if used from hdf5.