ukoethe / vigra

a generic C++ library for image analysis
http://ukoethe.github.io/vigra/
Other
407 stars 191 forks source link

hdf5 1.12.0 support #476

Closed ArchangeGabriel closed 2 years ago

ArchangeGabriel commented 4 years ago

hdf5 1.12.0 introduces some API changes. This results in build error:

/build/vigra/src/vigra-1.11.1/src/impex/hdf5impex.cxx: In function ‘H5O_type_t vigra::HDF5_get_type(hid_t, const char*)’:
/build/vigra/src/vigra-1.11.1/src/impex/hdf5impex.cxx:193:60: error: too few arguments to function ‘herr_t H5Oget_info_by_name3(hid_t, const char*, H5O_info2_t*, unsigned int, hid_t)’                                                                                        
  193 |     H5Oget_info_by_name(loc_id, name, &infobuf, H5P_DEFAULT);
      |                                                            ^
In file included from /usr/include/H5Apublic.h:22,
                 from /usr/include/hdf5.h:23,
                 from /build/vigra/src/vigra-1.11.1/include/vigra/hdf5impex.hxx:51,
                 from /build/vigra/src/vigra-1.11.1/src/impex/hdf5impex.cxx:38:
/usr/include/H5Opublic.h:188:15: note: declared here
  188 | H5_DLL herr_t H5Oget_info_by_name3(hid_t loc_id, const char *name, H5O_info2_t *oinfo,
      |               ^~~~~~~~~~~~~~~~~~~~
make[2]: *** [src/impex/CMakeFiles/vigraimpex.dir/build.make:174: src/impex/CMakeFiles/vigraimpex.dir/hdf5impex.cxx.o] Error 1

There might be others, see the migrating help.

hmeine commented 4 years ago

What do you suggest?

  1. Should the configuration check that hdf5 version is <1.12?
  2. Should the code be adapted to version 1.12 and the CMakeFiles require that version?
  3. Should the code support older and newer versions and the CMakeFiles create a conditional #define?

Finally, would you be willing to supply a pull request?

ArchangeGabriel commented 4 years ago

If I’d care only about my use case (packaging for Arch Linux), I’d say 2., but 3. might be generally better.

I’d love to do a PR, but I definitively lack the skills for it (I only have very superficial knowledge of C/C++ and never wrote something else than a simple exercise in either of those languages). So while I can see the issue are changes in some H5 functions arguments, I would not trust my ability to fix them correctly.

dvzrv commented 4 years ago

@hmeine I fixed a similar issue for csound, it was relatively trivial: https://github.com/csound/csound/pull/1314

ArchangeGabriel commented 4 years ago

As a temporary measure I got it to build passing -DCMAKE_C_FLAGS="-DH5_USE_110_API" as a cmake arg. ;)

EDIT: Scratch that, I did not build against 1.12 actually, it still fails as above…

EDIT2: Of course, -DCMAKE_CXX_FLAGS="-DH5_USE_110_API" in addition seems to work.

hmaarrfk commented 2 years ago
diff --git a/src/impex/hdf5impex.cxx b/src/impex/hdf5impex.cxx
index 2c68342e..682e0126 100644
--- a/src/impex/hdf5impex.cxx
+++ b/src/impex/hdf5impex.cxx
@@ -190,7 +190,7 @@ H5O_type_t HDF5_get_type(hid_t loc_id, const char* name)
 {
     // get information about object
     H5O_info_t infobuf;
-    H5Oget_info_by_name(loc_id, name, &infobuf, H5P_DEFAULT);
+    H5Oget_info_by_name(loc_id, name, &infobuf, H5O_INFO_BASIC, H5P_DEFAULT);
     return infobuf.type;
 }

might work, the other alternative is to use H5Oget_info_by_name1 instead of H5Oget_info_by_name

https://portal.hdfgroup.org/display/HDF5/Migrating+from+HDF5+1.10+to+HDF5+1.12