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

Jump to bottom

Fix CMake build detection of libaec #5010

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
+16 −3
Open

Fix CMake build detection of libaec #5010

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 16 additions & 3 deletions CMakeFilters.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,15 +160,28 @@ if (HDF5_ENABLE_SZIP_SUPPORT)
endif ()
set(libaec_USE_STATIC_LIBS ${HDF5_USE_LIBAEC_STATIC})
set(SZIP_FOUND FALSE)
find_package (SZIP NAMES ${LIBAEC_PACKAGE_NAME}${HDF_PACKAGE_EXT} COMPONENTS ${LIBAEC_SEACH_TYPE})
find_package (libaec CONFIG QUIET)
if (NOT SZIP_FOUND)
find_package (SZIP) # Legacy find
find_package (SZIP NAMES ${LIBAEC_PACKAGE_NAME}${HDF_PACKAGE_EXT} COMPONENTS ${LIBAEC_SEACH_TYPE})
if (NOT SZIP_FOUND)
find_package (SZIP) # Legacy find
endif ()
endif ()
set(H5_SZIP_FOUND ${SZIP_FOUND})
if (H5_SZIP_FOUND)
set (H5_SZIP_INCLUDE_DIR_GEN ${SZIP_INCLUDE_DIR})
set (H5_SZIP_INCLUDE_DIRS ${H5_SZIP_INCLUDE_DIRS} ${SZIP_INCLUDE_DIR})
set (LINK_COMP_LIBS ${LINK_COMP_LIBS} ${SZIP_LIBRARIES})
set (SZIP_TARGET "szip-${LIBAEC_SEACH_TYPE}")
if (TARGET libaec::sz)
Copy link
Contributor

Choose a reason for hiding this comment

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

not always sz (like on windows)

Copy link
Contributor

Choose a reason for hiding this comment

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

Should avoid using hardcodded names - always if possible

Copy link
Contributor Author

@cho-m cho-m Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What CMake file is outputting something different on Windows? Can you provide reference for this?

The target names are created via libaec-config.cmake https://gitlab.dkrz.de/k202009/libaec/-/blob/master/cmake/libaec-config.cmake.in?ref_type=heads#L25.

If target doesn't exist, it should fall back on prior logic for else() case.

This is only way to get both static and shared support. If you use SZIP_LIBRARIES, it will hardcode the specific library found .a, .lib, .dll, .dylib, or .so

With prior logic, the resulting HDF5 cmake config file (if built with shared libaec) will cause:

set(libaec_USE_STATIC_LIBS ON)
find_package(ZLIB REQURIED)
find_package(libaec CONFIG REQURIED)
find_package(hdf5 CONFIG REQURIED)
add_executable(test test.cpp)
target_link_libraries(test hdf5-static)

to run (c|clang|g)++ test.cpp ... /path/to/libhdf5.a /path/to/libsz.{so,dylib}.

Whereas most users would expect: (c|clang|g)++ test.cpp ... /path/to/libhdf5.a /path/to/libsz.a.

Copy link
Contributor

Choose a reason for hiding this comment

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

hdf5 only needs static compression libs

Copy link
Contributor Author

@cho-m cho-m Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

hdf5 only needs static compression libs

But most external distribution require shared libraries and do not allow bundling libraries, e.g.

The default externally will always be shared. If static libraries are provided, then ideally should work as users expect.

Otherwise, most repositories/distros will just drop support for SZIP in HDF5. If that is the recommendation, can just go this route and close PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

hdf5 doesn't expose the API for compression, the reason for shared - static lib is only provided for linking the static hdf5. So if there is no static hdf5, then there is not a need for providing a static compression lib.
Also, the compression libraries do not require inline build with hdf5 - they be can be built externally and just use the proper include and lib paths to pull them into hdf5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway, without modification, existing logic is broken with libaec and will result in

CMake Error at src/CMakeLists.txt:1204 (get_target_property):
  get_target_property() called with non-existent target

Copy link
Contributor Author

@cho-m cho-m Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So if there is no static hdf5, then there is not a need for providing a static compression lib.

In Homebrew's case, we have both. We distribute a shared and static HDF5 from single build. We always default to shared as main library.

User expectation here is that each would behave as expected, depending on if they use hdf5-shared or hdf5-static target (or FindHDF5.cmake - HDF5_USE_STATIC_LIBRARIES=[ON|OFF])

Also, the compression libraries do not require inline build with hdf5 - they be can be built externally and just use the proper include and lib paths to pull them into hdf5

That is what we've done in Homebrew until 1.14.5 introduced a build regression. We have not updated to HDF5 1.14.5 yet due to these issues.

Issue was also reported by FreeBSD. It will impact pretty much all repositories/distros that use CMake build. Linux distros are pretty much all using autotools so won't be impacted until v2.

set (SZIP_TARGET libaec::sz)
endif ()
if (TARGET ${SZIP_TARGET})
get_target_property (libname ${SZIP_TARGET} LOCATION)
get_filename_component (libname ${libname} NAME_WE)
string (REGEX REPLACE "^lib" "" libname ${libname})
set_target_properties (${SZIP_TARGET} PROPERTIES OUTPUT_NAME ${libname})
set (LINK_COMP_LIBS ${LINK_COMP_LIBS} ${SZIP_TARGET})
endif ()
endif ()
else ()
if (HDF5_ALLOW_EXTERNAL_SUPPORT MATCHES "GIT" OR HDF5_ALLOW_EXTERNAL_SUPPORT MATCHES "TGZ")
Expand Down
Loading