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

added check H5_USE_1xx_API to make sure H5 is not using older API. #3156

Merged
merged 1 commit into from
Apr 5, 2022

Conversation

guj
Copy link
Contributor

@guj guj commented Apr 4, 2022

No description provided.

@guj guj linked an issue Apr 4, 2022 that may be closed by this pull request
Copy link
Contributor

@chuckatkins chuckatkins left a comment

Choose a reason for hiding this comment

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

Can you rebase this on release_28 re-target it for that branch so it can easily go into a patch release?

@chuckatkins
Copy link
Contributor

Fixes #3149

@chuckatkins
Copy link
Contributor

@eschnett does this fix #3149 for you?

@eschnett
Copy link
Contributor

eschnett commented Apr 4, 2022

HDF5 offers a simpler way to achieve this. I recommend calling H5Oget_info1 instead, which always accepts 2 arguments. Alternatively, H5Oget_info2 always accepts 3 arguments. These functions should exist starting from HDF5 1.10.5.

#if H5_VERSION_GE(1, 10, 5)
    herr_t ret = H5Oget_info1(m_FileId, &oinfo);
#else
    herr_t ret = H5Oget_info(m_FileId, &oinfo);
#endif

@guj
Copy link
Contributor Author

guj commented Apr 4, 2022

HDF5 offers a simpler way to achieve this. I recommend calling H5Oget_info1 instead, which always accepts 2 arguments. Alternatively, H5Oget_info2 always accepts 3 arguments. These functions should exist starting from HDF5 1.10.5.

#if H5_VERSION_GE(1, 10, 5)
    herr_t ret = H5Oget_info1(m_FileId, &oinfo);
#else
    herr_t ret = H5Oget_info(m_FileId, &oinfo);
#endif

Yes, I am aware of that. I just happen to prefer using the name get_info instead of get_info.

@eschnett
Copy link
Contributor

eschnett commented Apr 4, 2022

The condition defined H5_USE_16_API && defined H5_USE_18_API looks wrong. I think the individual API selection switches are exclusive.

Do you have CI infrastructure set up to test with both older and newer HDF5 versions?

@guj
Copy link
Contributor Author

guj commented Apr 4, 2022

The condition defined H5_USE_16_API && defined H5_USE_18_API looks wrong. I think the individual API selection switches are exclusive.

Do you have CI infrastructure set up to test with both older and newer HDF5 versions?

You are right. It should be ||.

Updated and checked with 1.13 with DEFAULT_API_VERSION=v18

@chuckatkins chuckatkins added this to the ADIOS v2.8.1 Patch Release milestone Apr 4, 2022
@chuckatkins chuckatkins added the bug label Apr 4, 2022
@chuckatkins chuckatkins changed the base branch from master to release_28 April 4, 2022 17:57
@chuckatkins
Copy link
Contributor

Rebased

@chuckatkins chuckatkins dismissed their stale review April 4, 2022 18:46

I took care of the rebase

@chuckatkins chuckatkins merged commit c2d9c6d into ornladios:release_28 Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HDF5 API checks are wrong
3 participants