Addresses issue: Update HDF5 format compatibility #951

[brtnfld]

Allow for the HDF5 file format to be compatible with features greater than HDF5 1.8. This will allow for HDF5 features (VDS, SWMR, new references, etc...) which may require to have a superblock greater than v2.

See for discussion Ref: Update HDF5 format compatibility #951

[brtnfld]

I'm not sure how to regenerate the test's *.nc reference files, I've not found a script that generated them and the ncdump reference files.

[Dave-Allured]

@brtnfld, why is there a need to regenerate the *.nc reference files? If they contain a format requirement, then (hypothetically) the related test would fail with some HDF5 library versions, i.e. library version compatibility would be lost.

[Dave-Allured]

P.S. Thanks for correcting my wishful thinking mistake with H5_VERSION_LT, which does not exist.

[brtnfld]

It looks like ncdump dumps the superblock for some of the tests. Since we bumped the superblock to min v2, it fails when it compares it to the reference dump. I think there might be an issue with 1.10.0 and 1.10.1 since they will default back to v0. So we would need two different reference files? I'm not all that familiar with the intricacies of the testing framework.

[Dave-Allured]

Yes, I too am not familiar. If there really are some superblock tests or whole-file tests in the netcdf test framework, then double reference files may be necessary.

A possible alternate strategy would be to simply emit "Expected Fail" when the particular unit tests fail, and HDF5 library 1.10.0 or 1.10.1 is detected.

[Dave-Allured]

Something is not right. The HDF5 version number is embedded in _NCProperties for all netcdf-4 formats, and the test suite must have a way of handling this. So varying superblock number should be handled in the same way as varying HDF5 version number. So there should already be an existing solution, and varying superblock should just be fitted into that solution, if you see what I mean.

[brtnfld]

The tests are filtering out that data:

# Remove the version information from _NCProperties
cleanncprops() {
  src="$1"
  dst="$2"
  rm -f $dst
  | sed -e '/:_Endianness/d' \
  | sed -e 's/_SuperblockVersion = 1/_SuperblockVersion = 0/' \
  | sed -e 's/\(netcdflibversion\|netcdf\)=.*|/\1=NNNN|/' \
  | sed -e 's/\(hdf5libversion\|hdf5\)=.*"/\1=HHHH"/' \
  | grep -v '_NCProperties' \
  | cat >$dst
}

I think I can modify this to use the HDf5 version number to sed the correct Superblock version.

[Dave-Allured]

I think you are on the right track. Here is a simple thing that does not add more library version number checks, just in case you did not already try this. Old/new:

  | sed -e 's/_SuperblockVersion = 1/_SuperblockVersion = 0/' \

  | sed -e 's/_SuperblockVersion = [12]/_SuperblockVersion = 0/' \

Both original and new versions presume that the reference files are always superblock version 0. This too can be changed if desired.

[DennisHeimbigner]

The point of the filtering is to convert the info to some consistent value.
So, the PR is incorrect wrt to the changes to ncdump/ref_tst_ncf213.cdl
and ncdump/ref_tst_special_atts.cdl and these should be left unchanged.
The correct solution is the change above #1931 (comment)
although I think the more general change is to use this:
| sed -e 's/_SuperblockVersion = [0-9]/_SuperblockVersion = 0/' \

[DennisHeimbigner]

As far as I know, there are not changes that look for a specific superblock number,
so the filtering above is just to force the expected output to be consistent
independent of the superblock number.
Do you plan to add tests that actually look-at/check the superblock number?

[Dave-Allured]

@DennisHeimbigner, agreed, this filtering is mainly to force consistent values. Now that I understand a little more, I agree, leave those two ref_tst.cdl's alone.

However, an advantage of my constrained suggestion sed -e 's/_SuperblockVersion = [12]/ is that you get a free test for unwanted superblock numbers >= 3, which are not universally readable. 'Tis a cheap trick, but would be valuable for unexpected circumstances.

[brtnfld]

That is good if I can ignore the value of the superblock version. I was not planning on adding any tests where the superblock version is verified, but I like the idea of checking for >=3.

[DennisHeimbigner]

you get a free test for unwanted superblock numbers >= 3, which are not universally readable.

Had not considered that; It is a good idea.

[WardF]

Reviewing this and other PR's now in preparation for the 4.8.0 release. Clearly, things have stacked up as we prepped the zarr-compatible release. Thanks all!

[WardF]

Merging in to 4.8.0-wellspring.wif which will percolate back into the main branch after v4.8.0 is released.

[Dave-Allured]

@WardF, please clarify. Does this mean that this #951 HDF5 compatibility update will be postponed until after 4.8.0 release? I am not objecting, I just want to understand the timing.

[WardF]

@Dave-Allured No problem. It will be part of the 4.8.0 release. The 'wellspring' branch contains changes that can be merged back into main and also into the v4.8.0-prep branch; it's essentially the 'staging grounds' branch. This way we are able to accommodate changes that aren't meant to go into a particular release, and also makes it easier to avoid polluting the main branch with release-specific files.

These release-specific files are another thing I'd like to sort out (or at least review to make sure our process is as sorted as it can currently be). Specifically, we include automake/autoconf-generated files. In the absence of these files, users would need to have a subset of the gnu build tools installed on their system. The downside is we are including generated files in revision control, which is not a great practice.

But I digress, you asked about the inclusion of this patch in 4.8.0, and yes, barring any surprises during testing, 4.8.0 will include this patch. The PR will be marked as merged when #1961 is merged.

[Dave-Allured]

If it is not too much trouble, would you please add two more auto-checks? Testing with HDF5 1.10.1 is needed to properly challenge these HDF5 format compatibility updates. I do not think that any other combinations such as nc3 or dap_on are needed for this purpose.

netCDF Tests / nc-autotools (1.10.1, nc4, dap_off)
netCDF Tests / nc-cmake (1.10.1, nc4, dap_off)

It is possible that HDF5 1.10.1 will cause some unrelated failures which should then be ignored.

[WardF]

@Dave-Allured I've added the 1.10.1 entry to the test matrix, thanks!