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

Update VOL CMake for REST VOL #3450

Merged
merged 2 commits into from
Aug 31, 2023
Merged

Update VOL CMake for REST VOL #3450

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf7cd4c
Update VOL CMake for REST VOL
mattjala Aug 29, 2023
2c27728
Prevent linking static libs to VOLs
mattjala Aug 30, 2023
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
26 changes: 25 additions & 1 deletion CMakeVOL.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,31 @@ if (HDF5_VOL_ALLOW_EXTERNAL)
message (FATAL_ERROR "HDF5_ALLOW_EXTERNAL_SUPPORT must be set to 'GIT' to allow building of external HDF5 VOL connectors")
endif ()

set (HDF5_LIB_TARGETS "")
set (HDF5_HL_LIB_TARGETS "")

if (BUILD_STATIC_LIBS)
Copy link
Contributor

@byrnHDF byrnHDF Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If both static and shared are enabled you will have all libraries in the link!
If VOL uses dynamic load function, then you can't use static HDF5 libs, anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, make sure these variables aren't used anywhere else, maybe?

list(APPEND HDF5_LIB_TARGETS ${HDF5_LIB_TARGET})
list(APPEND HDF5_HL_LIB_TARGETS ${HDF5_HL_LIB_TARGET})
endif()

if (BUILD_SHARED_LIBS)
list(APPEND HDF5_LIB_TARGETS ${HDF5_LIBSH_TARGET})
list(APPEND HDF5_HL_LIB_TARGETS ${HDF5_HL_LIBSH_TARGET})
endif()

# For compatibility, set some variables that projects would
# typically look for after calling find_package(HDF5)
set (HDF5_FOUND 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is where these vars should have a prefix like the plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HDF5_FOUND, HDF5_LIBRARIES, and HDF5_INCLUDE_DIRS, are specifically set to match the expected output of find_package(HDF5), which would set these same variables

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is where these vars should have a prefix like the plugins.

This code is specifically removing the find_package(HDF5) call in an external connector's CMake code and then setting the CMake variables that that call would have set if it had been made. Basically replacing the find_package functionality since HDF5 will get built at the time the connector does.

Copy link
Contributor

@byrnHDF byrnHDF Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Right. In plugins and others I do the same thing except with a prefix - of course it requires some control over the other package or we do this with zlib/szip - overwrite the CMakeLists.txt file that gets imported from the repo. See the config/cmake dirs LIBEAC and SZIP.

Copy link
Collaborator

@jhendersonHDF jhendersonHDF Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Here using a prefix would break the builds since we don't have any control over the external packages really and can't reliably predict what their CMake code would look like in order to try overwriting things to use a prefix. find_package(HDF5 ...) is pretty standardized, so we can get away with replacing that, but everything else is up in the air.

set (HDF5_LIBRARIES "${HDF5_LIBSH_TARGET};${LINK_LIBS};${LINK_COMP_LIBS};$<$<BOOL:${HDF5_ENABLE_PARALLEL}>:MPI::MPI_C>")
set (HDF5_LIBRARIES "${HDF5_LIB_TARGETS};${LINK_LIBS};${LINK_COMP_LIBS};$<$<BOOL:${HDF5_ENABLE_PARALLEL}>:MPI::MPI_C>")
set (HDF5_INCLUDE_DIRS "${HDF5_SRC_INCLUDE_DIRS};${HDF5_SRC_BINARY_DIR};$<$<BOOL:${HDF5_ENABLE_PARALLEL}>:${MPI_C_INCLUDE_DIRS}>")
set (HDF5_DIR "${HDF5_SOURCE_DIR}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe HDF5_DIR is supposed to point to an installed HDF5 directory location, so setting it to the source dir for HDF5 might cause issues. Might want to set it to something like ${CMAKE_INSTALL_PREFIX}

Copy link
Contributor

Choose a reason for hiding this comment

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

Do the VOLs actually use this HDF5_DIR variable - because I think it is only informational and shouldn't be used otherwise.

Copy link
Contributor Author

@mattjala mattjala Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It was used in the vol-tests repo/submodule, which is now deprecated. I replaced it in the REST VOL's CMake with a direct reference to HDF5_SOURCE_DIR, and removed this line.

Copy link
Collaborator

Choose a reason for hiding this comment

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

HDF5_SOURCE_DIR comes from the HDF5 project and so isn't likely to be a variable when building the REST VOL, except for possibly in the case of the prebuilt HDF5 notion that we're looking at removing now

Copy link
Contributor Author

@mattjala mattjala Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If the VOL is built as a subdirectory under the HDF5 library, then I think it should inherit HDF5's CMake variables, including this one.

Copy link
Contributor

@byrnHDF byrnHDF Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

But it doesn't exist yet, until packaged? HDF5_DIR is NOT HDF5_SOURCE_DIR. Need to look at the CMake FindHDF5.cmake module file.

Copy link
Contributor

Choose a reason for hiding this comment

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

At least I think so. How does the VOL actually use that variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The VOL uses HDF5_SOURCE_DIR to locate the headers to include when the high level library is enabled (Specifically, to add ${HDF5_SOURCE_DIR}/hl/src as an include directory).

I might be confused - I think HDF5_SOURCE_DIR is automatically created when project(HDF5) is invoked in HDF5's top level CMakeLists.txt, and because we're reworking the VOLs to be built as a CMake subdirectory of the HDF5 project, the CMakeLists.txt for the VOLs will have access to HDF5_SOURCE_DIR.


set (HDF5_C_LIBRARIES "${HDF5_LIB_TARGETS}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, shouldn't this essentially be the same as HDF5_LIBRARIES?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed - this was initially to prevent spurious dependency creation in the REST VOL CMake, but I fixed that on the REST VOL's side.


if (HDF5_BUILD_HL_LIB)
set (HDF5_C_HL_LIBRARIES "${HDF5_HL_LIB_TARGETS}")
endif()

set (HDF5_MAX_EXTERNAL_VOLS 10)
set (HDF5_EXTERNAL_VOL_TARGETS "")
Expand Down Expand Up @@ -134,6 +154,8 @@ if (HDF5_VOL_ALLOW_EXTERNAL)
define_property (
TARGET
PROPERTY HDF5_VOL_TARGETS
BRIEF_DOCS "Generated targets of this connector"
FULL_DOCS "Generated targets of this connector"
)

set_target_properties (
Expand All @@ -149,6 +171,7 @@ if (HDF5_VOL_ALLOW_EXTERNAL)
TARGET
PROPERTY HDF5_VOL_NAME
BRIEF_DOCS "VOL connector name to use for the HDF5_VOL_CONNECTOR environment variable when testing"
FULL_DOCS "VOL connector name to use for the HDF5_VOL_CONNECTOR environment variable when testing"
)

set_target_properties (
Expand All @@ -164,6 +187,7 @@ if (HDF5_VOL_ALLOW_EXTERNAL)
TARGET
PROPERTY HDF5_VOL_TEST_PARALLEL
BRIEF_DOCS "Whether the VOL connector should be tested with the parallel API tests"
FULL_DOCS "Whether the VOL connector should be tested with the parallel API tests"
)

set_target_properties (
Expand Down