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

Rework the compression library search #5085

Open
wants to merge 33 commits into
base: develop
Choose a base branch
from
Open

Rework the compression library search #5085

wants to merge 33 commits into from

Conversation

byrnHDF
Copy link
Contributor

@byrnHDF byrnHDF commented Nov 8, 2024

Added default names for search use.

@byrnHDF byrnHDF added Priority - 2. Medium ⏹ It would be nice to have this in the next release Component - C Library Core C library issues (usually in the src directory) Type - Bug / Bugfix Please report security issues to [email protected] instead of creating an issue on GitHub labels Nov 8, 2024
@byrnHDF byrnHDF self-assigned this Nov 8, 2024
@byrnHDF
Copy link
Contributor Author

byrnHDF commented Nov 20, 2024

The search path for libs can be controlled by CMake system variables - see https://cmake.org/cmake/help/latest/command/find_package.html

jhendersonHDF
jhendersonHDF reviewed Nov 21, 2024
View reviewed changes
CMakeFilters.cmake Outdated Show resolved Hide resolved
lrknox
lrknox reviewed Nov 21, 2024
View reviewed changes
CMakeFilters.cmake Outdated Show resolved Hide resolved
lrknox
lrknox previously approved these changes Nov 21, 2024
View reviewed changes
lrknox
lrknox previously approved these changes Nov 22, 2024
View reviewed changes
@lrknox
Copy link
Collaborator

lrknox commented Nov 25, 2024

This command successfully downloads and configures zlib szip and hdf5_plugins from their GitHub repos along with HDF5 develop:
cmake -C ../hdf5-2.0.0/config/cmake/cacheinit.cmake -G "Unix Makefiles" -DHDF5_ALLOW_EXTERNAL_SUPPORT:STRING="GIT" -DZLIB_USE_LOCALCONTENT=OFF -DLIBAEC_USE_LOCALCONTENT=OFF -DHDF5_ENABLE_ROS3_VFD=ON -DHDF5_ENABLE_ZLIB_SUPPORT=ON -DHDF5_ENABLE_SZIP_SUPPORT=ON -DHDF5_ENABLE_PLUGIN_SUPPORT=ON -DCMAKE_BUILD_TYPE:STRING=Debug --log-level=VERBOSE ../hdf5-2.0.0

This one fails to download hdf5_plugins.tar.gz. Zlib and szip tar.gz files are downloaded:
cmake -C ../hdf5-2.0.0/config/cmake/cacheinit.cmake -G "Unix Makefiles" -DHDF5_ALLOW_EXTERNAL_SUPPORT:STRING="TGZ" -DZLIB_USE_LOCALCONTENT=OFF -DPLUGIN_USE_LOCALCONTENT=OFF -DLIBAEC_USE_LOCALCONTENT=OFF -DHDF5_ENABLE_ROS3_VFD=ON -DHDF5_ENABLE_ZLIB_SUPPORT=ON -DHDF5_ENABLE_SZIP_SUPPORT=ON -DHDF5_ENABLE_PLUGIN_SUPPORT=ON -DCMAKE_BUILD_TYPE:STRING=Debug --log-level=VERBOSE ../hdf5-2.0.0

This works: wget https://github.com/HDFGroup/hdf5_plugins/releases/download/snapshot/hdf5_plugins-master.tar.gz
This fails: https://github.com/HDFGroup/hdf5_plugins/releases/download/snapshots/hdf5_plugins-master.tar.gz

It appears the CMakePresets.json is correct, but cacheinit.cmake and INSTALL_CMake.txt need the final 's' removed from 'snapshots':
[lrknox@jelly hdf5-2.0.0]$ grep -r hdf5_plugins * | grep download
CMakePresets.json: "PLUGIN_TGZ_ORIGPATH": {"type": "STRING", "value": "https://github.com/HDFGroup/hdf5_plugins/releases/download/snapshot"},
config/cmake/cacheinit.cmake:set (PLUGIN_TGZ_ORIGPATH "https://github.com/HDFGroup/hdf5_plugins/releases/download/snapshots" CACHE STRING "Use PLUGINS from original location" FORCE)
release_docs/INSTALL_CMake.txt: set (PLUGIN_TGZ_ORIGPATH "https://github.com/HDFGroup/hdf5_plugins/releases/download/snapshots" CACHE STRING "Use PLUGINS from original location" FORCE)

jhendersonHDF
jhendersonHDF reviewed Nov 26, 2024
View reviewed changes
release_docs/INSTALL_CMake.txt
@@ -68,7 +68,7 @@ Download from https://github.com/HDFGroup/hdf5/blob/develop/config/cmake/scripts
External compression plugin libraries from https://github.com/HDFGroup/hdf5_plugins:
hdf5_plugins.tar.gz

External compression szip and zlib libraries:
External compression szip and zlib libraries:0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Accidental?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most likely yes.

jhendersonHDF
jhendersonHDF reviewed Nov 26, 2024
View reviewed changes
release_docs/RELEASE.txt Outdated
@@ -50,7 +50,7 @@ New Features
- Renamed the option: HDF5_ENABLE_Z_LIB_SUPPORT

The option has been renamed to HDF5_ENABLE_ZLIB_SUPPORT to be consistent
with the naming of other options.
with the naming of other options. Also, the option defaults to OFF.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely want to make sure this is very well called out for the next release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely

lrknox
lrknox previously approved these changes Dec 2, 2024
View reviewed changes
mattjala
mattjala reviewed Dec 2, 2024
View reviewed changes
release_docs/NEWSLETTER.txt
@@ -3,7 +3,13 @@ HDF5 version 2.0.0 currently under development
Features included for the next major release:
----------------------------------------------------------------------------

*
************ Renamed the option: HDF5_ENABLE_Z_LIB_SUPPORT ************
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we break this into two items, with one indicating the rename and one indicating that it now defaults to off? We'd want someone skimming the headlines to catch both of these

mattjala
mattjala reviewed Dec 2, 2024
View reviewed changes
release_docs/RELEASE.txt Outdated

The option has been renamed to HDF5_ENABLE_ZLIB_SUPPORT to be consistent
with the naming of other options.
*** Also, the option defaults to OFF. This requires the user to explicitly ***
Copy link
Contributor

Choose a reason for hiding this comment

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

Same idea about breaking this into two items

@brtnfld
Copy link
Contributor

brtnfld commented Dec 2, 2024

I suggest we transition from using txt files to markdown, enabling us to highlight important information more effectively by the use of:

https://github.blog/changelog/2023-12-14-new-markdown-extension-alerts-provide-distinctive-styling-for-significant-content/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattjala mattjala mattjala left review comments

@jhendersonHDF jhendersonHDF jhendersonHDF left review comments

@lrknox lrknox lrknox left review comments

@derobins derobins Awaiting requested review from derobins derobins is a code owner

@fortnern fortnern Awaiting requested review from fortnern fortnern is a code owner

@qkoziol qkoziol Awaiting requested review from qkoziol qkoziol is a code owner

@vchoi-hdfgroup vchoi-hdfgroup Awaiting requested review from vchoi-hdfgroup vchoi-hdfgroup is a code owner

@bmribler bmribler Awaiting requested review from bmribler bmribler is a code owner

@glennsong09 glennsong09 Awaiting requested review from glennsong09 glennsong09 is a code owner

@brtnfld brtnfld Awaiting requested review from brtnfld brtnfld is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

@byrnHDF byrnHDF

Labels
Component - C Library Core C library issues (usually in the src directory) 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@byrnHDF @lrknox @brtnfld @jhendersonHDF @mattjala