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

CMake improvements #151

Merged
merged 4 commits into from
Sep 6, 2020
Merged

CMake improvements #151

merged 4 commits into from
Sep 6, 2020

Conversation

MaartenBent
Copy link
Collaborator

@MaartenBent MaartenBent commented Sep 5, 2020

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
Copy link

coveralls commented Sep 5, 2020

Coverage Status

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

@tbeu
Copy link
Owner

tbeu commented Sep 5, 2020

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
Copy link
Owner

tbeu commented Sep 5, 2020

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

image

@MaartenBent
Copy link
Collaborator Author

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

@tbeu
Copy link
Owner

tbeu commented Sep 5, 2020

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
Copy link
Collaborator Author

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
Copy link
Owner

tbeu commented Sep 5, 2020

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
Copy link
Collaborator Author

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

@tbeu
Copy link
Owner

tbeu commented Sep 5, 2020

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!

The config files require an exact version match. Also use the provided targets directly.
Define H5_BUILT_AS_DYNAMIC_LIB when HDF5 1.8 config file is used.
User can override it by setting HDF5_USE_STATIC_LIBRARIES themselves.

Also add an attempt to find the static zlib library, since FindZLIP does not do this.
Copy link
Owner

@tbeu tbeu left a comment

Choose a reason for hiding this comment

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

Excellent work. Thank you.

@tbeu tbeu merged commit 04a24f2 into tbeu:master Sep 6, 2020
@tbeu
Copy link
Owner

tbeu commented Sep 6, 2020

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.

@MaartenBent MaartenBent deleted the cmake-fixes branch October 27, 2020 22:44
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 participants