Hdf5 1.14.5 #4940

blowekamp · 2024-11-07T21:34:04Z

PR Checklist

No API changes were made (or the changes have been approved)
No major design changes were made (or the changes have been approved)
Added test (or behavior not changed)
Updated API documentation (or API not changed)
Added license to new files (if any)
Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

hjmjohnson

There is a WIP commit that indicates this PR is not yet ready for review, so I made it a draft.

dzenanz

Generally looks good

dzenanz · 2024-11-08T14:45:11Z

Modules/ThirdParty/HDF5/src/CMakeLists.txt

@@ -64,6 +63,23 @@ set(HDF_TEST_EXPRESS OFF CACHE INTERNAL "Control testing framework (0-3)")
 set(BUILD_STATIC_EXECS OFF CACHE INTERNAL "Build Static Executables")
 set(BUILD_USER_DEFINED_LIBS OFF CACHE INTERNAL "Build user defined libs")

+set (HDF5_DIMENSION_SCALES_NEW_REF OFF CACHE INTERNAL "Use new-style references with dimension scale APIs" )
+set (HDF5_EXTERNAL_LIB_PREFIX "itk" CACHE INTERNAL "Use prefix for custom library naming.")


@jcfr this has a potential to interact with Slicer@aacaefd and other Slicer customizations.

Same of them have made their way into official repo, e.g. #4102.

dzenanz · 2024-11-08T14:50:14Z

Modules/ThirdParty/HDF5/src/CMakeLists.txt

+set( HDF5_ENABLE_MAP_API              OFF CACHE INTERNAL "Build the map API")
+set( HDF5_ENABLE_WARNINGS_AS_ERRORS  OFF CACHE INTERNAL "Interpret some warnings as errors")
+#set( HDF5_MINGW_STATIC_GCC_LIBS       OFF CACHE INTERNAL "")
+set( HDF5_MSVC_NAMING_CONVENTION      OFF CACHE INTERNAL "Use MSVC Naming conventions for Shared Libraries")


Does this matter for Windows/MSVC?

dzenanz · 2024-11-08T14:54:59Z

Modules/ThirdParty/HDF5/src/itkhdf5/src/CMakeLists.txt

@@ -1,5 +1,11 @@
 cmake_minimum_required (VERSION 3.12)
 project (HDF5_SRC C)
+# ITK --start
+set(MANGLE_PREFIX itk)


Should we make MANGLE_PREFIX an ITK top-level option?

blowekamp · 2024-11-08T20:01:27Z

The following command can be run to show the local changes in the third party library:
$ git rev-list upstream-HDF5 -n 1 83f14e7ecba1f91b4152f692f54e56306ba3e182 $git diff "upstream-HDF5" HEAD:./Modules/ThirdParty/HDF5/src/itkhdf5/

There are only a couple changes that appear to be needed locally still:
bdc5234 @hjmjohnson Did this get contributed upstream?

blowekamp · 2024-11-11T14:18:16Z

The CI for the builds are passing 🎉

However ghost flow checker as a couple issues:

missing newline at the end of file in src/H5Odtype.c.
- Is there a way to suppress this for this one file, or do we ignore ghost flow here?
hdf.icns with size 326012 bytes (318.37 KiB) which is greater than the maximum size 300000 bytes (292.97 KiB).
- This is just an icon file, not code. It's used with CPack, may not be reachable. Maybe it should be "rm"'d in the UpdateFromUpstream script?
H5Ppublic.h with size 404419 bytes (394.94 KiB)
- So this needs to be added to the .gitattributes file. Does that change need to occur before the commit where it's added?

It's going to be difficult to change the initial sub-tree commit and redo the merge. I want to make sure I have all the necessary changes done in the correct order before going through that process again.

hjmjohnson · 2024-11-11T14:27:17Z

Ensure that #4716 modifications are also included in this PR.

dzenanz · 2024-11-11T15:17:18Z

It is better to make changes to .gitattributes and the update script before before running it. We don't have to fix all the ghostflow warnings.

Code extracted from: https://github.com/HDFGroup/hdf5.git at commit 106fb56936d1d785ca96147601a3608e433ddc3b (hdf5-1_12_3).

* upstream-HDF5: HDF5 2023-11-06 (106fb569) Using "theirs" to resolve conflicts and prefer upstream changes over local modifications.

