(0032955)
Brad King (manager)
2013-04-29 10:07

This module seems to be doing a lot of unnecessary work. If HDF5_LIBRARIES simply contained the names of the imported targets then CMake would link them by full path. There is no need to explicitly extract LOCATION property values at all. That's just duplicating logic already implemented properly in CMake itself.

(0032956)
Brad King (manager)
2013-04-29 10:09

You propose this text:

+# If a CMake installed HDF5 is present on the system, it is recommended
+# that you use find_package(HDF5 NO_MODULE) to bypass this module and instead
+# import the hdf5 targets directly.

The module should follow its own advice and be no more than a thin wrapper around the package config file.

(0032958)
John Biddiscombe (developer)
2013-04-29 10:57

>>The module should follow its own advice and be no more than a thin wrapper around the package config file.

>>This module seems to be doing a lot of unnecessary work. If HDF5_LIBRARIES simply contained the names of the imported targets then CMake would link them by full path

Agreed. Unfortunately, projects like paraview (when use system hdf5 is enabled) call FindHDF5 and then add the libraries to projects like netcdf, exodus, xdmf, etc etc and then fail to compile at the top level because of missing lib directories.

I have tried every combination of this, on many projects (I have many that use HDF5) most were written some time ago for HDF5 installations which do not use CMake and they fail to compile and link without the location extraction.

The existing FindHDF5 is broken and tries to link against DLL files. This patch fixes that issue.

[I would rather not use this module at all. It's absolutely terrible, overcomplicated and pointless, simply linking against hdf5, hdf5_hl, etc is the correct way as you point out, but projects designed for the old find method don't work for me]

(0032959)
Brad King (manager)
2013-04-29 11:21

For reference it looks like the location-expansion was added as part of the original NO_MODULE support:

http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=3978f322 [^]

It was probably done that way to support the historical use cases you mention.

(0032960)
Brad King (manager)
2013-04-29 11:24

What is the purpose of the comment block you add:

+ ##do we need both
+ # set(HDF5_C_LIBRARIES hdf5 CACHE PATH "Library")
+ # set(HDF5_CXX_LIBRARIES hdf5_cpp CACHE PATH "Library")
+ # set(HDF5_HL_LIBRARIES hdf5_hl CACHE PATH "Library")
+ # set(HDF5_FORTRAN_LIBRARIES hdf5_fortran CACHE PATH "Library")
+ ##here they are
+ # set(HDF5_C_LIBRARY hdf5 CACHE PATH "Library")
+ # set(HDF5_CXX_LIBRARY hdf5_cpp CACHE PATH "Library")
+ # set(HDF5_HL_LIBRARY hdf5_hl CACHE PATH "Library")
+ # set(HDF5_FORTRAN_LIBRARY hdf5_fortran CACHE PATH "Library")

? If we're computing the values from the target imports then they are not meant for human editing and do not need to be cached. Also the plural list names should never be cached anyway.

(0035686)
Michael Sarahan (reporter)
2014-04-13 21:23

Just a status check here: I think this issue is affecting me with CMake version 2.8.12.2. Any chance John's patch can be worked into the next CMake release?

(0035689)
Brad King (manager)
2014-04-14 09:12

Re 0014111:0035686: It is too late for 3.0 but I've added this to the roadmap for 3.1.

I would appreciate feedback on my question in 0014111:0032960.

(0035776)
Michael Sarahan (reporter)
2014-04-24 22:18

I'm no expert (just simply a user), but what you say in 0014111:0032960 makes sense to me.

(0035788)
Brad King (manager)
2014-04-25 10:00

The FindHDF5 module needs a major overhaul. Using the LOCATION of imported targets provided by HDF5 is not a good solution. As of CMake 3.0, using the LOCATION properties is disallowed by policy CMP0026:

http://www.cmake.org/cmake/help/v3.0/policy/CMP0026.html [^]

If the module wants to help projects work with full library paths it should find them with find_library rather than depending on the imported targets.

Some kind of transition plan is needed to get consuming projects off the library full paths.

Projects can also just do

find_package(HDF5 NO_MODULE)

to skip the FindHDF5 module altogether and find HDF5 directly and use its imported targets.

(0035789)
Brad King (manager)
2014-04-25 10:03

Re 0014111:0035788: For reference, the FindITK module:

http://cmake.org/gitweb?p=cmake.git;a=blob;f=Modules/FindITK.cmake;hb=v3.0.0-rc4 [^]

is now just a thin wrapper around find_package(ITK NO_MODULE).

(0037623)
tom.albrecht (reporter)
2015-01-07 07:27

Is there any hope this will be fixed, soon? We have a lot of trouble in our project with this FindHDF5 module.

The HDF5 project ships their own FindHDF5.cmake file, which is essentially a wrapper around hdf5-config.cmake.

