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

Add SZip support using libaec #179

Merged
merged 6 commits into from
Dec 21, 2022
Merged

Add SZip support using libaec #179

merged 6 commits into from
Dec 21, 2022

Conversation

adamcpovey
Copy link
Contributor

@adamcpovey adamcpovey commented Aug 4, 2022

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Closes #120

I've only tested the Linux build as that's the machine I have access to.


(Edit by @jakirkham): More context on SZip support in libaec can be found in this README.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Aug 4, 2022

Very nice. Only osx cross compilation failing.

recipe/install.bat Outdated Show resolved Hide resolved
@@ -85,6 +85,7 @@ fi
--host="${HOST}" \
--build="${BUILD}" \
--with-zlib="${PREFIX}" \
--with-szlib="${PREFIX}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

You might need to export something to help hdf5 identify the configuration for cross compilation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took me a bit to work out what you meant, but I eventually tracked down which bit of the configure was failing. I can't check it here so let's push and see what happens

Comment on lines -99 to +101
${hdf5_disable_tests}
${hdf5_disable_tests} \
|| (cat config.log; false)
Copy link
Member

Choose a reason for hiding this comment

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

This should help us see any details related to configuration errors (atm ones on macOS, but is generally useful outside of this need)

@jakirkham
Copy link
Member

Added a note in the OP for those interested in knowing how libaec provides SZip support

Hope that is ok 🙂

@jakirkham
Copy link
Member

@conda-forge-admin, please re-render

conda-forge-webservices[bot] and others added 2 commits August 4, 2022 21:01
- configure can't run AC_LANG_PROGRAM to check if SZip encodes when
  cross-compiling so we override that with an export
- Used incorrect library extension for Windows
recipe/install.bat Outdated Show resolved Hide resolved
Co-authored-by: Mark Harfouche <mark.harfouche@gmail.com>
@hmaarrfk
Copy link
Contributor

hmaarrfk commented Aug 7, 2022

Conda is still not detecting libaec being linked to in the windows builds. We need to investigate a little more.

@adamcpovey
Copy link
Contributor Author

Conda is still not detecting libaec being linked to in the windows builds. We need to investigate a little more.

It just occurred to me to check that repo. Doesn't this line mean they're skipping Windows: https://github.com/conda-forge/libaec-feedstock/blob/caeab9823b3bf204a9d443f966cf8e0ff56862c8/recipe/meta.yaml#L20

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Aug 7, 2022

No. That is only for windows and visual studio 2008

@ocefpaf
Copy link
Member

ocefpaf commented Oct 26, 2022

Folks, what is the status of this PR? I was going to tackle this today but found this one open.

@hmaarrfk
Copy link
Contributor

I believe this is still true.

Conda is still not detecting libaec being linked to in the windows builds. We need to investigate a little more.

@ocefpaf
Copy link
Member

ocefpaf commented Oct 26, 2022

I believe this is still true.

Conda is still not detecting libaec being linked to in the windows builds. We need to investigate a little more.

Thanks Mark! Would folks be OK with a not win version of this? We can open an issue an work on it later.

@hmaarrfk
Copy link
Contributor

beyond licensing concerns, which seem to be addressed. I'm not super concerned.

@jakirkham
Copy link
Member

jakirkham commented Oct 26, 2022

Yeah we can always raise an issue about Windows support for libaec

Idk if there is a good test we could add to exercise libaec support. That might be worth doing

Having this improvement (even in a limited form) would be good

@ocefpaf
Copy link
Member

ocefpaf commented Oct 31, 2022

Yeah we can always raise an issue about Windows support for libaec

+1

Idk if there is a good test we could add to exercise libaec support. That might be worth doing

@adamcpovey is there something we can add as a test? Maybe a small compress file and try to read it?

I'm keen to merge this soon. Should we remove the changes on Windows and raise the issue? Or leave them there to help the next person who picks this up?

@jakirkham
Copy link
Member

TBH the test isn't a blocker, but it would just help make sure things continue to work

Perhaps we could repurpose issue ( #120 ) and rescope to Windows only? Though a new issue would work (if preferred)

Would lean towards dropping the Windows changes just to avoid giving the impression this is working. Though no strong feelings if others have preferences

Generally +1 on moving to getting this in ASAP

@adamcpovey
Copy link
Contributor Author

Yeah we can always raise an issue about Windows support for libaec

+1

Idk if there is a good test we could add to exercise libaec support. That might be worth doing

@adamcpovey is there something we can add as a test? Maybe a small compress file and try to read it?

I'm keen to merge this soon. Should we remove the changes on Windows and raise the issue? Or leave them there to help the next person who picks this up?

The computer I do all of this work on is currently unresponsive. Once it works again, I'll try to put together a snippet of code that makes a small compressed file then opens it.

@hmaarrfk
Copy link
Contributor

@ocefpaf did you want to merge this?

@ocefpaf
Copy link
Member

ocefpaf commented Dec 21, 2022

@ocefpaf did you want to merge this?

I do! Sorry I forgot about this one. We are missing Windows support right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add szip support
5 participants