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

CMake: (fix) Szip / libaec filter #3035

Merged
merged 2 commits into from
Jun 5, 2023

Conversation

jwsblokland
Copy link
Contributor

  • Fixed the cmake configuration for the filter Szip / libaec. Now, the decoding of this filter is properly configured and included in the HDF5 library.

@hyoklee hyoklee added Branch - 1.14 PRs to hdf5_1_14 Component - Build CMake, Autotools Priority - 0. Blocker ⛔ This MUST be merged for the release to happen Type - Bug Please report security issues to [email protected] instead of creating an issue on GitHub labels Jun 2, 2023
endif ()
if (SZIP_FOUND)
set (H5_HAVE_FILTER_SZIP 1)
set (H5_HAVE_SZLIB_H 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually these first three set routines should be collated into the last if (SZIP_FOUND) block to have one source of these settings.

Copy link
Contributor Author

@jwsblokland jwsblokland Jun 5, 2023

Choose a reason for hiding this comment

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

Yes, that is a good idea. I forced pushed this modification.

Would it be possible to test szip/libaec compression via Github actions on a more structure level? In this case, it was a bit of luck I catch this issue while working on improving the current ROS3 implementation by switching on both the encoding and decoding functionality of th szip/libaec filter.

@derobins derobins added Merge - To 1.12 and removed Branch - 1.14 PRs to hdf5_1_14 labels Jun 3, 2023
- Fixed the cmake configuration for the filter Szip / libaec.
  Now, the decoding of this filter is properly configured and
  included in the HDF5 library.
@byrnHDF
Copy link
Contributor

byrnHDF commented Jun 5, 2023

Looks great!

@byrnHDF
Copy link
Contributor

byrnHDF commented Jun 5, 2023

For consistency - would you mind moving/collating the similar three lines in the zlib code (see lines 94-96)

- Improved the cmake configuration for the filter zlib.
@jwsblokland
Copy link
Contributor Author

For consistency - would you mind moving/collating the similar three lines in the zlib code (see lines 94-96)

Sure no problem. I have pushed the changes. I noticed that lines 113-115 could potentially be moved to the bottom if statement if we can assume that ${ZLIB_INCLUDE_DIRS} is properly defined for that case. Currently, I do not make that assumption.

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 Please report security issues to [email protected] instead of creating an issue on GitHub
Projects
Status: Needs Merged
Development

Successfully merging this pull request may close these issues.

5 participants