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

Develop libaec fix #5182

Merged
merged 13 commits into from
Jan 10, 2025
Merged

Develop libaec fix #5182

merged 13 commits into from
Jan 10, 2025

Conversation

byrnHDF
Copy link
Contributor

@byrnHDF byrnHDF commented Dec 16, 2024

This change allows pre-installed libaec to be used. It does not work for libaec installs that are not in a common root directory. Those will need further work, most likely adding a libaec/SZIP CMake Find module.

Fixes #4614

@byrnHDF byrnHDF added Priority - 0. Blocker ⛔ This MUST be merged for the release to happen Component - Build CMake, Autotools Type - Bug / Bugfix Please report security issues to [email protected] instead of creating an issue on GitHub labels Dec 16, 2024
@byrnHDF byrnHDF self-assigned this Dec 16, 2024

The CMake logic for finding the libaec compression library has been
modified for a system-installed version of the library. Two options
must be set, HDF5_ALLOW_EXTERNAL_SUPPORT:STRING=NO and *_USE_EXTERNAL:BOOL=OFF.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there is only one *_USE_EXTERNAL:BOOL=OFF option to set? Is it libaec_USE_EXTERNAL:BOOL=OFF, or one or more others?

@@ -295,10 +295,15 @@ IV. Further considerations
-DSZIP_LIBRARY:FILEPATH=some_location/lib/szlib.lib
-DSZIP_INCLUDE_DIR:PATH=some_location/include
-DSZIP_USE_EXTERNAL:BOOL=OFF
(For libaec use -Dlibaec_* options)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe (For libaec use -Dlibaec_* in place of -DSZIP_* for these options) would be clearer, if that is the actual meaning?

Copy link

@vessokolev vessokolev Dec 20, 2024

Choose a reason for hiding this comment

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

That is the actual meaning indeed. The original SZIP package (under that name) is not only carrying code with problematic licence, it is very old as a conception. Currently, the only reliable SZIP compressor and encoder is provided by Libaec. Maybe some sites still support SZIP, but I consider that to be driven by purely anthropological interest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This command "cmake -C ../<hdf5_src_dir>/config/cmake/cacheinit.cmake -G "Unix Makefiles" -DHDF5_ALLOW_EXTERNAL_SUPPORT:STRING="GIT" -DHDF5_ENABLE_ROS3_VFD=ON -DHDF5_ENABLE_ZLIB_SUPPORT=ON -DHDF5_ENABLE_SZIP_SUPPORT=ON -DHDF5_ENABLE_PLUGIN_SUPPORT=ON -DCMAKE_BUILD_TYPE:STRING=Release ../<hdf5_src_dir>" now downloads and builds szip from https://github.com/MathisRosenhauer/libaec.git using the code from byrnHDF:develop-libaec-fix.

lrknox
lrknox previously approved these changes Dec 17, 2024
@byrnHDF
Copy link
Contributor Author

byrnHDF commented Dec 20, 2024

Yes that is true - however as a general rule we try not to force options upon the community.
However I should switch the SZIP to be the "other" choice and make libaec be the example.

Copy link
Collaborator

@lrknox lrknox left a comment

Choose a reason for hiding this comment

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

@lrknox
Copy link
Collaborator

lrknox commented Jan 10, 2025

The remaining problem finding system installed libaec seems to occur when 1.) the include directory (containing libaec.h and szlib.h) and the lib directory (with libaec* and libsz*) are not installed in the same location, or when the lib files don't match the pattern beginning with lib (VCPkg on windows). These installs will most likely require a SZip/libaec CMake Find module (issue #5217) as setting libaec_LIBRARY and libaec_INCLUDE_DIR don't currently work when they don't share the same root directory

Otherwise, testing this PR on macOS 10.13 and 15.1, Ubuntu 24.04 and Windows 11:

  • building libaec with "sudo make install" per the CMake directions in the INSTALL.md file installs the files in /usr/local on the ubuntu, where it is found when -DHDF5_ENABLE_SZIP_SUPPORT=ON without needing any other CMake options.
  • brew install libaec on the older Mac installs it similarly with the same results. I don't have sudo privilege on the newer one.
  • For libaec installs in non-system locations, setting libaec_ROOT is sufficient provided the include and lib directories share that common root directory.
  • The VCPcg install, where the lib files are named aec.lib and szip.lib includes a libaec-config.cmake file with find_library commands for those filenames, but our current HDF5 CMake code doesn't use it.

@lrknox lrknox merged commit 58ae0ae into HDFGroup:develop Jan 10, 2025
76 checks passed
qkoziol pushed a commit to qkoziol/hdf5 that referenced this pull request Jan 13, 2025
* Use the ${LIBAEC_PACKAGE_NAME} variable instead of SZIP
@byrnHDF byrnHDF deleted the develop-libaec-fix branch January 14, 2025 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Build CMake, Autotools Priority - 0. Blocker ⛔ This MUST be merged for the release to happen 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.

Unable to build HDF5 with locally compiled szip or Zlib
4 participants