-
-
Notifications
You must be signed in to change notification settings - Fork 244
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
CMake: (feature) libaec #703
Conversation
find_package (SZIP) # Legacy find | ||
if (USE_LIBAEC) | ||
set(libaec_USE_STATIC_LIBS ${USE_LIBAEC_STATIC}) | ||
find_package (libaec 1.0.5 CONFIG) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
find_package (SZIP) # Legacy find | ||
if (USE_LIBAEC) | ||
set(libaec_USE_STATIC_LIBS ${USE_LIBAEC_STATIC}) | ||
find_package (libaec 1.0.5 CONFIG) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Implemented better support for the compression library libaec. Especially, for the new version 1.0.5: - The CMake config files of this version 1.0.5 now set certain SZIP variables which can be used in the existing CMake structure. - Both static and shared SZIP compatible libaec library contains all required objects such it can be easily used when building HDF5 from scratch. - Introduced the USE_LIBAEC_STATIC option to make use of the static version of SZIP compatible libaec library.
Implemented the fallback option for the case if version 1.0.5 of libaec could not be found.
2fb2872
to
9a8dfbc
Compare
Added short description about the libaec improvement to the Bug Fixes section in the Configuration sub-section.
This PR appears to be waiting for reviewers to green light it, but I've not seen any responses in over two weeks. What is the current status of this PR? |
Implemented better support for the compression library libaec.
Especially, for the new version 1.0.5:
SZIP variables which can be used in the existing
CMake structure.
all required objects such it can be easily used when building
HDF5 from scratch.
version of SZIP compatible libaec library.