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.

Sign up for GitHub

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

Clean up CMake direct VFD handling #4161

Merged
merged 4 commits into from
Mar 19, 2024
+16 −57
Merged

Clean up CMake direct VFD handling #4161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c2577dd
Clean up CMake direct VFD handling
derobins Mar 16, 2024
a380be7
CMake tweaks
derobins Mar 16, 2024
a782cb7
Why is the CMake indent 2?
derobins Mar 16, 2024
b64e82b
CMake docs say to prefer CHECK_SYMBOL_EXISTS
derobins Mar 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 15 additions & 33 deletions config/cmake/ConfigureChecks.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,38 +653,20 @@ unset (CMAKE_EXTRA_INCLUDE_FILES)
#-----------------------------------------------------------------------------

#-----------------------------------------------------------------------------
# Check if Direct I/O driver works
#-----------------------------------------------------------------------------
if (NOT WINDOWS)
option (HDF5_ENABLE_DIRECT_VFD "Build the Direct I/O Virtual File Driver" OFF)
if (HDF5_ENABLE_DIRECT_VFD)
set (msg "Performing TEST_DIRECT_VFD_WORKS")
set (MACRO_CHECK_FUNCTION_DEFINITIONS "-DTEST_DIRECT_VFD_WORKS -D_GNU_SOURCE ${CMAKE_REQUIRED_FLAGS}")
TRY_RUN (TEST_DIRECT_VFD_WORKS_RUN TEST_DIRECT_VFD_WORKS_COMPILE
${CMAKE_BINARY_DIR}
${HDF_RESOURCES_DIR}/HDFTests.c
CMAKE_FLAGS -DCOMPILE_DEFINITIONS:STRING=${MACRO_CHECK_FUNCTION_DEFINITIONS}
OUTPUT_VARIABLE OUTPUT
)
if (TEST_DIRECT_VFD_WORKS_COMPILE)
if (TEST_DIRECT_VFD_WORKS_RUN EQUAL "0")
HDF_FUNCTION_TEST (HAVE_DIRECT)
set (CMAKE_REQUIRED_DEFINITIONS "${CMAKE_REQUIRED_DEFINITIONS} -D_GNU_SOURCE")
add_definitions ("-D_GNU_SOURCE")
else ()
set (TEST_DIRECT_VFD_WORKS "" CACHE INTERNAL ${msg})
message (VERBOSE "${msg}... no")
file (APPEND ${CMAKE_BINARY_DIR}/CMakeFiles/CMakeError.log
"Test TEST_DIRECT_VFD_WORKS Run failed with the following output and exit code:\n ${OUTPUT}\n"
)
endif ()
else ()
set (TEST_DIRECT_VFD_WORKS "" CACHE INTERNAL ${msg})
message (VERBOSE "${msg}... no")
file (APPEND ${CMAKE_BINARY_DIR}/CMakeFiles/CMakeError.log
"Test TEST_DIRECT_VFD_WORKS Compile failed with the following output:\n ${OUTPUT}\n"
)
endif ()
# Check whether we can build the direct VFD
#-----------------------------------------------------------------------------
option (HDF5_ENABLE_DIRECT_VFD "Build the Direct I/O Virtual File Driver" OFF)
if (HDF5_ENABLE_DIRECT_VFD)
# The direct VFD is tied to POSIX direct I/O as enabled by the O_DIRECT
# flag. No other form of direct I/O is supported. This feature also
# requires posix_memalign().
CHECK_SYMBOL_EXISTS (O_DIRECT "fcntl.h" HAVE_O_DIRECT)
CHECK_SYMBOL_EXISTS (posix_memalign "stdlib.h" HAVE_POSIX_MEMALIGN)

if (HAVE_O_DIRECT AND HAVE_POSIX_MEMALIGN)
set (${HDF_PREFIX}_HAVE_DIRECT 1)
else ()
message (FATAL_ERROR "The direct VFD was requested but cannot be built.\nIt requires O_DIRECT flag support and posix_memalign()")
endif ()
endif ()

Expand All @@ -705,7 +687,7 @@ option (HDF5_ENABLE_ROS3_VFD "Build the ROS3 Virtual File Driver" OFF)
endif ()

# ----------------------------------------------------------------------
# Check whether we can build the Mirror VFD
# Check whether we can build the mirror VFD
# ----------------------------------------------------------------------
option (HDF5_ENABLE_MIRROR_VFD "Build the Mirror Virtual File Driver" OFF)
if (HDF5_ENABLE_MIRROR_VFD)
Expand Down
23 changes: 0 additions & 23 deletions config/cmake/HDFTests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,29 +89,6 @@ int main ()

#endif /* DEV_T_IS_SCALAR */

#ifdef TEST_DIRECT_VFD_WORKS

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

int main(void)
{
int fid;

if ((fid = open("tst_file", O_CREAT | O_TRUNC | O_DIRECT, 0755)) < 0)
return 1;
close(fid);
remove("tst_file");

return 0;
}
#endif

#ifdef HAVE_DIRECT
SIMPLE_TEST(posix_memalign());
#endif

#ifdef HAVE_DEFAULT_SOURCE
/* Check default source */
#include <features.h>
Expand Down
2 changes: 1 addition & 1 deletion config/cmake/libhdf5.settings.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ Dimension scales w/ new references: @DIMENSION_SCALES_WITH_NEW_REF@
With deprecated public symbols: @HDF5_ENABLE_DEPRECATED_SYMBOLS@
I/O filters (external): @EXTERNAL_FILTERS@
Map (H5M) API: @H5_HAVE_MAP_API@
Direct VFD: @H5_HAVE_DIRECT@
Direct VFD: @HDF5_ENABLE_DIRECT_VFD@
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though you stop the configure if (HAVE_O_DIRECT AND HAVE_POSIX_MEMALIGN) fails, the more correct is the H5_HAVE_DIRECT setting, instead of the option setting.
Also it would be consistent with the other VFDs.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you want it to say ON/OFF instead of 1/0 (as everything else in this file does), you need to use HDF5_ENABLE_DIRECT_VFD.

Copy link
Member Author

Choose a reason for hiding this comment

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

The other VFDs should be adjusted similarly.

libhdf5.settings is a mess in CMake and clearly handled as an afterthought.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want it to say ON/OFF instead of 1/0 (as everything else in this file does), you need to use HDF5_ENABLE_DIRECT_VFD.

Oh, yes we want yes/no in the file. As long as the option matches the actual setting, then the option is preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

The other VFDs should be adjusted similarly.

libhdf5.settings is a mess in CMake and clearly handled as an afterthought.

It was copied from the autotools version and tried to use similar variables.

Mirror VFD: @H5_HAVE_MIRROR_VFD@
Subfiling VFD: @H5_HAVE_SUBFILING_VFD@
(Read-Only) S3 VFD: @H5_HAVE_ROS3_VFD@
Expand Down
Loading