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

Fix for H5Literate() callback versioning #2888

Merged
merged 5 commits into from
Mar 14, 2024
Merged

Conversation

derobins
Copy link
Contributor

The netCDF library supports many versions of HDF5, which handles API compatibility via a set of API-call-specific macros. netCDF uses H5Literate(), which was versioned in the 1.12.x maintenance line in order to better support the virtual object layer (VOL).

h5_test/tst_h_files4.c failed to compile with certain compilers when the HDF5 library was built using pre-VOL versions of the library- e.g., 1.10, which can be configured with --with-default-api-version=110. This was due to the API compatibility macros being used to select the 1.10 version of H5Literate(), but not its callback function, which was set using a H5_VERSION_GE() macro that does not take the compatibility macros into consideration.

Fixing the problem involved removing the H5_VERSION_GE() macro and letting the compatibility macros handle the versioning, and using the H5_USE_XXX_API_DEFAULT symbols to protect the H5Oopen_by_addr() call used in the callback (a new call that wasn't versioned, hence the different protection mechanism).

Tested w/ HDF5's develop branch w/ both 1.14 and 1.10 API bindings

Fixes #2886 (4118 in HDF5's issue tracker)

The netCDF library supports many versions of HDF5, which handles API
compatibility via a set of API-call-specific macros. netCDF uses
H5Literate(), which was versioned in the 1.12.x maintenance line
in order to better support the virtual object layer (VOL).

h5_test/tst_h_files4.c failed to compile with certain compilers when the
HDF5 library was built using pre-VOL versions of the library- e.g., 1.10,
which can be configured with --with-default-api-version=110. This was due
to the API compatibility macros being used to select the 1.10 version of
H5Literate(), but not its callback function, which was set using a
`H5_VERSION_GE()` macro that does not take the compatibility macros into
consideration.

Fixing the problem involved removing the `H5_VERSION_GE()` macro and
letting the compatibility macros handle the versioning, and using the
`H5_USE_XXX_API_DEFAULT` symbols to protect the H5Oopen_by_addr() call
used in the callback (a new call that wasn't versioned, hence the
different protection mechanism).

Tested w/ HDF5's develop branch w/ both 1.14 and 1.10 API bindings

Fixes Unidata#2886 (4118 in HDF5's issue tracker)
@CLAassistant
Copy link

CLAassistant commented Mar 13, 2024

CLA assistant check
All committers have signed the CLA.

@WardF
Copy link
Member

WardF commented Mar 13, 2024

Thank you for the quick fix! It is strange that this did not manifest until recent versions of clang. Thanks!

@derobins
Copy link
Contributor Author

clang must be fussier about function pointer signatures matching exactly.

I see we should probably have H5_API_VERSION_GE/LE macros as well...

WardF
WardF previously approved these changes Mar 13, 2024
@WardF
Copy link
Member

WardF commented Mar 13, 2024

Thanks!

@WardF WardF self-assigned this Mar 13, 2024
@WardF WardF added this to the 4.9.3 milestone Mar 13, 2024
WardF added a commit to WardF/netcdf-c that referenced this pull request Mar 13, 2024
@WardF
Copy link
Member

WardF commented Mar 13, 2024

Fixing the AppVeyor config to use HDF5 1.10.x; also will update the documentation to reflect this new requirement. It does not seem unreasonable to need to drop support for 1.8.x at this point.

@derobins
Copy link
Contributor Author

I'll have a fix for this in a second. I didn't realize 1.8 never had a H5_USE_18_API_DEFAULT symbol defined.

Dana Robinson added 2 commits March 13, 2024 13:40
@derobins
Copy link
Contributor Author

As for 1.8 in AppVeyor, we no longer support any version of the library earlier than 1.14, so people should try to upgrade. Security fixes, build system improvements, etc. will not be heading downstream to unsupported branches.

@WardF WardF merged commit 0f7558c into Unidata:main Mar 14, 2024
103 checks passed
@WardF
Copy link
Member

WardF commented Mar 14, 2024

Thanks! We are moving away from appveyor (as mentioned during our call :)) so I'm not worried about that too much, we'll modernize in the Windows-based github action; our *nix-based testing already uses a matrix of modern HDF5 versions :)

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.

Error compiling h5_test/tst_h_files4 on MacOS
3 participants