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

Link system liblz4 when available #39

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

musicinmybrain
Copy link

@musicinmybrain musicinmybrain commented Nov 15, 2023

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 trifectatechfoundation/bzip2-rs#58 (comment), 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 #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 trifectatechfoundation/bzip2-rs#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
Copy link

mgorny commented Sep 6, 2024

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
Copy link

lu-zero commented Sep 7, 2024

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
Copy link
Author

Rebased on master.

@pmarks
Copy link

pmarks commented Sep 26, 2024

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
Copy link
Author

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.

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.

Link system liblz4?
4 participants