aldanor / hdf5-rust

HDF5 for Rust
https://docs.rs/hdf5
Apache License 2.0
310 stars 85 forks source link

Add 1.12.0 support #95

Closed mulimoen closed 4 years ago

mulimoen commented 4 years ago

Add the newest changes according to https://portal.hdfgroup.org/display/HDF5/Software+Changes+from+Release+to+Release+for+HDF5-1.12. Currently compiles and tests complete sucessfully on all platforms but new functions are not tested nor used anywhere.

Fixes #94

aldanor commented 4 years ago

Ok, I'll look at this now, will get back with some comments.

Given the sizeable change list, I'll probably have to diff the C headers myself, just to give it a second look, manually.

The dodgy parts are the whole func1/func2 (which are macroed in C code) as they can cause linkage errors if not done carefully, and the whole enum renumbering business (does it mean it's a breaking change now or not?).

Also, regarding the CI:

mulimoen commented 4 years ago

I'll add some more brew jobs! It seems the HDF group restricted binary releases for HDF5 so we need to log in the get these it seems.

The func1/func2 is a bit annoying. The original names will be shadowed based on API compatability version selected during build time. We can't just point func to point to either func1 or func2 based on the version number. We can point func1 and func to the same function for versions before the introduction of func2 to avoid having a breaking change.

aldanor commented 4 years ago

It seems the HDF group restricted binary releases for HDF5 so we need to log in the get these it seems.

I've raised this on their forum, please feel free to chime in :) https://forum.hdfgroup.org/t/please-provide-stable-urls-for-official-binaries-without-auth/7378

mulimoen commented 4 years ago

I went through and manually diffed the HPXpublic.h headers, which resulted in some additional changes to several more enums.

aldanor commented 4 years ago

@mulimoen I've left a few comments, most of them cosmetic/minor.

One concern here is this: we will now import things like H5Literate1 in hdf5::hl::group which, I think, will cause deprecated warnings for downstream builds on 1.12 (correct me if I'm wrong or missing something).

mulimoen commented 4 years ago

import things like H5Literate1 in hdf5::hl::group which, I think, will cause deprecated warnings for downstream builds on 1.12

Unfortunately this breaks compile time tests, so I've set these to be ignored. I'll open an issue for dealing with deprecated items.

mulimoen commented 4 years ago

It was trivial to use v2 of the API...

aldanor commented 4 years ago

Skimming through changes again, I think it looks good to go, we'll fix up the opened tickets later. Any thoughts?

Would probably make sense to release a version too to include all the recent changes.

mulimoen commented 4 years ago

@aldanor I rebased and fixed two renamings, so we are compatible on minor semver.

It would be great to have a new version cut, both to test docs.rs, and to make it easier to build hdf5 on all platforms!

aldanor commented 4 years ago

Cool! Just two very minor deprecated message fixes (for consistency throughout), and I'll merge this in then.

I'll get to pushing a release then - I was thinking 0.7.0, just because of the amount of stuff, changes to the build system and a new major HDF5 version.

mulimoen commented 4 years ago

And fixed!

gauteh commented 4 years ago

It seems hdf5-src is still tagged to 1.10, any reason for that?

aldanor commented 4 years ago

I really think we should support multiple major versions for static builds

gauteh commented 4 years ago

Yes, that would be great. Seems to be significant breakage between hdf5 versions. So would be great to pin to an older version of hdf-src.

fre. 28. aug. 2020, 20:12 skrev Ivan Smirnov notifications@github.com:

I really think we should support multiple major versions for static builds

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/aldanor/hdf5-rust/pull/95#issuecomment-683020991, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAN367TEOTGMO6OWFDLZP3SC7XSNANCNFSM4MYBWFYQ .

aldanor commented 4 years ago

Problem is, if we ship 1.8/1.10/1.12/1.13, we'd have to include source code for all those HDF5 versions, thus quadrupling the size which is already substantial.

Idk, maybe we can include zipped source in the crate and unpack it at build time?

mulimoen commented 4 years ago

@gauteh hdf5-src is tagged to 1.10 as that should be well supported by 'netCDF', which is why this was implemented in the first place

mulimoen commented 4 years ago

@aldanor we could have a base version, and a zipped diff for every other version released. This should not be too massive

aldanor commented 4 years ago

@mulimoen Source can be compressed 5-6x, so I think even if we provide 4 packed versions it will be smaller than the current crate size

gauteh commented 4 years ago

Ivan Smirnov writes on August 28, 2020 20:39:

@mulimoen Source can be compressed 5-6x, so I think even if we provide 4 packed versions it will be smaller than the current crate size

Is this for crates.io? Or because you need more submodules?

aldanor commented 4 years ago

For crates.io; and the easiest way would probably be to add more submodules, like ext/1.8, ext/1.10, ext/1.12, etc...