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

Use only one find_package call for zlib. #5280

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

Conversation

byrnHDF
Copy link
Contributor

@byrnHDF byrnHDF commented Jan 27, 2025

Addresses #5155, and related #4614 #4904

zlib system installs tend to not have CMake config files and therefore require that a plain vanilla find_package call be used. This uses the find module for zlib. For more control over what gets found, an option to use config mode of find package is added. Which allows the use of static libs and better target properties. Also native zlib-ng, which usually has config files, would be better served with the config mode.

@byrnHDF byrnHDF added Priority - 2. Medium ⏹ It would be nice to have this in the next release Component - Build CMake, Autotools Type - Improvement Improvements that don't add a new feature or functionality labels Jan 27, 2025
@byrnHDF byrnHDF self-assigned this Jan 27, 2025
bmribler
bmribler previously approved these changes Jan 28, 2025
brtnfld
brtnfld previously approved these changes Jan 28, 2025
@byrnHDF byrnHDF dismissed stale reviews from brtnfld and bmribler via f54d538 January 29, 2025 14:40
CMakeFilters.cmake Outdated Show resolved Hide resolved
release_docs/RELEASE.txt Outdated Show resolved Hide resolved
lrknox
lrknox previously approved these changes Jan 29, 2025
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.

2 code suggestions - indentation and missing "be". Currently testing cmake configure commands; ok so far.

CMakeFilters.cmake Outdated Show resolved Hide resolved
@lrknox
Copy link
Collaborator

lrknox commented Jan 29, 2025

Tested minimal cmake configure commands, e.g. cmake -G "Unix Makefiles" -DHDF5_ENABLE_ZLIB_SUPPORT=ON ../hdf5, on jelly, ec2-ubuntu24.04, and macmini-m1. In a spack environment and outside the spack environment on jelly and macmini-m1. In the spack environment the previous zlib build in spack is added; outside it picks up the one beneath /usr.

On Windows 11 I tested with a VCPkg installed zlib, szip(libaec), and curl and ssl for ros3. I added CMake ROOT variables for these since they were not system installs. I ran builds and tests with no errors/failures on all the machines.

lrknox
lrknox previously approved these changes Jan 29, 2025
jhendersonHDF
jhendersonHDF previously approved these changes Jan 29, 2025
Copy link
Collaborator

@jhendersonHDF jhendersonHDF left a comment

Choose a reason for hiding this comment

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

Approved for develop, but we should discuss whether this is needed in 1.14.6

@brtnfld
Copy link
Collaborator

brtnfld commented Jan 29, 2025

What does this get us to put into develop? Develop is not a production branch, so why is there an urgency? We have time before the 2.0 release, so why not resolve this with other interested parties if needed? We should not add a new option for 1.14 if we are not convinced it is the correct solution. We also have the option of not fixing this in 1.14.6, as this needs a targeted long-term fix plan that can involve Kitware and other parties.

lrknox
lrknox previously approved these changes Jan 29, 2025
@brtnfld
Copy link
Collaborator

brtnfld commented Jan 29, 2025

Also, nearly zero comments in the code explain the logic of this fix.

@byrnHDF
Copy link
Contributor Author

byrnHDF commented Jan 29, 2025

Also, nearly zero comments in the code explain the logic of this fix.

Because it is in the issues?
But I can update it.

@byrnHDF byrnHDF dismissed stale reviews from lrknox and jhendersonHDF via 0c01767 January 30, 2025 21:33
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 - Improvement Improvements that don't add a new feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants