Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build HDF5 1.14.0 for mingw-w64 #15033

Merged
merged 3 commits into from
Jan 23, 2023

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Jan 13, 2023

This adapts the mingw-w64-hdf5 script for version 1.14.0 of the HDF5 C library.

Note that a 1.12.3 release is also expected in the near future.

Patch hdf5-fix-pkgconfig.patch was already applied upstream.

@mkitti
Copy link
Contributor Author

mkitti commented Jan 13, 2023

Sure. How does that work? Do we still need to copy the mod files?

@mkitti
Copy link
Contributor Author

mkitti commented Jan 13, 2023

Does using that option replace what we are doing with this copy command?

    cp ${srcdir}/build-${MSYSTEM}/mod/static/*.mod ${srcdir}/build-${MSYSTEM}/bin/static/Release

@mmuetzel
Copy link
Collaborator

mmuetzel commented Jan 13, 2023

ArchLinux seems to install Fortran .mod files directly in the include folder. E.g.: https://archlinux.org/packages/community/x86_64/netcdf-fortran/files/
or: https://archlinux.org/packages/community/x86_64/hdf5/files

Should we do the same? I.e., -DHDF5_INSTALL_MODULE_DIR=include?

Does using that option replace what we are doing with this copy command?

    cp ${srcdir}/build-${MSYSTEM}/mod/static/*.mod ${srcdir}/build-${MSYSTEM}/bin/static/Release

Yes, these copy commands would no longer be needed, I'd guess.

@mkitti
Copy link
Contributor Author

mkitti commented Jan 13, 2023

I think we should probably still deploy the mod files to ${srcdir}/build-${MSYSTEM}/bin/static/Release here purely based on precedent.

@mkitti
Copy link
Contributor Author

mkitti commented Jan 13, 2023

Do we have some other precedent in MSYS2/MINGW where Fortran users would expect .mod files to be in the include folder?

@mmuetzel
Copy link
Collaborator

mmuetzel commented Jan 13, 2023

Do we have some other precedent in MSYS2/MINGW where Fortran users would expect .mod files to be in the include folder?

E.g., flang installs the .mod files in include/flang:
https://packages.msys2.org/package/mingw-w64-clang-x86_64-flang?repo=clang64

IIUC, it is up to the project if they prefer installing their headers directly in include or in include/${name}. I'd guess the same can be applied for Fortran modules.

@mmuetzel
Copy link
Collaborator

I think we should probably still deploy the mod files to ${srcdir}/build-${MSYSTEM}/bin/static/Release here purely based on precedent.

Are there actually any .mod files that are copied by that command? Does it fail to install the .mod files if those commands are removed?
Maybe, that copy command was only needed in older versions?

@mmuetzel
Copy link
Collaborator

The proposed change is not about those files copied to bin directory, It's about the files above.
They were installed under include now they are installed in mod.

Correct. That would be fixed by setting -DHDF5_INSTALL_MODULE_DIR=include.

Continuing, @mkitti started to look at other parts of the build rules. Those might be worth checking now, too, imho.

@mkitti
Copy link
Contributor Author

mkitti commented Jan 13, 2023

I used the new option and cleaned up the patch files.

@@ -125,6 +130,8 @@
--- a/config/cmake/HDFMacros.cmake 2023-01-13 04:34:01.443034400 -0500
+++ b/config/cmake/HDFMacros.cmake 2023-01-13 04:36:14.419475400 -0500
@@ -126,10 +126,12 @@
OUTPUT_NAME_RELEASE ${LIB_RELEASE_NAME}
OUTPUT_NAME_MINSIZEREL ${LIB_RELEASE_NAME}
OUTPUT_NAME_RELWITHDEBINFO ${LIB_RELEASE_NAME}
+ RUNTIME_OUTPUT_NAME ${LIB_RELEASE_NAME}-0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two lines can probably be removed: RUNTIME_OUTPUT_NAME can be replaced w/ -DCMAKE_SHARED_LIBRARY_NAME_WITH_VERSION=ON in PKGBUILD, and default ARCHIVE_OUTPUT_NAME should be correct already?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so basically remove the patch and just add the option in PKGBUILD?

Copy link
Contributor

@kmilos kmilos Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a bit weird... So either we bite the bullet, use the same SOVERSION as other platforms and rebuild, or leave things as is...

Edit: Perhaps it makes sense to leave as is for now, and actually try to address that unnecessary WIN32/MINGW/CYGWIN complexity/misunderstandings upstream first.

Copy link
Contributor

@kmilos kmilos Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, the SOVERSION did change between 1.12.2 and 1.14.0, so keeping this artificially at -0 makes no sense at all. A rebuild is needed anyway.

https://github.com/HDFGroup/hdf5/blob/3e847e003632bdd5fdc189ccbffe25ad2661e16f/config/lt_vers.am#L19-L21

https://github.com/HDFGroup/hdf5/blob/0553fb7ac7f03a919c91cdbbd5648b8aadbe05af/config/lt_vers.am#L18-L20

Or don't bump to 1.14 now and wait for 1.12.3 instead...

Copy link
Contributor Author

@mkitti mkitti Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make both 1.14.0 and 1.12.3 available? Are they mutually exclusive?

1.12.3 is the end of the road for 1.12.x by the way, so I think we would want to move to 1.14 immediately.

image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, the SOVERSION did change between 1.12.2 and 1.14.0, so keeping this artificially at -0 makes no sense at all. A rebuild is needed anyway.

The package grokker only pointed out that mingw-w64-python-h5py would need to be rebuilt. (Maybe, that is the only package that uses a function for which the SOVERSION was bumped upstream?)

@mkitti
Copy link
Contributor Author

mkitti commented Jan 13, 2023

I think we may be trying to do too much here. Could some of this be deferred to a pkgrel=2?

mingw-w64-hdf5/PKGBUILD Outdated Show resolved Hide resolved
@@ -125,6 +130,8 @@
--- a/config/cmake/HDFMacros.cmake 2023-01-13 04:34:01.443034400 -0500
+++ b/config/cmake/HDFMacros.cmake 2023-01-13 04:36:14.419475400 -0500
@@ -126,10 +126,12 @@
OUTPUT_NAME_RELEASE ${LIB_RELEASE_NAME}
OUTPUT_NAME_MINSIZEREL ${LIB_RELEASE_NAME}
OUTPUT_NAME_RELWITHDEBINFO ${LIB_RELEASE_NAME}
+ RUNTIME_OUTPUT_NAME ${LIB_RELEASE_NAME}-0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, the SOVERSION did change between 1.12.2 and 1.14.0, so keeping this artificially at -0 makes no sense at all. A rebuild is needed anyway.

The package grokker only pointed out that mingw-w64-python-h5py would need to be rebuilt. (Maybe, that is the only package that uses a function for which the SOVERSION was bumped upstream?)

@mmuetzel
Copy link
Collaborator

According to the package grokker results, you'll still need to bump the pkgrel of mingw-w64-python-h5py.

@mkitti
Copy link
Contributor Author

mkitti commented Jan 14, 2023

According to the package grokker results, you'll still need to bump the pkgrel of mingw-w64-python-h5py.

Sorry, I misunderstood. I've bumped it in 0907e25

@kmilos
Copy link
Contributor

kmilos commented Jan 15, 2023

Please take this opportunity to remove the nonsense -0 versioning.

@mmuetzel
Copy link
Collaborator

Please take this opportunity to remove the nonsense -0 versioning.

I agree that this could probably be removed (unless there are some dependent packages that need the library to have a SOVERSION). But I also agree with @mkitti that it might be better to defer this to a subsequent PR because it would require to rebuild all of the reverse dependencies.

@mkitti
Copy link
Contributor Author

mkitti commented Jan 15, 2023

They are asking about the so version upstream as well:
https://forum.hdfgroup.org/t/msys2-mingw-w64-hdf5-1-14-0-build-scripts-send-patches-upstream/10693/2?u=kittisopikulm

I'll try dropping the -0 later today. If it gets complicated, then maybe we defer?

@mkitti
Copy link
Contributor Author

mkitti commented Jan 15, 2023

We're going to need a new h5py release with h5py/h5py#2194 for hdf5 1.14.0 compatibility

@mkitti
Copy link
Contributor Author

mkitti commented Jan 15, 2023

@mkitti
Copy link
Contributor Author

mkitti commented Jan 15, 2023

Maybe we should bump to libhdf5-1.dll?

@mkitti
Copy link
Contributor Author

mkitti commented Jan 15, 2023

Here's the PR that added the search for -0 in upstream h5py. It was for mingw compat:
h5py/h5py#2105

cygwin uses cyghdf5-200.dll apparently?

@mkitti
Copy link
Contributor Author

mkitti commented Jan 16, 2023

Building h5py/h5py@01be823 with this patch works...

diff --git a/setup_configure.py b/setup_configure.py
index a36aa1fd..d0b67860 100644
--- a/setup_configure.py
+++ b/setup_configure.py
@@ -249,8 +249,8 @@ class HDF5LibWrapper:
                 default_path = 'hdf5.dll'
                 regexp = re.compile(r'^hdf5.dll')
             else:
-                default_path = 'libhdf5-0.dll'
-                regexp = re.compile(r'^libhdf5-[0-9].dll')
+                default_path = 'libhdf5.dll'
+                regexp = re.compile(r'^libhdf5.dll')
             if sys.version_info >= (3, 8):
                 # To overcome "difficulty" loading the library on windows
                 # https://bugs.python.org/issue42114

It does look like we're going to need a h5py release though to get the h5py package to work with HDF5 1.14.

@mkitti
Copy link
Contributor Author

mkitti commented Jan 16, 2023

Without h5py master, I get the following error:

$ python
>>> import h5py
D:/msys64/mingw64/lib/python3.10/site-packages/numpy/core/getlimits.py:493: UserWarning: Signature b'\x00\xd0\xcc\xcc\xcc\xcc\xcc\xcc\xfb\xbf\x
1e\xb2*\x00\x00\x00' for <class 'numpy.float128'> does not match any known type: falling back to type probe function.
This warnings indicates broken support for the dtype!
  machar = _get_machar(dtype)

# Gap inserted by me for clarity

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "D:/msys64/mingw64/lib/python3.10/site-packages/h5py/__init__.py", line 56, in <module>
    from . import h5a, h5d, h5ds, h5f, h5fd, h5g, h5r, h5s, h5t, h5p, h5z, h5pl
  File "h5py/h5fd.pyx", line 220, in init h5py.h5fd
RuntimeError: Wrong file driver version # (wrong file driver version #)

The fix for this is h5py/h5py#2187 which is now merged to h5py master branch.

Do we cherry pick this, pull h5py master, or wait for h5py to release a new version before proceeding?

@mkitti
Copy link
Contributor Author

mkitti commented Jan 16, 2023

I cherry picked h5py/h5py#2194

We can now import h5py

Type "help", "copyright", "credits" or "license" for more information.
>>> import h5py
D:/msys64/mingw64/lib/python3.10/site-packages/numpy/core/getlimits.py:493: UserWarning: Signature b'\x00\xd0\xcc\xcc\xcc\xcc\xcc\xcc\xfb\xbf\x
1eS\x88\x00\x00\x00' for <class 'numpy.float128'> does not match any known type: falling back to type probe function.
This warnings indicates broken support for the dtype!
  machar = _get_machar(dtype)
>>> 

@mmuetzel
Copy link
Collaborator

If you prefer to remove the SOVERSION suffix now, you'll need to rebuild all dependers as part of this PR. And check that the respective configuration steps are still able to detect the library without the suffix.

@mmuetzel
Copy link
Collaborator

check that the respective configuration steps are still able to detect the library without the suffix.

configuration steps search for import libraries which do not have soversion

Apparently, at least h5py is looking for the .dll.

@@ -23,16 +23,20 @@ makedepends=("${MINGW_PACKAGE_PREFIX}-cython"
"${MINGW_PACKAGE_PREFIX}-pkg-config")
source=(https://github.com/h5py/h5py/releases/download/${pkgver}/h5py-${pkgver}.tar.gz
001-mingw-python.patch
002-Merge-pull-request-2194-from-h5py-libhdf5-1.14.patch
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, use spaces instead of tab for indentation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When cherry-picking a patch from upstream, it is preferable to download the patch directly instead of checking in a copy here:

Suggested change
002-Merge-pull-request-2194-from-h5py-libhdf5-1.14.patch
002-Merge-pull-request-2194-from-h5py-libhdf5-1.14.patch::https://github.com/h5py/h5py/commit/3dc3d7c089acaa4532f43ad2283e65a440ec55ac.patch

That will also make it clearer where this patch came from
That is, in case it applies as-is. If it does, don't forget to update the checksums if necessary and remove the patch from the repository.

Copy link
Contributor Author

@mkitti mkitti Jan 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I literally git cherry-picked it from the master branch on to the 3.7.0 tag since I did not know if the the patch would apply directly. Let me see if I can generate the patch directly from the commit and have it apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to download the patch directly because it is a merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, use spaces instead of tab for indentation.

Fixed in 0461af9

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to download the patch directly because it is a merge.

Does the suggested change not download a patch that suffices. On a superficial look, the only difference to the merge is changes to their CI (which we don't need). Did I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the annotation in 76568a0

Copy link
Collaborator

@mmuetzel mmuetzel Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With that changed, please delete the checked in copy of the patch (both locally and from the repository) and update with updpkgsums.

Edit: Maybe also change the name of the patch if you think that makes sense.

@mkitti
Copy link
Contributor Author

mkitti commented Jan 20, 2023

There is a license file currently packaged at mingw64/share/doc/hdf5/COPYING. Do you want to remove it from that location and move it to mingw64/share/license/hdf5/COPYING ?

@mmuetzel
Copy link
Collaborator

There is a license file currently packaged at mingw64/share/doc/hdf5/COPYING. Do you want to remove it from that location and move it to mingw64/share/license/hdf5/COPYING ?

In that case, nevermind. Leave it as is. 👍

@mmuetzel
Copy link
Collaborator

mmuetzel commented Jan 22, 2023

I installed the MINGW64 build artifact from this PR and tried to re-run the test suite of Octave.
It is failing with the following error:

  libinterp\octave-value\ov-oncleanup.cc-tst ..................... pass    1/1
  libinterp\octave-value\ov-range.cc-tst .........................Warning! ***HDF5 library version mismatched error***
The HDF5 header files used to compile this application do not match
the version used by the HDF5 library to which this application is linked.
Data corruption or segmentation faults may occur if the application continues.
This can happen when an application was compiled by one version of HDF5 but
linked with a different version of static or shared HDF5 library.
You should recompile the application or check your shared library related
settings such as 'LD_LIBRARY_PATH'.
You can, at your own risk, disable this warning by setting the environment
variable 'HDF5_DISABLE_VERSION_CHECK' to a value of '1'.
Setting it to 2 or higher will suppress the warning messages totally.
Headers are 1.12.2, library is 1.14.0
        SUMMARY OF THE HDF5 CONFIGURATION
        =================================
[...]
Bye...
fatal: caught signal fatal: caught signal C:\msys64\mingw64\bin\octave.exe: No error
Segmentation fault

It is likely that some (or all?) of the dependent packages need to be rebuilt even without the SOVERSION bump.

For a list of packages that likely need a pkgrel bump, see e.g. the Required by section here:
https://packages.msys2.org/package/mingw-w64-ucrt-x86_64-hdf5?repo=ucrt64

@mmuetzel
Copy link
Collaborator

mmuetzel commented Jan 23, 2023

The PR doesn't apply again. That might be because of the merge commit. Could you please try to strip that (and maybe rebase your changes instead)?

Additionally, adios2 fails to compile with the following error:

  C:/_/mingw-w64-adios2/src/ADIOS2-2.8.3/source/h5vol/H5VolUtil.c:59:48: error: unknown type name 'usigned'; did you mean 'signed'?
     59 | void gUtilConvert(hsize_t *fromH5, size_t *to, usigned int ndims)
        |                                                ^~~~~~~
        |                                                signed

That looks like a typo in one of our patches to me:

+void gUtilConvert(hsize_t *fromH5, size_t *to, usigned int ndims)

That should probably be unsigned (not usigned). I wonder why that didn't fail before.

@mkitti
Copy link
Contributor Author

mkitti commented Jan 23, 2023

Let's get this working first, and then I'll rebase. Do you want the pkgrel bumps all in one commit or multiple?

@mmuetzel
Copy link
Collaborator

Let's get this working first, and then I'll rebase.

Whatever you prefer. But keep in mind that stripping a merge commit might be getting more and more complicated if you push further changes on top of it.

Do you want the pkgrel bumps all in one commit or multiple?

Whatever works best for you. I personally often separate commits if they are more than "just" a pkgrel bump.

mingw-w64-adios2/PKGBUILD Outdated Show resolved Hide resolved
@mmuetzel
Copy link
Collaborator

Please, wait a second with further pushes. I opened #15223 which will hopefully "future proof" the uint replacement for adios2.

@mkitti mkitti force-pushed the mkitti/mingw-w64-hdf5_1_14_0 branch from 64d6f6c to 3432e83 Compare January 23, 2023 09:19
@mkitti
Copy link
Contributor Author

mkitti commented Jan 23, 2023

Sorry. I did not see your comment until just now. I rebased without a rebuild for adios2 incorporated to see if there are any other challenges ahead.

@mmuetzel
Copy link
Collaborator

I rebased without a rebuild for adios2 incorporated to see if there are any other challenges ahead.

Good idea. 👍 Let's see how that fares.

In the meantime, #15223 is merged. When the current CI run has finished, please rebase again and bump the pkgrel of the updated adios2 package.

@mmuetzel
Copy link
Collaborator

The pkgrel of Octave also needs to get bumped. (That might have gone lost when you rebased.)

But that might have been a "lucky error" in the current test run because it takes pretty long to compile. If we are lucky, the CI will finish without it before the 6(?) hours timeout.

@mmuetzel
Copy link
Collaborator

CI timed out. But at least until that point, it didn't fail...

I asked on Discord if it would be ok to merge anyway (after the pkgrel of Octave and ADIOS2 is bumped again).

@mmuetzel mmuetzel merged commit 2df89b0 into msys2:master Jan 23, 2023
@mmuetzel
Copy link
Collaborator

I rebased, bumped the pkgrel of the remaining packages and merged.

@mkitti: Thank you very much!

@MehdiChinoune
Copy link
Collaborator

We missed the opportunity to enable SOVERSION for HDF5.

@mmuetzel
Copy link
Collaborator

We missed the opportunity to enable SOVERSION for HDF5.

We can still do that in another PR.
IIRC, you were ok with either keeping -0 or enabling SOVERSION.

@MehdiChinoune
Copy link
Collaborator

IIRC, you were ok with either keeping -0 or enabling SOVERSION.

Keeping -0 to avoid rebuilding all dependent packages, but you rebuild all of them after all.

@mmuetzel
Copy link
Collaborator

Sorry. That wasn't clear to me. I'd thought this was because some packages seemed to look for a hard-coded -0 at the end of the library name...

@mmuetzel
Copy link
Collaborator

But I agree: The way how the library refuses to work if the version in the public headers doesn't match the library essentially means that it requires a SOVERSION.

@mkitti
Copy link
Contributor Author

mkitti commented Jan 23, 2023

h5py seems to allow a range of single digit numbers for the SOVERSION. Frankly, the patch applied to h5py was intended specifically for msys2. It seems they would be happy to accommodate whatever scheme is determined here.

Before coming up with our own scheme, I also suggest researching what happens on other platforms. I can communicate with The HDF Group directly regarding their opinion on the matter as well.

@lazka
Copy link
Member

lazka commented Jan 23, 2023

vigra failed to build, as it doesn't find boost python (despiteit being there) and then later on fails when fixing up the then missing cmake files.

edit: #15231

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.

5 participants