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

CMake: (feature) libaec #703

Merged
merged 3 commits into from
Jul 14, 2021
Merged

CMake: (feature) libaec #703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
257e19c
CMake: (feature) libaec
jwsblokland May 28, 2021
9a8dfbc
CMake: (fix) libaec
jwsblokland Jun 15, 2021
2c80177
Docs: Added description about libaec implementation.
jwsblokland Jun 16, 2021
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
18 changes: 15 additions & 3 deletions CMakeFilters.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
# help@hdfgroup.org.
#
option (USE_LIBAEC "Use AEC library as SZip Filter" OFF)
option (USE_LIBAEC_STATIC "Use static AEC library " OFF)

include (ExternalProject)
include (FetchContent)
Expand Down Expand Up @@ -110,13 +111,24 @@ option (HDF5_ENABLE_SZIP_SUPPORT "Use SZip Filter" OFF)
if (HDF5_ENABLE_SZIP_SUPPORT)
option (HDF5_ENABLE_SZIP_ENCODING "Use SZip Encoding" OFF)
if (NOT SZIP_USE_EXTERNAL)
find_package (SZIP NAMES ${SZIP_PACKAGE_NAME}${HDF_PACKAGE_EXT} COMPONENTS static shared)
if (NOT SZIP_FOUND)
find_package (SZIP) # Legacy find
set(SZIP_FOUND FALSE)
if (USE_LIBAEC)
set(libaec_USE_STATIC_LIBS ${USE_LIBAEC_STATIC})
find_package (libaec 1.0.5 CONFIG)
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/MathisRosenhauer/libaec only has a tag to 1.0.4 plus other links are further behind, specifying 1.0.5 will likely fail on most supported platforms.

Copy link

Choose a reason for hiding this comment

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

@byrnHDF: When HDF5's CMake find_package call in config mode is executed, it is not aware of any git tags in libaec. Rather, it will look in libaec's produced CMake version files. Those will include the libaec project version as identified in its main CMakeLists.txt, which for libaec is currently set to 1.0.5.
So, I think line 116 above is the correct statement in this pull request, and I see no basis for assuming it will fail.
It would however make sense to have the git tags in libaec be consistent with the CMake project version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignoring git, for now, this will fail the find_package if the version available on the build machine is not the latest. This will need to be tested on a variety of platforms first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me put things in context.

In the past I tried to use libaec 1.0.4 but with mixed success. My user community has strong desire to make use of static libraries. Unfortunately, using static version of library libaec I was not able to build HDF5 library 1.12.0 with SZIP/libaec support. This is due to the fact how the CMake build system libaec was designed. I decided to make certain modifications to its build system and upstream to the libaec github. All these changes have been accepted by the author/maintainer of libaec. Version 1.0.5 incorporates all these changes including the option to make use of the static library, exposing certain SZIP variables and proper CMake config files such it can be easily integrated in the HDF5 build system. This is the main reason why you find the find_package (libaec 1.0.5 CONFIG) statement, which indicates the minimum required version 1.0.5. For example, version 1.0.4 does not expose these SZIP variables which makes it more difficult to integrate in the your CMake build system.

Having said this all, I agree with you it would be good if libaec reop could have a proper git tag or branch related this 1.0.5 version. I will ask the author/maintainer of libaec library if he could create this git tag/branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the 1.0.5 find package - but if the build does not have the latest version, then we want to use an older version if available. 1.12.0 CMake is very old and before we changed to include libaec. Develop and the dev branches have the latest changes that work with 1.0.4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As requested I made a small update to my implementation. Now there is a fallback option in the case libaec version 1.0.5 has not been found. I have tested it using libaec 1.0.4. Furthermore, everything is based on the develop branch of 15 June 2021). One thing I did notice is the following:

Suppose an user has downloaded libaec 1.0.4 from github (https://github.com/MathisRosenhauer/libaec) and builds a shared library (using Linux) and uses it in the HDF5 build. The build itself runs fine but I noticed that not all symbols will be resolved. This is due to the fact that libsz.so depends on libaec.so. This dependency is not specified anywhere. In version 1.0.5 the libaec build has been changed such that libsz.so is self-contained so there no longer any dependency on an external library. Just something to be aware of.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.
Going forward, we certainly want to 1.0.5+.
As far as 1.0.4, at least we are not worse off.
Maybe you could add a Release.txt note that summarizes the above discussion (including the 1.0.4 warning)

Copy link
Contributor Author

@jwsblokland jwsblokland Jun 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sure no problem to add a Release.txt note. What is the most convenient way for you to add this note? I mean, should I add it to this pull request, put it somewhere in the repository (which location should I use) or add this note to an existing file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Include in this PR. Add to release_docs/RELEASE.txt file. There are two major sections, the first is New Features, the second is Bug Fixes since 1.12.0. Add the note to the second section (starts about line 884) in the sub-section Configuration (about line 1061). The format is simply a one line description, a paragraph or so of text describing the problem and solution, and finally a one line id: (initials - date[, issue #]). (Use GH-### for github issues)

Thanks

Copy link
Contributor Author

@jwsblokland jwsblokland Jun 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

As requested I added a short summary to the file release_docs/RELEASE.txt. Furthermore, Mathis Rosenhauer (author/maintainer of libaec) made version 1.0.5 available.

Copy link
Member

Choose a reason for hiding this comment

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

Why are we hard-coding a specific version of libaec?

Copy link

Choose a reason for hiding this comment

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

@derobins: In CMake, this is the way to indicate a minimum requirement. Requiring a specific version would be done by adding the "EXACT" keyword to the find_package statement. Since the feature being introduced is only available since 1.0.5, this looks like the correct CMake syntax to me.
See the documentation here:
https://cmake.org/cmake/help/latest/command/find_package.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mention by Siebren, this 1.0.5 version is a minimum version requirement. Why this is needed is explain in my response to byrnHDF comment. If this explanation is not clear, please let me know.

if (SZIP_FOUND)
set (LINK_COMP_LIBS ${LINK_COMP_LIBS} ${SZIP_LIBRARIES})
endif ()
endif ()

if (NOT SZIP_FOUND)
find_package (SZIP NAMES ${SZIP_PACKAGE_NAME}${HDF_PACKAGE_EXT} COMPONENTS static shared)
if (NOT SZIP_FOUND)
find_package (SZIP) # Legacy find
if (SZIP_FOUND)
set (LINK_COMP_LIBS ${LINK_COMP_LIBS} ${SZIP_LIBRARIES})
endif ()
endif ()
endif ()
endif ()
if (SZIP_FOUND)
set (H5_HAVE_FILTER_SZIP 1)
Expand Down
15 changes: 15 additions & 0 deletions release_docs/RELEASE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1060,6 +1060,21 @@ Bug Fixes since HDF5-1.12.0 release

Configuration
-------------
- Better support for libaec (open-source Szip library) in CMake

Implemented better support for libaec 1.0.5 (or later) library. This version
of libaec contains improvements for better integration with HDF5. Furthermore,
the variable USE_LIBAEC_STATIC has been introduced to allow to make use of
static version of libaec library. Use libaec_DIR or libaec_ROOT to set
the location in which libaec can be found.

Be aware, the Szip library of libaec 1.0.4 depends on another library within
libaec library. This dependency is not specified in the current CMake
configuration which means that one can not use the static Szip library of
libaec 1.0.4 when building HDF5. This has been resolved in libaec 1.0.5.

(JWSB - 2021/06/22)

- Refactor CMake configure for Fortran

The Fortran configure tests for KINDs reused a single output file that was
Expand Down