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.

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

Fix CMake build detection of libaec #5010

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

cho-m
Copy link
Contributor

@cho-m cho-m commented Oct 25, 2024

Partial revert of #4567 as libaec is the correct name of the package and should be searched first.

Also fixes pkg-config generation handling. This does mean the output CMake configuration files need to find_package(libaec) prior to hdf5-static, but it also avoids using absolute path to shared library so that users can do set(libaec_USE_STATIC_LIBS ON).


libaec package installs libaec-config.cmake.

CMake does not provide a FindSZIP.cmake module, internal copy was removed (2c3797d), and older SZIP didn't include CMake config files. So, could consider dropping older find_package(SZIP ...) as the only valid one is now find_package(libaec ...).

Resulting hdf5.pc has:

Libs.private:   -lz -lsz

Resulting hdf5-targets.cmake has:

set_target_properties(hdf5-static PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include;${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "\$<LINK_ONLY:m>;\$<LINK_ONLY:dl>;\$<LINK_ONLY:ZLIB::ZLIB>;\$<LINK_ONLY:libaec::sz>;\$<\$<PLATFORM_ID:Windows>:>;\$<\$<NOT:\$<PLATFORM_ID:Windows>>:>;\$<\$<BOOL:OFF>:MPI::MPI_C>;\$<LINK_ONLY:\$<\$<OR:\$<BOOL:OFF>,\$<BOOL:OFF>>:Threads::Threads>>"
)

Related to #4949

Note that Debian does not include libaec CMake files so they would need a different fix (though maybe better for them to support CMake files).

@@ -160,15 +160,27 @@ 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)
Copy link
Contributor

@byrnHDF byrnHDF Oct 25, 2024

Choose a reason for hiding this comment

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

original szip could still be used with previous but probably need to make sure that LIBAEC_PACKAGE_NAME matches libaec otherwise might be a mismatch if LIBAEC_PACKAGE_NAME is used later and doesn't match

Copy link
Contributor Author

@cho-m cho-m Oct 25, 2024

Choose a reason for hiding this comment

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

Mismatching names are not recommended, which is why CMake will output warning https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/FindPackageHandleStandardArgs.cmake#L441-446

This also breaks libaec detection due to https://gitlab.dkrz.de/k202009/libaec/-/blob/master/cmake/libaec-config.cmake.in?ref_type=heads#L49 (also COMPONENTS are not defined in libaec).


Original szip should still be supported by fallbacks. As long as you don't add REQUIRED (and there is no conflicting libaec somewhere) it should always work.

If you don't want output warnings, then the recommendation is usually running with QUIET and only outputting warning for last attempt.

If there is equal preference, then a dedicated CMake option would be better to control priority

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})
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

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

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

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.

Copy link
Contributor

@byrnHDF byrnHDF left a comment

Choose a reason for hiding this comment

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

Hardcoded names should not be used

@cho-m
Copy link
Contributor Author

cho-m commented Oct 25, 2024

Hardcoded names should not be used

Root causing further, the change in #4567 and 4fa004e did something worse than "hardcoding". It relied on unofficial / build-system-specific (vcpkg) patch.

It introduced regression on official builds of libaec and szip. Even builds with original SZIP are broken without vcpkg patch (https://github.com/microsoft/vcpkg/blob/master/ports/hdf5/szip.patch) as linker flags are missing

EDIT 1: Further regressions in libaec are from expecting the same find_package behavior. There are no COMPONENTS defined in libaec.

EDIT 2: Given various breakages, would recommend adding a CMake build test with system libaec on Linux CI, but the limitation here is lack of libaec CMake files on Ubuntu. You would need to set up another CI runner on Fedora, Arch Linux, Alpine, or something else.


So, yes, relying on Vcpkg's "hardcoded" workaround should be avoided. Overriding SZIP_LIBRARIES (and thus causing confusion on intended output) should be avoided.

Also using SZIP_LIBRARIES as a target when it is not guaranteed to be so is also a bad idea.

Instead, the correct targets should be used maybe like

if (TARGET libaec::sz)
  set(SZIP_TARGET libaec::sz)
elseif (TARGET szip-shared)
  set(SZIP_TARGET szip-shared)
elseif (TARGET szip-static)
  set(SZIP_TARGET szip-static)
endif()

Or

set (SZIP_TARGET "szip-${LIBAEC_SEACH_TYPE}")
if (TARGET libaec::sz)
  set (SZIP_TARGET libaec::sz)
endif ()

Since the logic has to plug into the pkgconfig handler which needs CMake targets (SZIP_LIBRARIES is not one):

hdf5/src/CMakeLists.txt

Lines 1225 to 1229 in ee61dd5

foreach (libs ${LINK_COMP_LIBS})
# set (_PKG_CONFIG_REQUIRES_PRIVATE "${_PKG_CONFIG_REQUIRES_PRIVATE} -l${libs}")
get_target_property (libname ${libs} OUTPUT_NAME)
set (_PKG_CONFIG_LIBS_PRIVATE "${_PKG_CONFIG_LIBS_PRIVATE} -l${libname}")
endforeach ()

Otherwise you get:

Libs.private:   -lz -llibname-NOTFOUND

Partial revert of HDFGroup#4567 as libaec
is the correct name of latest package and should be searched first.
Otherwise, it is not found even if `LIBAEC_PACKAGE_NAME=libaec`.

Also fixes pkg-config generation handling. This does mean the output
CMake configuration files need to `find_package(libaec)` prior to
`hdf5-static`, but it also avoids using absolute path to shared library
so that users can do `set(libaec_USE_STATIC_LIBS ON)`.

Also fix build with original SZIP without vcpkg patch.
@cho-m cho-m force-pushed the fix-libaec-detection branch from b31c318 to 8089a2d Compare October 25, 2024 22:52
@byrnHDF
Copy link
Contributor

byrnHDF commented Oct 29, 2024

I will be investigating all the issues with the zlib and szip functionality in the HDF5 CMake code. There are a variety of ways to build hdf5 with zlib/szip support and it's time to re-evaluate that code before the end of the year.

@derobins derobins added Priority - 2. Medium ⏹ It would be nice to have this in the next release Component - Build CMake, Autotools Type - Bug / Bugfix Please report security issues to [email protected] instead of creating an issue on GitHub labels Nov 4, 2024
@derobins derobins added this to the 2.0.0 milestone Nov 4, 2024
@byrnHDF
Copy link
Contributor

byrnHDF commented Dec 16, 2024

Please check #5182 - I think it should solve the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Build CMake, Autotools Priority - 2. Medium ⏹ It would be nice to have this in the next release Type - Bug / Bugfix Please report security issues to [email protected] instead of creating an issue on GitHub
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants