Move h5repacktst file generation to h5repackgentest #5564
Conversation
mattjala
added
the
Priority - 2. Medium
mattjala
added
the
Component - Tools
mattjala
requested review from
fortnern,
jhendersonHDF,
qkoziol and
vchoi-hdfgroup
as code owners
mattjala
added
the
Component - Testing
| */ | ||
| int | ||
| make_h5repack_testfiles(void) |
There was a problem hiding this comment.
These ~5,000 lines are just copied from h5repacktst.c and don't need to be closely inspected
|
|
||
| #define MAX_NAME_SIZE 256 | ||
| #define PAGE_SIZE_DEFAULT 4096 |
There was a problem hiding this comment.
h5repacktst originally used FS_PAGESIZE_DEF from h5repack.h. To avoid using h5repack internal symbols, we define a different symbol here with the same value.
| int verify_userblock(const char *filename); | ||
|
|
||
| /* fill value test */ | ||
| #define H5REPACK_FNAME0 "h5repack_fill.h5" |
There was a problem hiding this comment.
These symbols are shared between h5repackgentest.c and h5repacktst.c
|
Marked as draft until I fix build issues on Windows |
mattjala
force-pushed
the
h5repacktst_no_gen
branch
from
da59399 to
22f9b77
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes the generation of h5repacktst files into the h5repackgentest generator, and updates CMake to only build dependent tests when generators are enabled.
- Declare and implement
make_h5repack_testfilesinh5repackgentest.hand call it from the generic test driver - Add filename macros for all repack test cases in
h5repackgentest.h - Guard VFD-based and h5repack tests in CMake on
HDF5_BUILD_GENERATORSand link the new generator library
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/test/h5repack/h5repackgentest.h | Add make_h5repack_testfiles declaration and new test filename macros |
| tools/test/h5repack/CMakeVFDTests.cmake | Wrap VFD tests in an if(HDF5_BUILD_GENERATORS) guard |
| tools/test/h5repack/CMakeLists.txt | Conditionally add and link h5repackgentest under generators flag |
| tools/test/h5gentest.c | Invoke make_h5repack_testfiles() in gen_h5repack_files |
| tools/test/CMakeLists.txt | Link generator tests against ${HDF5_TEST_LIB_TARGET} under generators flag |
Comments suppressed due to low confidence (1)
tools/test/h5repack/h5repackgentest.h:23
- [nitpick] The function name
make_h5repack_testfilesis inconsistent with the existinggenerate_*prefix. Consider renaming it togenerate_h5repack_testfilesfor consistency.
int make_h5repack_testfiles(void);
mattjala
force-pushed
the
h5repacktst_no_gen
branch
from
83db2a9 to
566e70b
Compare
| add_executable (h5repacktest ${REPACK_COMMON_SOURCES} ${HDF5_TOOLS_TEST_H5REPACK_SOURCE_DIR}/h5repacktst.c) | ||
| target_include_directories (h5repacktest | ||
| PRIVATE "${HDF5_TOOLS_SRC_H5REPACK_SOURCE_DIR};${HDF5_TOOLS_ROOT_DIR}/lib;${HDF5_SRC_INCLUDE_DIRS};${HDF5_TEST_SRC_DIR};${HDF5_SRC_BINARY_DIR};$<$<BOOL:${HDF5_ENABLE_PARALLEL}>:${MPI_C_INCLUDE_DIRS}>" | ||
| ) | ||
| if (BUILD_STATIC_LIBS) |
There was a problem hiding this comment.
I think this is an issue where we need to determine the full purpose of HDF5_BUILD_STATIC_TOOLS and align between it and BUILD_STATIC_LIBS. Since this is a regular test program, it should probably be using BUILD_STATIC_LIBS as before rather than HDF5_BUILD_STATIC_TOOLS. But, when it comes time to link against the tools library we need to check HDF5_BUILD_STATIC_TOOLS to link against the correct one (though we should probably solve this higher up and just give lower level CMake code a single target to link against unconditionally).
| TARGET_C_PROPERTIES (h5repacktest STATIC) | ||
| target_link_libraries (h5repacktest PRIVATE ${HDF5_TOOLS_LIB_TARGET} ${HDF5_TEST_LIB_TARGET}) | ||
| target_link_libraries (h5repacktest PRIVATE ${HDF5_TOOLS_LIB_TARGET} ${HDF5_TEST_LIB_TARGET} h5repackgentest) |
There was a problem hiding this comment.
Since the tool gentest object libraries are currently only built when HDF5_BUILD_GENERATORS is ON, this linking step fails when it's OFF. I'm guessing the reason it passes in most of the workflows is because they use the cacheinit file which sets that ON by default. I would generally like to avoid the requirement of HDF5_BUILD_GENERATORS=ON in cases where we don't need it, but this PR seems like it will need that, even for regular non-VOL testing. Maybe we need to decide if the HDF5_BUILD_GENERATORS option is still useful or if we should just always build the generators? Or maybe we should always build the generator libraries but only build the generator binaries when HDF5_BUILD_GENERATORS=ON.
|
It would be nice to remove h5repacktst entirely since it's superseded by the h5repack CMake tests, but the VFD tests for h5repack still use the script. h5repacktst can still be removed, but it will require some substantial modification to the h5repack tests not dissimilar to the changes needed to set up VOL testing. For now, I think we should leave the script in - reworking h5repack VFD testing should be considered separately. Marking this as draft since it depends on #5622 |
This is motivated primarily by the need to be able to generate the h5repacktst files for h5repack VOL tests.
Depends on #5622