Respect libgit2's ABI instability

[saethlin]

Notably this PR does not close the mentioned issues. The problem there is that old versions of this library become broken when the system version of the library moves on. All I can suggest is that maybe old versions of this library might be better yanked or with this fix backported.

libgit2 does not have a stable ABI across minor versions, as has been
demonstrated in the last few minor releases, see #813 and #746. This
pain is primarily suffered by users of rolling release distros, because
they tend to get a new libgit2 version before the authors of libraries
release versions that pull in the new libgit2-sys version which works
with the new binary. (see also rust-lang/crater#598)

This patch means that going forward, users don't need to rush to
upgrade their version of libgit2-sys or suffer errors/UB in the
meantime. If the system version changes, they will just start using
the vendored version which has been tested against the bindings.

[ehuss]

It's not clear to me how this will entirely fix the problem. If a Rust tool is using the system libgit2 1.4, and then the system updates to an ABI breaking 1.5, wouldn't there still be a problem? Or conversely, if 1.5 is already installed, then I think this will just switch to being a static build?

I would think the Rust side would need to do some kind of runtime version detection?

Personally, I think using the system library is a bad idea. I know others don't agree, but it generally seems like it is a recipe for problems. I feel to some degree that shared linking should be opt-in instead of opt-out.

[saethlin]

If a Rust tool is using the system libgit2 1.4, and then the system updates to an ABI breaking 1.5, wouldn't there still be a problem?

Ah, right. There also needs to be an adjustment to what we ask the linker for. -lgit2.so.1.4 or such. I think that's possible, I'll give it a shot in a few hours.

Or conversely, if 1.5 is already installed, then I think this will just switch to being a static build?

Yup. Or we could produce a build-time error.

Personally, I think using the system library is a bad idea.

I agree. It's a mistake to use components delivered by the system package manager without integrating with it. You may have a program on your system that requires libgit2 version 1.4.0 but if it isn't integrated with the package manager, the package manager has no way to know that it can't just upgrade to 1.5.0. There's no "just upgrade the shared library to upgrade all the apps" story here that people often use to argue for dynamic linkage, because git2 is shipping ABI breaks so often.

But even though we can't tell the package manager not to upgrade to a version of libgit2 that breaks the Rust program, we can at least not have UB. And also a better error. This change wouldn't completely fix the problem, but it would shave off the class of error which I and others have opened issues about. So I don't see a way that this sort of change (once I finish it, as you point out) would make things worse, but I would be in favor of removing dynamic linkage support entirely.

[saethlin]

I looked into this and I'm not sure that it's possible, but I'm also not sure if it's necessary.

In short, it looks like Cargo doesn't support specifying a versioned native library. You can't pass cargo:rustc-link-arg because that doesn't apply to dependents. And cargo:rustc-link-library just forwards down to -l, so you can't provide the file extension through that flag. And you can't even use this suggestion by saying cargo:rustc-link-lib=:libgit2.so.1.4 because this invokes some piece of Cargo functionality that I'm currently unable to locate documentation for (I feel like it should be in the build script docs but I can't find it there):

   Compiling libgit2-sys v0.13.1+1.4.2 (/home/ben/git2-rs/libgit2-sys)
error: renaming of the library `` was specified, however this crate contains no `#[link(...)]` attributes referencing this library

error: could not compile `libgit2-sys` due to previous error

But also, I'm an Arch user so I've recently pulled down the latest libgit2 and my .cargo/bin is in shambles:

╭ ➜ ben@archlinux:~/.cargo/bin
╰ ➤ ldd * | grep libgit
	libgit2.so.1.3 => not found
	libgit2.so.1.3 => not found
	libgit2.so.1.3 => not found
	libgit2.so.1.3 => not found
	libgit2.so.1.3 => not found
	libgit2.so.1.3 => not found
	libgit2.so.1.3 => not found
	libgit2.so.1.3 => not found

I don't know that we can rely on this extension versioning to always kick in when linking, but this seems to be what it is for and it is doing its job. I suspect if we just prevent invalid linkage at build time, things will correctly break when the system is upgraded.

[alexcrichton]

If libgit2's ABI is unstable, which it was assumed to not be with the 1.0 version and beyond, then I agree with @ehuss that the option to dynamically link against a system-provided version should be opt-in, not opt-out.

[saethlin]

Just to be totally clear, here is libgit2's own wording: https://github.com/libgit2/libgit2/blob/1327dbcf2a4273a8ba6fd978db5f0882530af94d/docs/api-stability.md?plain=1#L44-L62

Most notably:

* Our ABI is only considered stable through a _minor_ release.

I'm up for amending this PR to also make the vendored library the default.

[alexcrichton]

I think that defaulting to the vendored copy probably shouldn't be done with features since it's typically pretty hard to turn off default features and this otherwise may want to be configured more regularly. Instead could an env var be invented for opting-in to using pkg-config? Maybe something like GIT2RS_TRY_PKGCONFIG=1 or something like that. Build scripts often have random environment variables (in my experience at least) associated with them.

Alternatively we could have an off-by-default Cargo feature which enables pulling in pkg-config and actually uses it to try to find a system version.

[joshtriplett]

I know that Linux distros will find it hard to work with a default of not using the system library, and will get annoyed at having to set one more environment variable or feature to use the system library.

In general, I wish we had a better unified mechanism for making this decision cargo-crate-graph-wide rather than unique mechanisms on a crate-by-crate basis.

But in the meantime, I agree that we shouldn't just set the vendoring feature by default. And conversely, having a feature that enables searching for a system library has the opposite problem: if a single crate enables it, it's hard to disable. This isn't really a good fit for Cargo features at all.

I do think at a minimum we should merge the change to look for one minor version of the library, so that we can detect the issue at link time. That's not a complete fix, but it's an improvement.

And apart from that, someone may want to have a detailed and polite conversation with libgit2 upstream to explain that shared library versioning doesn't work that way. If the library isn't ABI-compatible, the SONAME has to change. If the SONAME hasn't changed, the library must be ABI-compatible. libgit2 has bindings to many, many languages, and those bindings necessarily rely on the ABI, not just the API. Declaring otherwise in a document doesn't change the technical mechanisms of dynamic linking, and causes breakage exactly like we observed.

• Since many FFIs use ABIs directly (for example, .NET P/Invoke or Rust), this instability is unfortunate.

"unfortunate" is an understatement.

[saethlin]

The other alternative is that we could retain the status quo with features, but let users set GIT2RS_TRY_PKGCONFIG=0 to force git2-rs anywhere in the tree to use the vendored build.

@joshtriplett They have set the SONAME correctly:

╭ ➜ ben@archlinux:~
╰ ➤ objdump -p /usr/lib/libgit2.so | grep SONAME
  SONAME               libgit2.so.1.4

If I understand correctly, that's why I had to reinstall tools like cargo-edit when I updated my system a few days ago.

[joshtriplett]

@joshtriplett They have set the SONAME correctly:

╭ ➜ ben@archlinux:~
╰ ➤ objdump -p /usr/lib/libgit2.so | grep SONAME
  SONAME               libgit2.so.1.4

If I understand correctly, that's why I had to reinstall tools like cargo-edit when I updated my system a few days ago.

Oh, huh. My apologies, I misread the library on my system because I didn't realize my system copy of libgit2 was 1.1. I think it hasn't been updated in Debian in a while because it changes SONAME and thus requires a transition.

OK, if the SONAME is actually changing with each minor version, then I think it does suffice to just change our pkg-config search to look for the correct range of versions. Ideally, the pkg-config file itself would reflect the SONAME (e.g. libgit2-1-4.pc), but we can at least work around that.

I think we should just change our pkg-config search, and not change our defaults; that should be sufficient to prevent using an incompatible system library.

[joshtriplett]

LGTM.

@alexcrichton @ehuss Thoughts on merging this in its current form?

[alexcrichton]

I'm confused, this is still trying to use the system library by default? (but with more strict version constraints, which I thought is what this PR originally looked like and at the time I thought it would be better to not use pkg-config by default)

Personally though this has already taken more of my time than I'm willing to give, so I'll leave this up to y'all.

[saethlin]

My biggest concern is that we are getting reports of projects that are built in a broken state, because the version constraint in here is wrong. This fixes that.

I think that defaulting to something other than pkg-config or introducing an environment variable, even if we do it, is going to stir up bikeshedding and I'd rather get a fix to users ASAP.

[ethomson]

👋 Hello from https://github.com/libgit2/libgit2, where I've been seeing some reports of rust projects having a bad day with libgit2 releases. This is obviously disappointing. So I just wanted to mention that we would welcome a detailed and polite conversation about how we can ease your pain. As is noted upthread, we do try to be mindful of these issues by updating our SOVERSION when there are ABI changes, but there's certainly a lot more that we could do.

This probably isn't the right place to have that chat, but feel free to drop by https://github.com/libgit2/libgit2/discussions if you'd like to discuss ABIs.

[joshtriplett]

@ethomson Thank you, and sorry for the initial misunderstanding about libgit2's SONAME. (I've seen libraries that do the breakage-in-minor-versions thing without setting the SONAME accordingly, and I misread the state of libgit2 on my system and thought that it had done so.)

Going forward, I think we'll be able to address this breakage by having git2-rs check pkg-config more precisely for only one supported minor version number. We may also want to issue an updated 0.13 release with an upper bound on the pkg-config check.

That said, I think the Rust git2-rs bindings and any other binding that depends on a compiled binary would benefit from doing more traditional breakage-on-major-version-only library versioning, if only to match semantic versioning expectations from other libraries and make transitions easier. It's not that big a deal if libgit2 ends up having a major version number of 4 or 5.

[saethlin]

I think that what this library should do going forward is a great topic for discussion, but the diff in this PR is small and I think uncontroversial? So it would be nice if it were not blocked by that discussion.