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

Unbundle windows #22

Merged
merged 22 commits into from
May 16, 2022
Merged

Unbundle windows #22

merged 22 commits into from
May 16, 2022

Conversation

zklaus
Copy link

@zklaus zklaus commented May 11, 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.

Fixes #7

@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.

@zklaus
Copy link
Author

zklaus commented May 11, 2022

@conda-forge-admin, please rerender

@hmaarrfk
Copy link
Contributor

Don't you need to prefer external and for good measure remove the bundled sources all together?

@zklaus
Copy link
Author

zklaus commented May 11, 2022

Don't you need to prefer external and for good measure remove the bundled sources all together?

Of course, thanks! I scanned the bld file and saw the PREFER_EXTERNAL lines but didn't read carefully enough to see that it was OFF 🤦.

@zklaus zklaus force-pushed the unbundle-windows branch from 9f50984 to 8097217 Compare May 11, 2022 13:47
@zklaus
Copy link
Author

zklaus commented May 11, 2022

@conda-forge-admin, please rerender

@zklaus
Copy link
Author

zklaus commented May 11, 2022

@conda-forge-admin, please rerender

Klaus Zimmermann and others added 2 commits May 11, 2022 17:01
@zklaus zklaus force-pushed the unbundle-windows branch from 31b7889 to 8fe58b3 Compare May 11, 2022 15:02
@zklaus
Copy link
Author

zklaus commented May 11, 2022

All seems to work except for three compatibility tests. That means that blosc2 fails to decompress the reference files for zlib compressed data that were created with the versions 1.3.0, 1.7.0, and 1.14.0. This may be due to the fact that zlib-ng is compiled in compatibility mode in case of the vendored copy, but not in the conda-forge version. If that is indeed the case, this would probably be an upstream bug.

@hmaarrfk
Copy link
Contributor

I used to have this error, but it is because I had things poorly defined in my builds for external zlib Ng. You can look at my PR upstream recently made for clues on where to debug.

@zklaus
Copy link
Author

zklaus commented May 11, 2022

Hm. Good to know. Do you mean Blosc/c-blosc2#393? Do you have any more pointers?

@hmaarrfk
Copy link
Contributor

Blosc/c-blosc2#358 also

@zklaus zklaus closed this May 13, 2022
@zklaus zklaus reopened this May 13, 2022
@hmaarrfk
Copy link
Contributor

a breakthrough perhaps?

@zklaus
Copy link
Author

zklaus commented May 16, 2022

Yeah, could be. Basically, it seems zlib-ng is aligned much closer with c-blosc than the original zlib in terms of types, i.e. it uses size_t and uint8_t and friends instead of the peculiar uLongf and similar. Now c-blosc, in its wrapper of the zlib functions, comes in with the right types for zlib-ng, but casts them to the original zlib types. For some reason, this fails on windows; I cannot say wether that is a genuine error, possibly because mscv by default uses llp64 instead of the lp64 that is common in Linux (see 64-bit data models), or some compiler quirk.

The difference with llp64 is that long is 32bit, but size_t is 64bit, whereas in lp64 both are 64bit.

In any case, it's probably a good idea to polish the patch and send it upstream.

@zklaus
Copy link
Author

zklaus commented May 16, 2022

@conda-forge-admin, please rerender

@zklaus zklaus marked this pull request as ready for review May 16, 2022 15:34
@zklaus zklaus requested a review from FrancescAlted as a code owner May 16, 2022 15:34
@hmaarrfk hmaarrfk added the automerge Merge the PR when CI passes label May 16, 2022
@hmaarrfk
Copy link
Contributor

Thank you, this is great!

@zklaus
Copy link
Author

zklaus commented May 16, 2022

Thanks, @hmaarrfk! I have also pushed the patch upstream at Blosc/c-blosc2#402.

@github-actions github-actions bot merged commit d8cf7a7 into conda-forge:main May 16, 2022
@github-actions
Copy link
Contributor

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

@hmaarrfk
Copy link
Contributor

Yes i noticed. Thanks for that!

Copy link
Contributor

@FrancescAlted FrancescAlted left a comment

Choose a reason for hiding this comment

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

Looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unbundle libraries
4 participants