Fix 'make check-vfd' target for Autotools #4211

jhendersonHDF · 2024-03-22T04:25:22Z

Changes Autotools testing to use HDF5_TEST_DRIVER environment variable to avoid running tests that don't work well with several VFDs

Restores old h5_get_vfd_fapl() testing function to setup a FAPL with a particular VFD

jhendersonHDF · 2024-03-22T04:27:48Z

config/cmake/HDF5Macros.cmake

@@ -70,7 +70,13 @@ macro (H5_SET_VFD_LIST)
      split
      multi
      family
-      splitter
+      # Splitter VFD currently can't be tested with the h5_fileaccess()


The splitter VFD was previously being tested just with the environment variable approach, where it would add "_wo" to the end of filenames to create the name for the W/O file, which worked well in testing. With the changes in this PR though, any tests that use h5_fileaccess() would setup a FAPL with a single W/O path that gets used for any file create or open calls that use that FAPL, leading to failures due to multiple locks on the same file. We should revisit this at some point.

jhendersonHDF · 2024-03-22T04:28:09Z

config/cmake/HDF5Macros.cmake

@@ -82,16 +88,21 @@ macro (H5_SET_VFD_LIST)
    # list (APPEND VFD_LIST mpio)
  endif ()
  if (H5_HAVE_MIRROR_VFD)
-    list (APPEND VFD_LIST mirror)
+    # Mirror VFD needs network configuration, etc. and isn't easy to set
+    # reasonable defaults for that info.


Matching what's in Autotools

jhendersonHDF · 2024-03-22T04:30:23Z

src/H5FDtest.c

@@ -89,7 +89,10 @@ H5FD__supports_swmr_test(const char *vfd_name)

    FUNC_ENTER_NOAPI_NOINIT_NOERR

-    if (!vfd_name || !strcmp(vfd_name, "") || !strcmp(vfd_name, "nomatch"))


No need for the "nomatch" case. It was only checked in a few places where we can just as easily return "sec2" for the same behavior.

jhendersonHDF · 2024-03-22T04:30:48Z

test/CMakeVFDTests.cmake

@@ -71,6 +71,9 @@ add_custom_target(HDF5_VFDTEST_LIB_files ALL COMMENT "Copying files needed by HD
      links_env
      external_env
      vds_env
+      mirror_vfd
+      ros3
+      hdfs


Exclude VFD-specific tests from CMake's VFD testing, as it often doesn't make sense to run these tests with other VFDs.

jhendersonHDF · 2024-03-22T04:36:01Z

test/h5test.c

+ *-------------------------------------------------------------------------
+ */
+herr_t
+h5_get_vfd_fapl(hid_t fapl)


Restored old h5_get_vfd_fapl() function and added a few VFDs to it.

Changes Autotools testing to use HDF5_TEST_DRIVER environment variable to avoid running tests that don't work well with several VFDs Restores old h5_get_vfd_fapl() testing function to setup a FAPL with a particular VFD

test/h5test.c

qkoziol · 2024-03-22T16:48:10Z

test/h5test.c

+
+    assert(H5_DEFAULT_VFD == H5FD_SEC2);
+
+    if ((envval = getenv(HDF5_DRIVER)))


This seems to be mixing the HDF5_DRIVER source macro and the environment variable "HDF5_DRIVER".

On advice from someone else, I started defining macros that map to the name of the environment variable for maintenance. HDF5_DRIVER = "HDF5_DRIVER". We haven't really had an HDF5_DRIVER source macro for a while and I don't think we ever tested that that approach was working correctly.

I agree - I think the HDF5_DRIVER macro should be removed. That would clean up the code in this area quite a bit.

If it's your new macro, you should have one for the HDF5_TEST_DRIVER environment variable also.

I would also suggest putting 'ENV' somewhere in their names, so that it's a little more clear what their purpose is.

I'm not sure I understand. As far as I remember, HDF5_DRIVER never existed anywhere in the library or configure code until it was added as HDF5_DRIVER = "HDF5_DRIVER". One could have passed it as a compile option, but I don't think it was documented and we definitely didn't have a test for that at the time I worked on this previously. So it seems there's not much to remove here other than the one #ifdef HDF5_DRIVER I removed in this PR. HDF5_DRIVER is also in H5public.h so I'd prefer not to change it, but open to any discussion there.

No problem adding a macro for "HDF5_TEST_DRIVER", though my intention for this is for it to be a temporary environment variable, so that one would probably go in a private header somewhere. Using "HDF5_TEST_DRIVER" is just a way for CMake and Autotools to play nicely together since Autotools doesn't pick and choose which tests to run with all the VFDs and HDF5_DRIVER works similar to how HDF5_VOL_CONNECTOR does for setting something on a FAPL. Because of that, make check-vfd causes Autotools to run every single test with all the VFDs and fails in a lot of places while CMake is conservative about what it tests. It was an oversight at the time I implemented the VFD plugins feature.

I should clarify that by temporary, I mean that in the future we need to refactor what tests get run for VFD testing so Autotools can test more things. But until then, "HDF5_TEST_DRIVER" is a stopgap that lets CMake keep generally testing the same things it was with VFDs, while preventing Autotools from running too many tests against VFDs.

qkoziol · 2024-03-22T16:52:13Z

test/h5test.c


    if (drv_name)
-        return (!strcmp(drv_name, "sec2") || !strcmp(drv_name, "nomatch"));
+        return !strcmp(drv_name, "sec2");


It would be nice if this used the same symbol as the library to determine the default VFD, instead of hard-coding the "sec2" string here.

Seems easy enough to add a H5_DEFAULT_VFD_NAME macro if that's what you mean

Added this and updated a few occurrences that seemed reasonable to change.

qkoziol · 2024-03-22T17:04:18Z

Generally looking good, especially if we can resolve the HDF5_DRIVER / "HDF5_TEST_DRIVER" asymmetry.

jhendersonHDF · 2024-03-23T02:30:57Z

@qkoziol Merging this so that we have VFD testing back in a working state, but happy to follow up on any discussion here in a separate PR if needed.

qkoziol · 2024-03-23T18:49:14Z

@qkoziol Merging this so that we have VFD testing back in a working state, but happy to follow up on any discussion here in a separate PR if needed.

Sorry, couldn't get back to this yesterday - yes, I understand your comments and agree that the way you have it now (as a temporary solution, hopefully) is a good way forward.

Changes Autotools testing to use HDF5_TEST_DRIVER environment variable to avoid running tests that don't work well with several VFDs Restores old h5_get_vfd_fapl() testing function to setup a FAPL with a particular VFD Adds a macro for the default VFD name

* Call memset before stat calls (#4202) The buffers passed to stat-like calls are only partially filled in by the call, leaving ununitialized memory areas when the stat buffers are created on the stack. This change memsets the buffers to 0 before the stat calls, quieting the -fsanitze=memory complaints. * Remove unused CMake configuration checks (#4199) * Update link to Chunking in HDF5 page (#4203) * Fix H5Pset_efile_prefix documentation error (#4206) Fixes GH issue #1759 * Suggested header footer for NEWSLETTER (#4204) * Suggested header footer for NEWSLETTER * Updates * Add NEWSLETTER.txt to h5vers script * Fix grammar in README.md release template (#4201) * Add back snapshot names (#4198) * Use tar.gz extension for ABI reports (#4205) * Fix issue with Subfiling VFD and multiple opens of same file (#4194) * Fix issue with Subfiling VFD and multiple opens of same file * Update H5_subfile_fid_to_context to return error value instead of ID * Add helper routine to initialize open file mapping * Reverts AC_SYS_LARGEFILE change (#4213) We previously replaced local macros with AC_SYS_LARGEFILE, which is unfortunately buggy on some systems and does not correctly set the necessary defines, despite successfully detecting them. This restores the previous macro hacks to acsite.m4 * Propagate group creation properties to intermediate groups (#4139) * Add clarification for current behavior of H5Get_objtype_by_idx() (#4120) * Addressed Fortran issues with promoted integers and reals via compilation flags (#4209) * addressed issue wit promoted integers and reals * added option to use mpi_f08 * Summarize the library version table (#4212) Fixes GH-3773 * Fix URLs (#4210) Also removed Copyright.html context because it's no longer valid. * Fix 'make check-vfd' target for Autotools (#4211) Changes Autotools testing to use HDF5_TEST_DRIVER environment variable to avoid running tests that don't work well with several VFDs Restores old h5_get_vfd_fapl() testing function to setup a FAPL with a particular VFD Adds a macro for the default VFD name * Revert "Addressed Fortran issues with promoted integers and reals via compil…" (#4220) This reverts commit 06c42ff. * Backup and clear CMAKE_C_FLAGS before performing _Float16 configure checks (#4217) * Fix broken links (#4224) * Fix broken URLs in documentation (#4214) Fixes GH-3881 partially. There are pages that need to be recreated. * Avoid file size checks in C++ testhdf5 for certain VFDs (#4226) * Fix an issue with type size assumptions in h5dumpgentest (#4222) * Fix issue with -Werror cleanup sed command in configure.ac (#4223) * Fix Java JNI warnings (#4229) * Rework snapshots/release workflows for consistent args (#4227) * Fixed a cache assert with too-large metadata objects (#4231) If the library tries to load a metadata object that is above the library's hard-coded limits, the size will trip an assert in debug builds. In HDF5 1.14.4, this can happen if you create a very large number of links in an old-style group that uses local heaps. The library will now emit a normal error when it tries to load a metadata object that is too large. Partially addresses GitHub #3762 * Set DXPL in API context for native VOL attribute I/O calls (#4228) * Initialize a variable in C++ testhdf5's tattr.cpp (#4232) * Addressed Fortran issues with promoted integers and reals via compilation flags, part 2 (#4221) * addressed issue wit promoted integers and reals * fixed h5fcreate_f * added option to use mpi_f08 * change the kind of logical in the parallel tests * addressed missing return value from callback * Use cp -rp in test_plugin.sh (#4233) When building with debug symbols on MacOS, the cp -p commands in test_plugin.sh will attempt to copy the .dSYM directories with debugging info, which will fail since -r is missing. Using cp -rp is harmless and allows the test to run Fixes HDFFV-10542 * Clean up types in h5test.c (#4235) Reduces warnings on 32-bit and LLP64 systems * Fix example links (#4237) * Fix links md files (#4239) * Add markdown link checker action (#4219) * Match minimum CMake version to 3.18 (#4215)

jhendersonHDF requested review from lrknox, derobins, byrnHDF, fortnern, qkoziol, vchoi-hdfgroup, bmribler, glennsong09, mattjala and brtnfld as code owners March 22, 2024 04:25

jhendersonHDF commented Mar 22, 2024

View reviewed changes

jhendersonHDF force-pushed the vfd_test_fixes branch from 6c1c0d6 to dcace1f Compare March 22, 2024 05:00

Fix 'make check-vfd' target for Autotools

4b440e4

Changes Autotools testing to use HDF5_TEST_DRIVER environment variable to avoid running tests that don't work well with several VFDs Restores old h5_get_vfd_fapl() testing function to setup a FAPL with a particular VFD

jhendersonHDF force-pushed the vfd_test_fixes branch from 5ff7c9c to 4b440e4 Compare March 22, 2024 05:02

byrnHDF approved these changes Mar 22, 2024

View reviewed changes

qkoziol reviewed Mar 22, 2024

View reviewed changes

test/h5test.c Outdated Show resolved Hide resolved

qkoziol reviewed Mar 22, 2024

View reviewed changes

lrknox approved these changes Mar 22, 2024

View reviewed changes

Add macro for default VFD name

0b7a716

jhendersonHDF merged commit 86ca181 into HDFGroup:develop Mar 23, 2024
51 checks passed

jhendersonHDF deleted the vfd_test_fixes branch March 27, 2024 03:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 'make check-vfd' target for Autotools #4211

Fix 'make check-vfd' target for Autotools #4211

jhendersonHDF commented Mar 22, 2024

jhendersonHDF Mar 22, 2024 •

edited

Loading

jhendersonHDF Mar 22, 2024

jhendersonHDF Mar 22, 2024

jhendersonHDF Mar 22, 2024

jhendersonHDF Mar 22, 2024

qkoziol Mar 22, 2024

jhendersonHDF Mar 22, 2024 •

edited

Loading

qkoziol Mar 22, 2024

qkoziol Mar 22, 2024

qkoziol Mar 22, 2024

jhendersonHDF Mar 22, 2024

jhendersonHDF Mar 22, 2024

qkoziol Mar 22, 2024

jhendersonHDF Mar 22, 2024

jhendersonHDF Mar 22, 2024

qkoziol commented Mar 22, 2024

jhendersonHDF commented Mar 23, 2024

qkoziol commented Mar 23, 2024

		@@ -89,7 +89,10 @@ H5FD__supports_swmr_test(const char *vfd_name)

		FUNC_ENTER_NOAPI_NOINIT_NOERR

		if (!vfd_name \|\| !strcmp(vfd_name, "") \|\| !strcmp(vfd_name, "nomatch"))


		assert(H5_DEFAULT_VFD == H5FD_SEC2);

		if ((envval = getenv(HDF5_DRIVER)))

Fix 'make check-vfd' target for Autotools #4211

Fix 'make check-vfd' target for Autotools #4211

Conversation

jhendersonHDF commented Mar 22, 2024

jhendersonHDF Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhendersonHDF Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qkoziol commented Mar 22, 2024

jhendersonHDF commented Mar 23, 2024

qkoziol commented Mar 23, 2024

jhendersonHDF Mar 22, 2024 •

edited

Loading

jhendersonHDF Mar 22, 2024 •

edited

Loading