search

Starlink / starlink

Starlink Software Collection
162 stars 53 forks source link

Replace HDS with HDF #58

Closed timj closed 6 years ago

timj commented 7 years ago

This pull request allows for an HDSv5 to coexist with the existing HDSv4. v5 is based on HDF.

timj commented 7 years ago

I'd rather you didn't cherry pick commits to the pull request. When we merge this in we'll end up with lots of duplicate commits in the history. I'd prefer it if the PR was rebased on master as needed so we get a clean merge when done (I'd like a merge commit when we do merge so it's obvious how the HDS changes are related).

dsberry commented 7 years ago

ok

timj commented 6 years ago

I really am starting to worry about this pull request becoming a PR for "rewrite NDF in C and switch HDS to HDF5". These are distinct independent work packages that could be released on different timescales. ARY can be done independently of NDF if the Fortran APIs for ARY continue to work even if the internals are C.

dsberry commented 6 years ago

The problem is C ARY uses the new thread-locking functions I've added to HDS5, but I can't merge HDS5 into master yet because it's had very little testing at EAO. I'll be talking about testing HDS5 with the rest of scicom, but in the meantime it seemed simple enough just to treat the HDS5 branch as a "New data system" branch and include C ARY and NDF in there. I suppose I could make C ARY a branch off the hds5 branch... Or maybe I could just add some stubs for the new HDS thread-locking functions into HDS4 (i.e. master) and then I could make C ARY a simple branch of master.

dsberry commented 6 years ago

I'm hoping I've sorted this. I've modified hds on master so that the new C version of ARY will build and link with it, and I've moved the commits related to CARY from the hdsv5 branch to a new branch called "cary". Fingers crossed...

grahambell commented 6 years ago

dsberry commented on this pull request.

@@ -49,6 +49,7 @@ hds_test_LDADD = libhdsf.la

hdsTest_SOURCES = hdsTest.c hdsTest_LDADD = libhds.la +hdsTest_CFLAGS = $(AM_CFLAGS) -DHDS_INTERNAL_INCLUDES

@grahambell Why is -DHDS_INTERNAL_INCLUDES needed here, given it is supposed to be testing the installed stuff?

Because hds-v4 or hds-v5 were installing a hds_types.h which would cause the test to fail if they were built more recently than hds.

However I also stopped them from doing that, so maybe we don't need HDS_INTERNAL_INCLUDES at all for anything any more.