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

Wrap more of the HDF5 API with generated bindings #677

Merged
merged 1 commit into from
Sep 19, 2020

Conversation

jmert
Copy link
Contributor

@jmert jmert commented Sep 18, 2020

This wraps more direct uses of ccalls with bindings made with the generator.

There are a few notable exceptions due to either (a) memory handling which the binding generator cannot (yet?) deal with, and (b) a case where a call to the underlying H5Dwrite is relying on being able to declare the buffer to type Cstring for its data validation features.

One more interesting change is to the h5t_get_member_name and h5t_get_tag functions — this changes to using h5_free_memory from Libc.free as noted in the h5t_get_member_name documentation (which says it's more important on Windows).

src/HDF5.jl Show resolved Hide resolved
src/HDF5.jl Outdated Show resolved Hide resolved
src/HDF5.jl Outdated Show resolved Hide resolved
src/HDF5.jl Outdated Show resolved Hide resolved
src/HDF5.jl Outdated Show resolved Hide resolved
@musm
Copy link
Member

musm commented Sep 19, 2020

LGTM and a good clean up, just some minor comments!

@jmert
Copy link
Contributor Author

jmert commented Sep 19, 2020

I added a second commit that moves the definition of h5_get_libversion() and the corresponding use to set the libversion constant. That definition was being used to implicitly initialize the library, so to keep the read_const function working requires an explicit ccall to initialize the library.

It's a bit ugly, but that should maybe just be taken as motivation to work on sorting out the initialization process.

@jmert
Copy link
Contributor Author

jmert commented Sep 19, 2020

Hmmm... MacOS nightly failed in both Travis and Github Actions, apparently with libhdf5 not being defined, which should be imported from HDF5_jll and works as expected everywhere else (and this PR didn't change that).

@musm
Copy link
Member

musm commented Sep 19, 2020

Strange, let's drop that commit for now so we can get the rest of the changes in?

@jmert
Copy link
Contributor Author

jmert commented Sep 19, 2020

Strange, let's drop that commit for now so we can get the rest of the changes in?

I've backed off this branch by one commit. Let's see if MacOS CI is still unhappy or not.

Edit: Still fails. Looking at other commits to master, though, and I'm finding that MacOS nightly was already failing there, too — https://github.com/JuliaIO/HDF5.jl/runs/1137069384 — so it's not an interaction with this PR specifically.

@jmert
Copy link
Contributor Author

jmert commented Sep 19, 2020

Something needs to be fixed in HDF5_jll — I made a branch that just prints a bunch from deps/deps.jl, and it's showing that there is no library on MacOS with Julia latest: https://github.com/jmert/HDF5.jl/runs/1138352787?check_suite_focus=true

@musm
Copy link
Member

musm commented Sep 19, 2020

Thanks for detecting this bug. Let's see if we can get that fixed. So I guess this PR discovered a bug in the HDF_jll wrapper which is good.

@musm musm merged commit 9c39686 into JuliaIO:master Sep 19, 2020
@jmert jmert deleted the more_api branch September 19, 2020 18:45
@musm
Copy link
Member

musm commented Sep 19, 2020

osx on master should now be fixed

@musm
Copy link
Member

musm commented Sep 20, 2020

want to try that second commit now?

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.

2 participants