Remove code modifications in library infavor of setting internal cache variables at top level CMakeLists.txt file. Update to set HDF5 new BUILD_STATIC_LIBS variable.. Update to set COMP_LIBS for ITK's ZLIB. Remove local modified cmake policiy changes integrated into upstream.

Code extracted from: https://github.com/HDFGroup/hdf5.git at commit 0fe0459fc24d71be13d5f266513c2832b525671b (hdf5_1.14.5).

* upstream-HDF5: HDF5 2024-09-30 (0fe0459f)

blowekamp · 2024-11-12T17:05:56Z

Modules/ThirdParty/HDF5/.gitattributes

@@ -2,3 +2,5 @@ src/itkhdf5/src/H5C.c        hooks-max-size=500000
 src/itkhdf5/src/H5Dchunk.c   hooks-max-size=500000
 src/itkhdf5/src/H5Shyper.c   hooks-max-size=600000
 src/itkhdf5/src/H5Tconv.c    hooks-max-size=500000
+src/itkhdf5/src/H5Ppublic.h  hooks-max-size=500000
+src/H5Odtype.c               no-lf-at-eof


This attribute did not work to remove the Ghostflow checker error. The error comes from the sub tree. Likely the attribute should be in the subtree not the main tree. I don't think it's worth the effort to redo the merge again to address this.

Remove unused HDF5 variables. Used the following command to help locate CMake variable no longer used in HDF5: for i in $(grep 'set\s*(' CMakeLists.txt | sed -n 's/set\s*(\s*$HDF5[^ ]*$.*/\1/p'); do echo $i; git grep -c $i; done

blowekamp · 2024-11-13T16:46:26Z

Modules/ThirdParty/HDF5/src/itkhdf5/src/H5Apublic.h

+/* (Must be defined _after_ the function prototype) */
+/* (And must only defined when included in application code, not the library) */
+#ifndef H5A_MODULE
+#define H5Acreate_async(...)         HD5_MANGLE_PREFIX##_H5Acreate_async(__FILE__, __func__, __LINE__, __VA_ARGS__)


@thewtex @dzenanz These definitions like this are problematic for the name mangling. The current redefinitions work, but produce warnings. Each header ( 8-ish) has a "?_MODULE" conditional so that the definitional are applied externally/publicly but not applied when building the library.

My current thought is to add #undef before each of these secondary definitions. But this will be a lot of local modifications. Any other suggestions?

I haven't looked at HDF5 mangling in a long time. Do what you think is best.

If possible, pre-processor updates are added to itk_hdf5_mangle.h.in and this is included first. If this approach can be adapted to apply for this issue (?), this will keep the modifications in one place outside the upstream code, which would ease maintenance.

Unfortunately, you can not redefine a definition. It's rather unfortunate that the "internal" function name and the public CPP define are the same symbol and I can not thing of a way to name mangle it without the code modifications in the latest update.

no longer WIP in commit messages.

blowekamp · 2024-11-14T19:01:40Z

This PR is done as far as I am currently aware now.

thewtex · 2024-11-15T19:17:56Z

Were the cross-platform name mangling symbol checks described in itk_hdf5_mangle.h.in run?

blowekamp · 2024-11-15T19:34:35Z

Were the cross-platform name mangling symbol checks described in itk_hdf5_mangle.h.in run?

I ran the process on OSX arm and linux arm.

thewtex · 2024-11-15T21:40:57Z

We should cover Windows amd64, Linux amd64. There are often new symbols with an update.

hjmjohnson · 2024-11-15T22:12:51Z

@thewtex Do you have a windows box for doing the mangling?

dzenanz · 2024-11-15T23:24:32Z

I have a Windows computer, but I expect to be quite busy on Monday and Tuesday.

blowekamp · 2024-11-16T01:53:48Z

Merging before the final symbols are updated should not cause problems with the CIs or regular builds. The only issue would be for applications used ITK master and are integrating with other untangled HDF5 libraries.

thewtex · 2024-11-17T15:04:50Z

@thewtex Do you have a windows box for doing the mangling?

I am traveling now, but I could take a look next week.

Merging before the final symbols are updated should not cause problems with the CIs or regular builds. The only issue would be for applications used ITK master and are integrating with other untangled HDF5 libraries.

These symbols have caused crashes in Slicer, and there are likely similar applications out there. I would prefer not to merge until all the platforms have been checked.

CC @jcfr

seanm · 2024-11-19T22:17:05Z

Merging before the final symbols are updated should not cause problems with the CIs or regular builds. The only issue would be for applications used ITK master and are integrating with other untangled HDF5 libraries.

Some of my nightly ITK builds on cdash build against VTK too, so they might catch any duplicate symbols.

Thanks for working on this. I took a stab at a smaller update here but got stuck on various cmake stuff I did not understand.

I could clone this PR locally and try building my own app & test suite against it...

seanm · 2024-11-19T23:03:37Z

With ITK_USE_SYSTEM_ZLIB=ON (my typical) it fails to build with:

CMake Error at Modules/ThirdParty/HDF5/src/itkhdf5/src/CMakeLists.txt:1216 (get_target_property):
  get_target_property() called with non-existent target
  "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/lib/libz.tbd".

seanm · 2024-11-20T00:27:01Z

Turning ITK_USE_SYSTEM_ZLIB off, I was able to build my app! My app uses VTK & ITK and I built with ASan too. No link errors. No unit test failures.

So for me, this is in good shape, just needs a fix for ITK_USE_SYSTEM_ZLIB=ON.

dzenanz · 2024-11-20T19:39:47Z

I extracted symbols. I am not sure whether extraction scripts need an update - there are many symbols with extra closed parenthesis. win_hdf5_mangle.zip

Also, build in Debug mode fails - I included the build log. C:\Misc\ITK-patches2\Modules\ThirdParty\HDF5\src\itkhdf5\src\H5make_libsettings.c indeed does not exist.

dzenanz · 2024-11-20T19:49:40Z

Building in RelWithDebInfo mode succeeds, after which building in Debug mode also succeeds.

dzenanz · 2024-11-20T19:52:22Z

win_hdf5_mangle.zip I now also added the (large) intermediate outputs. Brad, can you merge it in?

blowekamp · 2024-11-20T20:07:18Z

I extracted symbols. I am not sure whether extraction scripts need an update - there are many symbols with extra closed parenthesis.

It looks like there are a bunch go C++ symbols ( HD5 namespace ). The scripts may not work with those. I believe we were creating the C++ library before but not mangling the symbols. We should check if ITK is using the C++ libraries.

Also I updated the docs to indicate that the mangling should be disabled before extracting the symbols. The defines already have the "itk_" prefix on them. I think I can remove those.

Build the HDF5 libraries on OSX, and Linux ARM64, in both debug and release mode. Then aggregated all the symbols with the exsiting ones.

The HDF5 library is adding a C-Preprocessor redefinitons pf function to automatically add context based information.

blowekamp requested review from hjmjohnson, jcfr, thewtex and seanm November 8, 2024 09:50

hjmjohnson marked this pull request as draft November 8, 2024 14:17

hjmjohnson previously requested changes Nov 8, 2024

View reviewed changes

blowekamp force-pushed the hdf5_1.12.3 branch from 8b902b8 to e5217e3 Compare November 8, 2024 14:21

blowekamp marked this pull request as ready for review November 8, 2024 14:21

dzenanz approved these changes Nov 8, 2024

View reviewed changes

blowekamp force-pushed the hdf5_1.12.3 branch from e5217e3 to 7cbd8d6 Compare November 8, 2024 19:51

blowekamp requested a review from hjmjohnson November 11, 2024 14:08

hjmjohnson mentioned this pull request Nov 11, 2024

Update HDF from 1.12.1 to 1.12.3 (2nd attempt) #4716

Closed

blowekamp and others added 7 commits November 12, 2024 09:30

ENH: script to update to HDF5 1.12.3

4e209db

HDF5 2023-11-06 (106fb569)

700aa8a

Code extracted from: https://github.com/HDFGroup/hdf5.git at commit 106fb56936d1d785ca96147601a3608e433ddc3b (hdf5-1_12_3).

Merge branch 'upstream-HDF5' into hdf5_1.12.3

cb983bd

* upstream-HDF5: HDF5 2023-11-06 (106fb569) Using "theirs" to resolve conflicts and prefer upstream changes over local modifications.

ENH: Update HDF5 script for 1.14.5

352f581

HDF5 2024-09-30 (0fe0459f)

f76e8a2

Code extracted from: https://github.com/HDFGroup/hdf5.git at commit 0fe0459fc24d71be13d5f266513c2832b525671b (hdf5_1.14.5).

Merge branch 'upstream-HDF5' into hdf5_1.12.3

32700a9

* upstream-HDF5: HDF5 2024-09-30 (0fe0459f)

blowekamp force-pushed the hdf5_1.12.3 branch from 7cbd8d6 to e3940a7 Compare November 12, 2024 16:14

blowekamp changed the title ~~Hdf5 1.12.3~~ Hdf5 1.14.5 Nov 12, 2024

blowekamp commented Nov 12, 2024

View reviewed changes

dzenanz approved these changes Nov 12, 2024

View reviewed changes

ENH: Updates to HDF5 configuration

e175473

Remove unused HDF5 variables. Used the following command to help locate CMake variable no longer used in HDF5: for i in $(grep 'set\s*(' CMakeLists.txt | sed -n 's/set\s*(\s*$HDF5[^ ]*$.*/\1/p'); do echo $i; git grep -c $i; done

blowekamp force-pushed the hdf5_1.12.3 branch 2 times, most recently from 408628d to ac0abad Compare November 13, 2024 16:34

blowekamp commented Nov 13, 2024

View reviewed changes

blowekamp force-pushed the hdf5_1.12.3 branch 3 times, most recently from f48fc47 to 4ee1977 Compare November 14, 2024 14:35

dzenanz approved these changes Nov 14, 2024

View reviewed changes

blowekamp added 3 commits November 20, 2024 15:30

ENH: Update HDF5 mangling

c57e997

Build the HDF5 libraries on OSX, and Linux ARM64, in both debug and release mode. Then aggregated all the symbols with the exsiting ones.

ENH: Mangle HDF5 functions re-defined

422bf2a

The HDF5 library is adding a C-Preprocessor redefinitons pf function to automatically add context based information.

ENH: Support thirdparty hdf5 using system zlib

5fea965

blowekamp force-pushed the hdf5_1.12.3 branch from 5227203 to 5fea965 Compare November 20, 2024 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hdf5 1.14.5 #4940

Hdf5 1.14.5 #4940

blowekamp commented Nov 7, 2024

hjmjohnson left a comment

dzenanz left a comment

dzenanz Nov 8, 2024

dzenanz Nov 8, 2024

dzenanz Nov 8, 2024

dzenanz Nov 8, 2024

blowekamp commented Nov 8, 2024

blowekamp commented Nov 11, 2024

hjmjohnson commented Nov 11, 2024

dzenanz commented Nov 11, 2024

blowekamp Nov 12, 2024

blowekamp Nov 13, 2024

dzenanz Nov 13, 2024

thewtex Nov 13, 2024

blowekamp Nov 13, 2024

blowekamp commented Nov 14, 2024

thewtex commented Nov 15, 2024

blowekamp commented Nov 15, 2024

thewtex commented Nov 15, 2024

hjmjohnson commented Nov 15, 2024

dzenanz commented Nov 15, 2024

blowekamp commented Nov 16, 2024

thewtex commented Nov 17, 2024

seanm commented Nov 19, 2024

seanm commented Nov 19, 2024

seanm commented Nov 20, 2024

dzenanz commented Nov 20, 2024

dzenanz commented Nov 20, 2024

dzenanz commented Nov 20, 2024

blowekamp commented Nov 20, 2024

Hdf5 1.14.5 #4940

Are you sure you want to change the base?

Hdf5 1.14.5 #4940

Conversation

blowekamp commented Nov 7, 2024

PR Checklist

hjmjohnson left a comment

Choose a reason for hiding this comment

dzenanz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blowekamp commented Nov 8, 2024

blowekamp commented Nov 11, 2024

hjmjohnson commented Nov 11, 2024

dzenanz commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blowekamp commented Nov 14, 2024

thewtex commented Nov 15, 2024

blowekamp commented Nov 15, 2024

thewtex commented Nov 15, 2024

hjmjohnson commented Nov 15, 2024

dzenanz commented Nov 15, 2024

blowekamp commented Nov 16, 2024

thewtex commented Nov 17, 2024

seanm commented Nov 19, 2024

seanm commented Nov 19, 2024

seanm commented Nov 20, 2024

dzenanz commented Nov 20, 2024

dzenanz commented Nov 20, 2024

dzenanz commented Nov 20, 2024

blowekamp commented Nov 20, 2024