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

Fix LOG VOL CI not including Zlib #5181

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Conversation

mattjala
Copy link
Contributor

#5085 changed the default for zlip support to 'off', so the CI should now specifically turn this one. The LOG VOL CI is currently failing two compression tests due to this issue.

@mattjala mattjala added Merge - Develop Only This cannot be merged to previous versions of HDF5 (file format or breaking API changes) Priority - 2. Medium ⏹ It would be nice to have this in the next release Component - Testing Code in test or testpar directories, GitHub workflows Type - Bug / Bugfix Please report security issues to [email protected] instead of creating an issue on GitHub labels Dec 16, 2024
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.

Since the workflow was already explicitly turning off zlib support before, we need to determine why the default being changed to OFF is now causing the LOG VOL CI to fail and fix that issue, rather than enabling zlib support to paper over the issue.

@mattjala
Copy link
Contributor Author

Since the workflow was already explicitly turning off zlib support before, we need to determine why the default being changed to OFF is now causing the LOG VOL CI to fail and fix that issue, rather than enabling zlib support to paper over the issue.

The workflow wasn't explicitly turning off zlib before. It used to include zlib by default like everything else. In the PR that changed the zlib default to off, the "ZLIB=OFF" line was redundantly added to the workflow. That should have been ZLIB=ON.

@jhendersonHDF
Copy link
Collaborator

Since the workflow was already explicitly turning off zlib support before, we need to determine why the default being changed to OFF is now causing the LOG VOL CI to fail and fix that issue, rather than enabling zlib support to paper over the issue.

The workflow wasn't explicitly turning off zlib before. It used to include zlib by default like everything else. In the PR that changed the zlib default to off, the "ZLIB=OFF" line was redundantly added to the workflow. That should have been ZLIB=ON.

I don't believe the line was redundant; more that it was either a copy-paste or wasn't functioning as intended before. Either way, this still seems to point to an issue that needs to be fixed on the LOG VOL's side before re-enabling ZLIB. Otherwise, the issue will go unfixed. @brtnfld do you happen to know anything about this?

@brtnfld
Copy link
Collaborator

brtnfld commented Dec 17, 2024

I'm unaware of how zlib is being tested in the log-vol; I've created an issue HDFGroup/vol-log-based#75 to determine what's happening.

@lrknox lrknox merged commit 7f93e90 into HDFGroup:develop Dec 17, 2024
68 checks passed
@mattjala mattjala deleted the log_vol_zlib branch December 24, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Testing Code in test or testpar directories, GitHub workflows Merge - Develop Only This cannot be merged to previous versions of HDF5 (file format or breaking API changes) 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
Development

Successfully merging this pull request may close these issues.

6 participants