Link correctly with zlib when using hdf5 config

[MaartenBent]

HAVE_ZLIB has never set when hdf5-config provides the zlib targets. Also map ZLIB::ZLIB to the correct target.

[coveralls]

Coverage remained the same at 82.574% when pulling 9672d2e on MaartenBent:cmake-zlib into 04a24f2 on tbeu:master.

[tbeu]

Thanks. That seems to fix the zlib linking issue.

Beside this issue I also noticed with the Conan recipe that HDF5 is only correctly found with latest CMake 3.18. It failed to find HDF5 with CMake 3.7 and any later other version I tested up to 3.17. Would it help to distribute the latest FindHDF5.cmake module along with matio or is it an issue with the recipe when configuring the CMake options?

[MaartenBent]

I tried CMake 3.15 to find the HDF5 1.12 libraries (downloaded and built like AppVeyor does) and it works fine. I made sure it did not use hdf5-config.

I know nothing about Conan, but are you supposed to set CMAKE_PREFIX_PATH (or HDF5_ROOT) and ZLIB_ROOT? I would think specifying it as requirement would be enough.

[tbeu]

I get

-- Could NOT find HDF5 (missing:  HDF5_LIBRARIES HDF5_INCLUDE_DIRS)
-- Could NOT find ZLIB (missing:  ZLIB_LIBRARY ZLIB_INCLUDE_DIR) (Required is at least version "1.2.3")

if I set neither CMAKE_PREFIX_PATH nor ZLIB_ROOT.

[tbeu]

I would think specifying it as requirement would be enough.

According to https://docs.conan.io/en/latest/integrations/build_system/cmake/find_packages.html CMAKE_INCLUDE_PATH and CMAKE_LIBRARY_PATH are set when stating the requirements.

[MaartenBent]

I looked at some Conan recipes at https://github.com/conan-io/conan-center-index/tree/master/recipes, and none of them specify this. They do have a CMakeLists.txt wrapper that contains something like:

include(conanbuildinfo.cmake)
conan_basic_setup(TARGETS)

Do you add this?

[tbeu]

Do you add this?

Ah, no. That was one reason, it failed. One other reason was the Conan options handling.

Now, it works with CMake 3.11 (and later), even without defining CMAKE_PREFIX_PATH and ZLIB_ROOT. However, it fails for 3.10 or 3.7 with the error:

matio/1.5.17@demo/testing: Calling build()
-- The C compiler identification is MSVC 19.16.27043.0
-- The CXX compiler identification is MSVC 19.16.27043.0
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.16.27023/bin/Hostx86/x64/cl.exe
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.16.27023/bin/Hostx86/x64/cl.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.16.27023/bin/Hostx86/x64/cl.exe
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.16.27023/bin/Hostx86/x64/cl.exe -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- The CMake version is 3.7.2
-- Conan: called by CMake conan helper
-- Conan: called inside local cache
-- Conan: Adjusting output directories
-- Conan: Using cmake global configuration
-- Conan: Adjusting default RPATHs Conan policies
-- Conan: Adjusting language standard
-- Found HDF5: C:/Users/tbeu/.conan/data/hdf5/1.12.0/_/_/package/ed2afcc1d3a2dce688bc6468ec8dcade598d3497/lib/hdf5.lib (found version "1.12.0")
-- Found ZLIB: C:/Users/tbeu/.conan/data/zlib/1.2.11/_/_/package/970e773c5651dc2560f86200a4ea56c23f568ff9/lib/zlib.lib (found suitable version "1.2.11", minimum required is "1.2.3")
CMake Error at source_subfolder/cmake/thirdParties.cmake:76 (target_include_directories):
  Cannot specify include directories for imported target "HDF5::HDF5".
Call Stack (most recent call first):
  source_subfolder/CMakeLists.txt:27 (include)


CMake Error at source_subfolder/cmake/thirdParties.cmake:77 (target_link_libraries):
  Cannot specify link libraries for target "HDF5::HDF5" which is not built by
  this project.
Call Stack (most recent call first):
  source_subfolder/CMakeLists.txt:27 (include)


-- Configuring incomplete, errors occurred!

[MaartenBent]

Can you try changing it back to the original

    set_target_properties(HDF5::HDF5 PROPERTIES
        INTERFACE_INCLUDE_DIRECTORIES "${HDF5_INCLUDE_DIRS}"
        INTERFACE_LINK_LIBRARIES "${HDF5_LIBRARIES}"
    )

We have the keep target_compile_definitions, or fix the -DH5_BUILT_AS_DYNAMIC_LIB another way.

[tbeu]

--- a/cmake/thirdParties.cmake
+++ b/cmake/thirdParties.cmake
@@ -73,9 +73,10 @@ if(HDF5_FOUND)
         endif()
     else()
         # results from CMake FindHDF5
-        target_include_directories(HDF5::HDF5 INTERFACE "${HDF5_INCLUDE_DIRS}")
-        target_link_libraries(HDF5::HDF5 INTERFACE "${HDF5_LIBRARIES}")
-        target_compile_definitions(HDF5::HDF5 INTERFACE "${HDF5_DEFINITIONS}")
+        set_target_properties(HDF5::HDF5 PROPERTIES
+            INTERFACE_INCLUDE_DIRECTORIES "${HDF5_INCLUDE_DIRS}"
+            INTERFACE_LINK_LIBRARIES "${HDF5_LIBRARIES}"
+        )
     endif()
 endif()

Yes, changing it back, resolved the Find-HDF5-Shared issue. Also, the H5_BUILT_AS_DYNAMIC_LIB was set as expected and the overall build was successful.

For Find-HDF5-Static I get

-- Could NOT find HDF5 (missing:  HDF5_LIBRARIES) (found version "1.12.0")

in CMake 3.7, no matter if above patch applied or not or CMAKE_PREFIX_PATH is set or not.

[tbeu]

This is likely due to another Conan misconfiguration since I thought I already had the static one working. Will only check tomorrow.

[MaartenBent]

You have to keep target_compile_definitions(HDF5::HDF5 INTERFACE "${HDF5_DEFINITIONS}"). Without it I don't get the H5_BUILT_AS_DYNAMIC_LIB definition.

I don't know what is going on with CMake 3.7. I'll try it with the libs from the hdf5 installer later. See if it fails with those as well, or if it is Conan related.

[tbeu]

Hm, I am all in favour to simply require CMake 3.11, where building both with static and shared hdf5 dependency succeeds.

[MaartenBent]

I tried CMake 3.7 with the HDF5 package from their website. Shared build fails because FindHDF5 does not define H5_BUILT_AS_DYNAMIC_LIB. Static build fails because that version of FindHDF5 does not search for static libs, it finds the shared lib instead.

I propose to include the CMake 3.18 version of FindHDF5 in this repo and use that one (if license allows this, bsd-2 vs bsd-3). If I use this version, both static and shared succeed with CMake 3.7.

I updated the readme to also mention calling cmake with -DHDF5_DIR="dir/to/hdf5/cmake", so hdf5-config is used.

There is one other issues which I like to mention but will not address in this PR. HDF5 can also have a szip dependency (their website builds do), and FindHDF5 does not include this library in static builds.

[MaartenBent]

One more thing I noticed when using FindHDF5. You can't just switch between shared and static builds. You need to clean the CMake cache first.

[tbeu]

One more thing I noticed when using FindHDF5. You can't just switch between shared and static builds. You need to clean the CMake cache first.

Sounds like a bug to me. @scivision @sethrj Is this a known issue?

[tbeu]

I propose to include the CMake 3.18 version of FindHDF5 in this repo and use that one (if license allows this, bsd-2 vs bsd-3). If I use this version, both static and shared succeed with CMake 3.7.

Thank you for your constant efforts to get this fixed while having too many degrees of freedom. License would not be problematic at all. But I actually wonder if it is worth the effort to distribute recent FindHDF5.cmake? I have no strong opinion. Ubuntu bionic is with CMake 3.10, thus it might be good to add it, instead of forcing CMake 3.11. On the other side it is a large portion of third-party code we need to maintain (see for example the cache issue you discovered).

[MaartenBent]

It might work since 3.9 or 3.10 if I look at https://github.com/Kitware/CMake/commits/master/Modules/FindHDF5.cmake, but I haven't tested those versions. You confirmed 3.11 works, so I use the built-in version for that and later versions.

I'm in favour of keeping it simple. So maybe just mention in the readme that older CMake versions might not be able to find all HDF5 libraries, and using hdf5-config is highly recommended. There is also no cache issue when using this.

[sethrj]

I didn't understand the question, but the latest (3.19 dev) FindHDF5 correctly includes transitive szip/zlib dependencies for shared/static hdf5, and the script does backport to at cmake 3.11.

[tbeu]

@sethrj @MaartenBent Thanks for the feedback.

I propose

• to keep CMake 3.7 as minimum required version
• to revert the last four commits in a single commit (while keeping the history)
• to recommend at least CMake 3.11 in the Readme files
• to patch CMake 3.7 to 3.11 for the Conan builds to make sure conan-center uses at least 3.11 for the builds.

[scivision]

I vendor (copy) the FindHDF5.cmake from CMake GitLab into my projects and edit slightly. Because even that FindHDF5 doesn't cover all my corner cases.

[scivision]

I have not yet had the privilege of using your project, but I do have some experience with the benefits and tradeoffs of bumping up the minimum CMake version. That might be for a separate Issue so as not to clutter here.

Looking at your Travis-CI it looks like you're wanting to support older versions of Linux, MacOS etc.

[MaartenBent]

Some of those commits are useful, I only reverted 6a6cd56.

Should I also remove the special case of finding a static zlib library, and just rely in the results from hdf5-config / FindZLIB?

[tbeu]

Should I also remove the special case of finding a static zlib library, and just rely in the results from hdf5-config / FindZLIB?

Hm, I trust your experience here since I cannot judge. Whatever makes configuration and code maintenance easier...

[MaartenBent]

I'll remove it then. If this special case is needed, it almost certainly needs something to find the static szip library as well. CMake does not have a FindSZIP, only using hdf5-config will support this case.

[MaartenBent]

This hopefully concludes adding CMake support. Let me know if there is anything else (CMake related) to do.

[scivision]

https://github.com/geospace-code/h5fortran/blob/master/cmake/Modules/FindSZIP.cmake
If it's useful to you

[MaartenBent]

https://github.com/geospace-code/h5fortran/blob/master/cmake/Modules/FindSZIP.cmake
If it's useful to you

Thanks. If using the config files provided by hdf5 proves to be insufficient, this will be useful.

[tbeu]

I now see that it also works with CMake 3.10 (both static/shared hdf5), but fails for 3.9. Can you confirm? If yes, we also could recommend 3.10 (being on bionic). Note, that I always set HDF5_USE_STATIC_LIBRARIES explicitly.

[MaartenBent]

I can't test it today, but I suspect this is the relevant fix Kitware/CMake@6f131f4. It has tag v3.10.0-rc1, which suggests it is in CMake 3.10.
So yes, I suppose you can change it to 3.10.

[tbeu]

SO yes, I suppose you can change it to 3.10.

OK, will do and merge.