Maybe that would be a good way to start.

(0037625)
Michael Sarahan (reporter)
2015-01-08 00:50

A kitware employee, JC Fillion-Robin, taught me a better way to use hdf5's provided cmake file if available, and fallback to this old approach. See my blog here:

http://t3hmikez0r.com/2014/07/11/cmake-and-hdf5-revisited/ [^]

(0037626)
tom.albrecht (reporter)
2015-01-08 02:21

Thank you for the tip. I actually got as far myself.
It does however not solve my problem completely because I also include VTK. And it uses its own methods for finding the HDF5 libraries, particularly the file
https://github.com/Kitware/VTK/blob/master/CMake/FindHDF5.cmake [^]

That file specifically includes the buggy FindHDF5.cmake file like so:

include(${CMAKE_ROOT}/Modules/FindHDF5.cmake)

So then it really does not help if I use the NO_MODULE option elsewhere. The faulty CMake module has to be repaired or removed.
In addition, I should check if there is already a bug report in the VTK project as well.

(0037629)
Brad King (manager)
2015-01-08 10:58

Re 0014111:0037623: The non-technical part of the problem is that no one is actively maintaining the module:

http://www.cmake.org/Wiki/CMake:Module_Maintainers [^]

(0042271)
Kitware Robot (administrator)
2016-06-10 14:28

Resolving issue as `moved`.

This issue tracker is no longer used. Further discussion of this issue may take place in the current CMake Issues page linked in the banner at the top of this page.

Issue History
Date Modified	Username	Field	Change
2013-04-26 07:59	John Biddiscombe	New Issue
2013-04-26 07:59	John Biddiscombe	Status	new => assigned
2013-04-26 07:59	John Biddiscombe	Assigned To	=> John Biddiscombe
2013-04-26 07:59	John Biddiscombe	File Added: 0001-Fix-some-problems-with-FindHDF5-with-windows-shared-.patch
2013-04-29 10:07	Brad King	Note Added: 0032955
2013-04-29 10:09	Brad King	Note Added: 0032956
2013-04-29 10:57	John Biddiscombe	Note Added: 0032958
2013-04-29 11:21	Brad King	Note Added: 0032959
2013-04-29 11:24	Brad King	Note Added: 0032960
2013-05-17 09:33	Robert Maynard	Target Version	CMake 2.8.11 => CMake 2.8.12
2014-04-13 21:23	Michael Sarahan	Note Added: 0035686
2014-04-14 09:12	Brad King	Note Added: 0035689
2014-04-14 09:12	Brad King	Target Version	CMake 2.8.12 => CMake 3.1
2014-04-24 22:18	Michael Sarahan	Note Added: 0035776
2014-04-25 10:00	Brad King	Note Added: 0035788
2014-04-25 10:03	Brad King	Note Added: 0035789
2014-06-27 12:02	Brad King	Target Version	CMake 3.1 =>
2015-01-07 07:27	tom.albrecht	Note Added: 0037623
2015-01-08 00:50	Michael Sarahan	Note Added: 0037625
2015-01-08 02:21	tom.albrecht	Note Added: 0037626
2015-01-08 10:58	Brad King	Note Added: 0037629
2016-06-10 14:28	Kitware Robot	Note Added: 0042271
2016-06-10 14:28	Kitware Robot	Status	assigned => resolved
2016-06-10 14:28	Kitware Robot	Resolution	open => moved
2016-06-10 14:31	Kitware Robot	Status	resolved => closed

Notes
(0032955) Brad King (manager) 2013-04-29 10:07	This module seems to be doing a lot of unnecessary work. If HDF5_LIBRARIES simply contained the names of the imported targets then CMake would link them by full path. There is no need to explicitly extract LOCATION property values at all. That's just duplicating logic already implemented properly in CMake itself.

(0032956) Brad King (manager) 2013-04-29 10:09	You propose this text: +# If a CMake installed HDF5 is present on the system, it is recommended +# that you use find_package(HDF5 NO_MODULE) to bypass this module and instead +# import the hdf5 targets directly. The module should follow its own advice and be no more than a thin wrapper around the package config file.

(0032958) John Biddiscombe (developer) 2013-04-29 10:57	>>The module should follow its own advice and be no more than a thin wrapper around the package config file. >>This module seems to be doing a lot of unnecessary work. If HDF5_LIBRARIES simply contained the names of the imported targets then CMake would link them by full path Agreed. Unfortunately, projects like paraview (when use system hdf5 is enabled) call FindHDF5 and then add the libraries to projects like netcdf, exodus, xdmf, etc etc and then fail to compile at the top level because of missing lib directories. I have tried every combination of this, on many projects (I have many that use HDF5) most were written some time ago for HDF5 installations which do not use CMake and they fail to compile and link without the location extraction. The existing FindHDF5 is broken and tries to link against DLL files. This patch fixes that issue. [I would rather not use this module at all. It's absolutely terrible, overcomplicated and pointless, simply linking against hdf5, hdf5_hl, etc is the correct way as you point out, but projects designed for the old find method don't work for me]

(0032959) Brad King (manager) 2013-04-29 11:21	For reference it looks like the location-expansion was added as part of the original NO_MODULE support: http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=3978f322 [^] It was probably done that way to support the historical use cases you mention.

(0032960) Brad King (manager) 2013-04-29 11:24	What is the purpose of the comment block you add: + ##do we need both + # set(HDF5_C_LIBRARIES hdf5 CACHE PATH "Library") + # set(HDF5_CXX_LIBRARIES hdf5_cpp CACHE PATH "Library") + # set(HDF5_HL_LIBRARIES hdf5_hl CACHE PATH "Library") + # set(HDF5_FORTRAN_LIBRARIES hdf5_fortran CACHE PATH "Library") + ##here they are + # set(HDF5_C_LIBRARY hdf5 CACHE PATH "Library") + # set(HDF5_CXX_LIBRARY hdf5_cpp CACHE PATH "Library") + # set(HDF5_HL_LIBRARY hdf5_hl CACHE PATH "Library") + # set(HDF5_FORTRAN_LIBRARY hdf5_fortran CACHE PATH "Library") ? If we're computing the values from the target imports then they are not meant for human editing and do not need to be cached. Also the plural list names should never be cached anyway.

(0035686) Michael Sarahan (reporter) 2014-04-13 21:23	Just a status check here: I think this issue is affecting me with CMake version 2.8.12.2. Any chance John's patch can be worked into the next CMake release?

(0035689) Brad King (manager) 2014-04-14 09:12	Re 0014111:0035686: It is too late for 3.0 but I've added this to the roadmap for 3.1. I would appreciate feedback on my question in 0014111:0032960.

(0035776) Michael Sarahan (reporter) 2014-04-24 22:18	I'm no expert (just simply a user), but what you say in 0014111:0032960 makes sense to me.

(0035788) Brad King (manager) 2014-04-25 10:00	The FindHDF5 module needs a major overhaul. Using the LOCATION of imported targets provided by HDF5 is not a good solution. As of CMake 3.0, using the LOCATION properties is disallowed by policy CMP0026: http://www.cmake.org/cmake/help/v3.0/policy/CMP0026.html [^] If the module wants to help projects work with full library paths it should find them with find_library rather than depending on the imported targets. Some kind of transition plan is needed to get consuming projects off the library full paths. Projects can also just do find_package(HDF5 NO_MODULE) to skip the FindHDF5 module altogether and find HDF5 directly and use its imported targets.

(0035789) Brad King (manager) 2014-04-25 10:03	Re 0014111:0035788: For reference, the FindITK module: http://cmake.org/gitweb?p=cmake.git;a=blob;f=Modules/FindITK.cmake;hb=v3.0.0-rc4 [^] is now just a thin wrapper around find_package(ITK NO_MODULE).

(0037623) tom.albrecht (reporter) 2015-01-07 07:27	Is there any hope this will be fixed, soon? We have a lot of trouble in our project with this FindHDF5 module. The HDF5 project ships their own FindHDF5.cmake file, which is essentially a wrapper around hdf5-config.cmake. Maybe that would be a good way to start.

(0037625) Michael Sarahan (reporter) 2015-01-08 00:50	A kitware employee, JC Fillion-Robin, taught me a better way to use hdf5's provided cmake file if available, and fallback to this old approach. See my blog here: http://t3hmikez0r.com/2014/07/11/cmake-and-hdf5-revisited/ [^]

(0037626) tom.albrecht (reporter) 2015-01-08 02:21	Thank you for the tip. I actually got as far myself. It does however not solve my problem completely because I also include VTK. And it uses its own methods for finding the HDF5 libraries, particularly the file https://github.com/Kitware/VTK/blob/master/CMake/FindHDF5.cmake [^] That file specifically includes the buggy FindHDF5.cmake file like so: include(${CMAKE_ROOT}/Modules/FindHDF5.cmake) So then it really does not help if I use the NO_MODULE option elsewhere. The faulty CMake module has to be repaired or removed. In addition, I should check if there is already a bug report in the VTK project as well.

(0037629) Brad King (manager) 2015-01-08 10:58	Re 0014111:0037623: The non-technical part of the problem is that no one is actively maintaining the module: http://www.cmake.org/Wiki/CMake:Module_Maintainers [^]

(0042271) Kitware Robot (administrator) 2016-06-10 14:28	Resolving issue as `moved`. This issue tracker is no longer used. Further discussion of this issue may take place in the current CMake Issues page linked in the banner at the top of this page.

Relationships