search

10XGenomics / lz4-rs

Rust LZ4 bindings
MIT License
46 stars 28 forks source link

Link system liblz4 when available #39

Open musicinmybrain opened 1 year ago

musicinmybrain commented 1 year ago

This allows linking an external/system copy of liblz4 (particularly, a system-wide shared library as preferred or required for Linux distribution packaging), in the same manner as bzip2-sys, by imitating:

As noted in https://github.com/alexcrichton/bzip2-rs/pull/58#issuecomment-792684925, some would consider changing the default linking of liblz4 a breaking change.

This PR is offered with the purpose of packaging the lz4-sys and lz4 crates in Fedora Linux, where bundling is discouraged, but allowed under certain circumstances.

This PR would fix https://github.com/10XGenomics/lz4-rs/issues/36.

Based on [feedback in the Fedora Linux package review](), I dropped the part of this PR that would have added a static feature akin to https://github.com/alexcrichton/bzip2-rs/pull/78, but I did so by reverting it rather than force-pushing, so you can still see how it would have worked if you are interested.

mgorny commented 2 months ago

For the record, thanks a lot for doing this. I find it distasteful that someone publishes a *-sys package pretending that actually does not use the system library.

lu-zero commented 2 months ago

Ideally https://docs.rs/system-deps/latest/system_deps/ would make even more streamlined dealing with this kind of dependencies, once this land it can be iterated over.

musicinmybrain commented 2 months ago

Rebased on master.

pmarks commented 2 months ago

Another option is the solution adopted in zstd-rs: a feature to opt-in to pkg-config, otherwise build the local source by default. That's probably my current preference, mainly my use cases prefer this approach.

I definitely don't want the linkage decision to depend on the whether or not a pkg-config query succeeds.

The system_deps solution is interesting: it would change the default to using pkg-config, and it uses an env var to select a internal build. The env var could be put into the config.toml of a project that wants an internal build, but that feels slightly less discoverable, given how most of the other crates work.

musicinmybrain commented 2 months ago

Any of those approaches could work.

If there is an opt-in feature, we would carry a downstream patch in Fedora’s rust-lz4-sys patch to make the pkg-config dependency and query unconditional (because distribution packages always need to link the system libraries). That’s feasible, and we often have to carry that kind of patch in -sys packages.

If pkg-config is the default and an environment variable is used to select an internal lz4, that’s OK for us because the default is what we want. The problem with environment variables, though, is that they have to be set in everything that directly or indirectly depends on the crate, so if the default were using the internal lz4, we would again be patching our rust-lz4-sys package to use pkg-config unconditionally.