search

rust-lang / git2-rs

libgit2 bindings for Rust
https://docs.rs/git2
Apache License 2.0
1.7k stars 388 forks source link

Allow overriding of the libgit2 pkgconfig at build time #1065

Open ReillyBrogan opened 3 months ago

ReillyBrogan commented 3 months ago

It is often desirable to Linux distributions to de-bundle packages as much as possible so that system versions of libraries are used instead of statically compiled versions. This is problematic with libgit2-sys as the supported version range is so narrow (also a problem for most libgit2 bindings). A common solution to this is to have co-installable versions of libgit2 where the pkgconfig file is renamed (perhaps to something like libgit-1.7 or similar). Support this in libgit2-sys so that distributions can override this at build time for arbitrary applications that may use the libgit2 bindings.

ReillyBrogan commented 2 months ago

@ehuss could we get this merged in before the next release? It shouldn't regress anything and it's entirely opt-in behavior. Renaming pkg-config files is also fairly standard when it comes to making different versions of a library co-installable.

weihanglo commented 2 months ago

Hi @ReillyBrogan. Thank you for the contribution. I have a mixed feeling with this change. While it does fix the issue, it is a bit uncomfortable that we are able to set an ad-hoc environment variable that affects your dependency randomly.

Do you have more references or examples about this kind of pkg-config lib name override? How do other Rust *-sys libraries address this? Any other libgit2 binding also use this workaround?

ReillyBrogan commented 2 months ago

that affects your dependency randomly

Keep in mind that if this searches for libgit2-1.18 for example that whatever pkgconfig file it finds will still need to pass the version check and if it's not actually libgit2 the build will fail when it tries to link against it.

I don't think it's realistic for a user to randomly set this environmental variable, happen to have a pkg-config with a version string somewhere in the supported version range, and also have it link successfully. The only people who are going to be setting this are going to be knowing what they are doing, or it just won't work at all.

How do other Rust *-sys libraries address this

They don't need to. This situation is fairly unique to libgit2 as it is probably the most widely used library on Linux that sees regular soname changes and has semi-frequent CVEs. Sqlite3 and zstd bindings, for comparison, haven't seen a single soname bump during the entire time that Rust has existed as a language. If there was a comparable situation, the same kind of solution would probably be the best approach.

Any other libgit2 binding also use this workaround?

No, because usually they are built with cmake or meson which have workarounds allowing for overriding a given pkgconfig search, or the search itself is usually in a build file in the source code that can be easily patched. Cargo projects are a different case because the libraries are usually downloaded at build-time (IE, it's non-trivial to patch them effectively) and the pkg_config crate only allows for overriding the pkg-config search path entirely (which doesn't help you if the pkg-config files are in the same directory).

I've been gradually converting packages on the Linux distribution I work on to use system libs as much as possible and as far as I can tell it's generally accepted practice to use environmental variables to override things in dependency builds. libgit2-sys already supports LIBGIT2_NO_VENDOR to force it to try to use the system libgit2 or fail if it can't, this new environmental variable just extends that by allowing it to use another libgit2 install if necessary.

weihanglo commented 2 months ago

Could you help document the environment variable somewhere in the root README.md or lib.rs of libgit2-sys? (I think I have missed documenting LIBGIT2_NO_VENDOR 😅)