conda-forge / zstandard-feedstock

A conda-smithy repository for zstandard.
BSD 3-Clause "New" or "Revised" License
0 stars 12 forks source link

Strictly depend on zstd #48

Closed chrisburr closed 1 year ago

chrisburr commented 1 year ago

The zstandard Python bindings depend on private APIs in zstd and refuse to load if libzstd.so mismatches:

https://github.com/indygreg/python-zstandard/blob/0063333790a853360c816101511635865405834f/c-ext/backend_c.c#L137-L150

Currently using zstandard fails to import with with:

[1] import zstandard

ImportError: zstd C API versions mismatch; Python bindings were not compiled/linked against expected zstd version (10505 returned by the lib, 10502 hardcoded in zstd headers, 10502 hardcoded in the cext)

This PR changes the build to depend on the same zstd version as was used for building.

conda-forge-webservices[bot] commented 1 year ago

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.

rmax commented 1 year ago

Thanks!

jaimergp commented 1 year ago

We might need to issue repodata patches for the previous builds too (by inspecting the rendered recipe in each package?) because previously published packages might still get selected with the wrong zstd (see https://github.com/conda/conda-libmamba-solver/issues/241).

jaimergp commented 1 year ago

Added https://github.com/conda-forge/conda-forge-repodata-patches-feedstock/pull/484 if you can take a look @conda-forge/zstd @chrisburr

hmaarrfk commented 1 year ago

this is somewhat troubling since now the migrator won't work. for this feedstock and you will need to manually maintain this.

can you link statically instead?

jaimergp commented 1 year ago

Looks like this feedstock was statically linking before, but then it got changed in #45.

The problem with this feedstock is that the zstd version is hardcoded in the bindings, so we can't just use arbitrary version. Actually, we don't want the migrators here at all, so it might be the right call?

hmaarrfk commented 1 year ago

No this is still problematic we should revert to static linking and document why it is done.

For example the current scenerio would break things:

  1. This package builds with version 1.2.3, thus pinning zstd to 1.2.3
  2. A new version of zstd come out, call it 1.2.4
  3. An other package gets built, and pulls in 1.2.4, pinning to >=1.2.4

Package 1 and package 3 are incompatible, but they really should be compatible.

jaimergp commented 1 year ago

Oops, you are right, yes. @dbast @isuruf, given your involvement with #43, does this sound ok to you? The plan is to simply list the license and statically link like we were doing before #45.

hmaarrfk commented 1 year ago

The other option is just to work with standard to understand where we need to patch out and just rebuild for every zstd version we find.

isuruf commented 1 year ago

zstandard usually makes a new release once there is a new zstd release. For example zstd 1.5.5 came out on April 4 and zstandard 0.21.0 was released on April 16. So, it's a matter of keeping this feedstock up-to-date.