Skip to content

Conversation

cbalint13
Copy link
Contributor

@cbalint13
Copy link
Contributor Author

@mshabunin , @vpisarev , @alalek

Could please help me with the buildbots, seems that on several hdf5-lib is missing:
-- Could NOT find HDF5 (missing: HDF5_LIBRARIES HDF5_INCLUDE_DIRS)

  • Only the Linux buildbot have it, so e.g. I cannot proof this PR is valid.
  • Would be nice to be available for VS2013, VS2014, Mac (perhaps android too ?!)

Thanks !

@alalek
Copy link
Member

alalek commented Feb 12, 2016

Almost all 3rdparty dependencies are optional:

  1. There is not easy to maintain them all, especially patched versions.
  2. We can't check all possible configurations (in your case, with and without hdf5 libraries), so we check "default" configuration at least.
  3. Windows environment is not user friendly to manage 3rdparty dependencies. But I will try to find some simple guide how to install HDF5 on Windows.
  4. Missed dependencies should not break existed builds (all is fine here). Users should be able to build OpenCV without installing "unused" 3rdparty software.

@alalek
Copy link
Member

alalek commented Feb 12, 2016

  1. I have installed HDF5 on Win10 slave (MSVS 2015 / Win64 only)
  2. Added valid HDF5_DIR environment variable.

But this doesn't work due some unexpected failures.

@cbalint13
Copy link
Contributor Author

@alalek ,

For 1,2,3,4 understood the points, there is no problem. However this PR still can be merged for MSVC users.

New issue, would require your help:

It appears that on VS2015 CMake's internal FindHDF.cmake module see the header files (H5version.h) since its able to detect the version but not the library itself (probably cannot find the path).

-- Could NOT find HDF5 (missing:  HDF5_LIBRARIES) (found version "1.8.16")
CMake Warning (dev) at C:/utils/cmake-3.4.1/share/cmake-3.4/Modules/FindHDF5.cmake:234 (get_target_property):
  Policy CMP0045 is not set: Error on non-existent target in
  get_target_property.  Run "cmake --help-policy CMP0045" for policy details.
  Use the cmake_policy command to set the policy and suppress this warning.

  get_target_property() called with non-existent target "hdf5".
Call Stack (most recent call first):
  C:/builds/precommit-contrib_windows_ten/opencv_contrib/modules/hdf/CMakeLists.txt:3 (find_package)
This warning is for project developers.  Use -Wno-dev to suppress it.
-- Checking SFM deps... FALSE

@alalek
Copy link
Member

alalek commented Feb 12, 2016

@cbalint13 I installed HDF5 on Windows slave as requested, errors(warnings) are coming from this change.

@alalek
Copy link
Member

alalek commented Feb 12, 2016

@cbalint13 Could you replace find_package(HDF5) statement to this?

find_package (HDF5 NAMES hdf5 COMPONENTS C static)

@cbalint13
Copy link
Contributor Author

@alalek ,

  • I did not saw that you added HDF5 to VS2015 build. As described by me (upper) I think cmake cannot see the very library at all, but sees the header files. In my experience HDF5_LIBRARIES is needed to be declared too manually, not only the generic HDF_DIR.

@cbalint13
Copy link
Contributor Author

@aalek,

  • I replace this now: find_package (HDF5 NAMES hdf5 COMPONENTS C static)

Lets see.

@cbalint13
Copy link
Contributor Author

@alalek

  • find_package (HDF5 NAMES hdf5 COMPONENTS C static) fails on local:
CMake Warning at opencv_contrib/modules/hdf/CMakeLists.txt:3 (find_package):
  Could not find a package configuration file provided by "HDF5" with any of
  the following names:

    hdf5Config.cmake
    hdf5-config.cmake

  Add the installation prefix of "HDF5" to CMAKE_PREFIX_PATH or set
  "HDF5_DIR" to a directory containing one of the above files.  If "HDF5"
  provides a separate development package or SDK, be sure it has been
  installed.

There are problems with such statement:

  • 1 CMake internal is called /usr/share/cmake/Modules/FindHDF5.cmake not hdf5Config.cmake.
  • 2 Forcing static would break linux distros where hdf comes in dynamic form.

This looks fine: find_package(HDF5 COMPONENTS C)
I believe buildbot can be curred only if HDF5_LIBRARIES is declared manually in case of MSVC build.

@alalek
Copy link
Member

alalek commented Feb 13, 2016

@cbalint13 Please note: I follow an official guide of HDF5 library and use HDF5_DIR:
https://www.hdfgroup.org/ftp/HDF5/current/src/unpacked/release_docs/USING_HDF5_CMake.txt

It is not easy to pass HDF5_LIBRARIES on slave via setting environment variable: http://pullrequest.opencv.org/buildbot/builders/precommit-contrib_windows_ten/builds/319/steps/cmake/logs/stdio

HDF5_LIBRARIES=C:/utils/HDF5/1.8.16/lib
...
Could NOT find HDF5 (missing:  HDF5_LIBRARIES HDF5_INCLUDE_DIRS)

Also it can't be passed via cmake command line due some problems (build commands are shared between slaves (we have several Windows slaves), so absolute paths are smell there). Also there is our policy of testing "default" setup, so commands should be short as possible.

You can try to play with it (insert before find package):

ocv_check_environment_variables(HDF5_LIBRARIES HDF5_INCLUDE_DIRS)

Also this approach works on Linux:

find_package (HDF5 QUIET NAMES hdf5 COMPONENTS C static)
if(NOT HDF5_FOUND)
  find_package(HDF5 QUIET)
endif()

Anyway I believe this switch helps in all other cases:

if (WIN32)
  ...
else()
  ...
endif()

@cbalint13
Copy link
Contributor Author

@alalek ,

  • First, many thanks for the time spent helping me !
  • I'll try tackle more as suggested, plus few my ideas, but will end up with IF WIN32 things if I fail.
  • Will look into a real MSVC machine too to better understand the failure.

Be back,
~cristian.

@cbalint13
Copy link
Contributor Author

@alalek ,

  • Can put back the package to the path C:/utils/HDF5/1.8.16 ?
  • Can send me in private the full dir layout of ls -lR C:/utils/HDF5/1.8.16/* ?

@alalek
Copy link
Member

alalek commented Feb 15, 2016

Done: hdf5_setup.txt

@cbalint13
Copy link
Contributor Author

@alalek ,

  • Can remove enviroment "HDF5_DIR=C:/utils/HDF5/1.8.16/cmake" ?
  • Will add myself localy during tests, thus not afect other builds.

@cbalint13
Copy link
Contributor Author

@alalek ,

Lets have HDF5_DIR in final, don't remove:

  • Here we go: cbalint13@b86bdb5, i just mimic official HDF5_DIR way on MSVC.
  • I hope will be acceptable to declare cmake's internal HDF5 routine broken on MSVC.
  • Seems that now have in buildbot 'vars' all the proper paths:
HDF5_INCLUDE_DIRS=C:/utils/HDF5/1.8.16/include
HDF5_LIBRARY=C:/utils/HDF5/1.8.16/lib/hdf5.lib

I'll wait until it builds fine.
On any other issues will fix (i expect none but it may require szip.lib dependency).

@cbalint13
Copy link
Contributor Author

@alalek ,

By hereby patch I propose a simple fix for MSVC land. Few thoughts over HDF5 & CMake especially on MSVC:

  • CMake internal FindHDF5.cmake is clearly broken, it even don't handle HDF5_DIR at all, i checked it source.
  • In contrast CMake internal FindHDF5.cmake may work better, but booth in fact are looking over env${PATH} as HINT to find folders, but not fully working if one tries it out as documentation states. The idea is to install their .msi installer and just to work out of the box via installed system PATH. Doesn't work either (only partially), at least for several latest hdf5 binary releases.
  • Original FindHDF5.cmake from hdf5 tarball (might be better then CMake's) package seems ever changing thus would be hard to maintain, not counting that I believe is over-sophisticated.

All three statements are valid even for older hdf5 binary release as all my tests concluded. My final conclusion is to handle with separate simple & maintainable routines via env{HDF5_DIRS} just like actual patch does. Or perhaps even accounting the PATH environment that is always settled by .msi installers. Actual patch can be extended with ease to handle more HINTS if will be necessary at any time.

Let me know if would like to add to HINT the +env{PATH} too (a bit regexped and filtered). Perhaps it is a good idea to have booth HDF5_DIR and PATH lookup.

@cbalint13 cbalint13 changed the title HDF: Fix MSVC build. (Issue #546). HDF: Fix MSVC build. (Issues #546, #497, #552). Feb 17, 2016
@alalek
Copy link
Member

alalek commented Feb 20, 2016

👍

@opencv-pushbot opencv-pushbot merged commit 133f9c8 into opencv:master Feb 20, 2016
opencv-pushbot pushed a commit that referenced this pull request Feb 20, 2016
@cbalint13 cbalint13 deleted the hdf branch March 12, 2016 04:07
mleotta added a commit to mleotta/fletch that referenced this pull request Jun 20, 2017
Back ported upstream OpenCV changes to fix build errors
in Visual Studio relating to the use of HDF5.
See upstream changes here:

opencv/opencv_contrib#547
allnes pushed a commit to allnes/opencv_contrib that referenced this pull request Jun 9, 2024
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