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

hdf5: first stab at hdf5-1.10 support #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sbinet
Copy link
Member

@sbinet sbinet commented May 23, 2018

This CL is a first step towards supporting new features and APIs
of HDF5 v1.10.

Right now, anecdotal evidence shows that HDF5 v1.8 is still massively
deployed so support for 1.10 has to be opt-in, by way of an optional
build tag aptly named "hdf5_v1.10"

Updates #16.

Please take a look.

@sbinet sbinet requested a review from kortschak May 23, 2018 14:36
This CL is a first step towards supporting new features and APIs
of HDF5 v1.10.

Right now, anecdotal evidence shows that HDF5 v1.8 is still massively
deployed so support for 1.10 has to be opt-in, by way of an optional
build tag aptly named "hdf5_v1.10"

Updates gonum#16.
@donkahlero
Copy link
Collaborator

That looks good. And to confirm it even further. We are running this lib for quite some time now on HDF5 v1.10.1 without any issues.

@sbinet
Copy link
Member Author

sbinet commented May 23, 2018

I am sure 1.10.x is great :)
no argument about that.

the issue is twofold:

  • IIUC, the way travis works, we can't reliably test gonum/hdf5 against both hdf5-1.8.x and hdf5-1.10.x: we have to pick one.
  • given the limited dev resources in Gonum and in gonum/hdf5 in particular, I don't think it's doable to correctly support both.

I'd be inclined to say we support hdf5-1.8.x (or try to) and accept PRs for 1.10.x.
when/if the HDFGroup declares 1.8.x deprecated, we say we drop it as well and enable 1.10.x by default.
alternatively, we could ask our users if they'd be willing to drop explicit support for 1.8.x and focus on 1.10.x (but this would involve some .travis.yml massaging to get the correct version installed there, I suppose...)

this PR was just for me to see how one could achieve support for 1.10.x w/o disturbing 1.8.x and lay some ground infra work down :)

@kortschak
Copy link
Member

IIUC, the way travis works, we can't reliably test gonum/hdf5 against both hdf5-1.8.x and hdf5-1.10.x: we have to pick one.

I don't think this is true. We could use an environment variable to choose which is installed and built against.

// #include "hdf5.h"
import "C"

// StartSWMRWrite enables SWMR writing mode for this file.
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth adding this to the pre-1.10 being an error-returning noop?

// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

//+build hdf5_v1.10
Copy link
Member

Choose a reason for hiding this comment

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

I think the convention more broadly has become // +build ....

@gustafj
Copy link
Contributor

gustafj commented May 24, 2018

IIUC, the way travis works, we can't reliably test gonum/hdf5 against both hdf5-1.8.x and hdf5-1.10.x: we have to pick one.

I don't think this is true. We could use an environment variable to choose which is installed and built against.

You can use the matrix expansion support in Travis to build against different versions.
I cant say how it would be done in the hdf5 case though (don't know enough about the your build setup).

Some references:
https://docs.travis-ci.com/user/build-stages/matrix-expansion/
https://docs.travis-ci.com/user/customizing-the-build/#Build-Matrix

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.

4 participants