[release-1.0] Relax libgit2 minor version check (#696) #697

2020-11-29T03:07:33-06:00

nmeum commented

2020-11-29 03:07:33 -06:00

(Migrated from github.com)

Backport of #696.

The major version must still be an exact match since libgit2 uses
semantic versioning and changes to the major number indicate backwards
incompatible changes to the API.

Fixes: #695
(cherry picked from commit 1fabe95fb7)

Backport of #696. ---- The major version must still be an exact match since libgit2 uses semantic versioning and changes to the major number indicate backwards incompatible changes to the API. Fixes: #695 (cherry picked from commit 1fabe95fb7275df980ff6ab03fb85eac91c5849d)

lhchavez (Migrated from github.com) reviewed 2020-11-29 08:12:05 -06:00

lhchavez (Migrated from github.com) left a comment

some differences from the master change need to be removed and we're ready to merge!

.github/workflows/ci.yml

lhchavez (Migrated from github.com) commented

2020-11-29 08:11:22 -06:00

        libgit2: [ '1.0.0', '1.1.0' ]

we only need to consider the endpoints of the supported revisions to avoid overstraining the CI.

```suggestion libgit2: [ '1.0.0', '1.1.0' ] ``` we only need to consider the endpoints of the supported revisions to avoid overstraining the CI.

git_bundled_static.go

lhchavez (Migrated from github.com) commented

2020-11-29 08:10:54 -06:00

Can these be written in the same style as the master change?

#if LIBGIT2_VER_MAJOR != 1 || LIBGIT2_VER_MINOR < 0 || LIBGIT2_VER_MINOR > 1
# error "Invalid libgit2 version; this git2go supports libgit2 between v1.0.0 and v1.1.0".

Can these be written in the same style as the master change? ```suggestion #if LIBGIT2_VER_MAJOR != 1 || LIBGIT2_VER_MINOR < 0 || LIBGIT2_VER_MINOR > 1 # error "Invalid libgit2 version; this git2go supports libgit2 between v1.0.0 and v1.1.0". ```

nmeum (Migrated from github.com) reviewed 2020-11-29 09:28:24 -06:00

git_bundled_static.go

nmeum (Migrated from github.com) commented

2020-11-29 09:28:24 -06:00

I don't think that it is wise to have an upper bound on the supported minor version, having one really defeats the purpose of forward compatibility IMHO. Let's assume we include an upper bound check as you suggest: If libgit2 1.2.0 is released next week we need to adjust the version check again and all go software vendoring an old version of git2go will need to update git2go in order to build with libgit 1.2.0 even if libgit2 1.2.0 is compatible with the vendored git2go (which it should be due to semantic versioning).

nmeum (Migrated from github.com) reviewed 2020-11-29 09:28:32 -06:00

.github/workflows/ci.yml

nmeum (Migrated from github.com) commented

2020-11-29 09:28:32 -06:00

Sure

lhchavez (Migrated from github.com) reviewed 2020-11-29 09:36:29 -06:00

git_bundled_static.go

lhchavez (Migrated from github.com) commented

2020-11-29 09:36:28 -06:00

it is similarly unwise to release untested combinations of software :(

semantic versioning is not perfect, unfortunately, and sometimes seemingly compatible changes are introduced that are not compatible with Go. we need to be defensive to avoid releasing any tag that is not backwards-compatible because that breaks the Go compatibility promises.

the good thing is that once new revisions of libgit2 are released, we just need to check that it's indeed compatible with the current branch and then tweak this version bound. we release a new tag and then everybody is happy since they can still refer to "github.com/libgit2/git2go/v31" (or whatever version they use) and be assured that the system library has been tested and works with their software.

it is similarly unwise to release untested combinations of software :( semantic versioning is not perfect, unfortunately, and sometimes seemingly compatible changes are introduced that are not compatible with Go. we need to be defensive to avoid releasing any tag that is not backwards-compatible because that breaks the Go compatibility promises. the good thing is that once new revisions of libgit2 are released, we just need to check that it's indeed compatible with the current branch and then tweak this version bound. we release a new tag and then everybody is happy since they can still refer to `"github.com/libgit2/git2go/v31"` (or whatever version they use) and be assured that the system library has been tested and works with their software.

nmeum (Migrated from github.com) reviewed 2020-11-29 10:57:56 -06:00

git_bundled_static.go

nmeum (Migrated from github.com) commented

2020-11-29 10:57:56 -06:00

I get your point, the problem I personally have with this approach is that I don't want to update the git2go version I am using all the time just for an increment in the version check. I really believe that this unnecessarily complicates installation of software which uses git2go as one can't tell users to simply install libgit2 1.X as a dependency. Every time a new libgit2 is released and I update my distro packages my build basically breaks. If you really insist on a strict minor version check would it be possible to introduce some kind of option for disabling it as an alternative? If I had written my libgit2-based program in C instead of Go I (as the downstream developer) would also be responsible for checking if the libgit2 version, installed by a user, is compatible with my program.

lhchavez (Migrated from github.com) reviewed 2020-11-29 11:46:11 -06:00

git_bundled_static.go

lhchavez (Migrated from github.com) commented

2020-11-29 11:46:10 -06:00

new versions are released very infrequently (1-2 per year), so the overhead would not be as large. we already need to do some work around those times (like create new branches, bump versions, etc.) so adding this extra task of tweaking the minor version ranges for the previous release branches is palatable for us.

that way we won't have the build breaking for the opposite problem: a version of libgit2 1.x that should have been compatible that for some reason isn't.

regarding adding an option for disabling the version check, how would it look like? AFAIK the only way to achieve this would be to add a new tag, but that would defeat the purpose of being go get compatible.

new versions are released very infrequently (1-2 per year), so the overhead would not be as large. we already need to do some work around those times (like create new branches, bump versions, etc.) so adding this extra task of tweaking the minor version ranges for the previous release branches is palatable for us. that way we won't have the build breaking for the opposite problem: a version of libgit2 1.x that should have been compatible that for some reason isn't. regarding adding an option for disabling the version check, how would it look like? AFAIK the only way to achieve this would be to add a new tag, but that would defeat the purpose of being `go get` compatible.

nmeum (Migrated from github.com) reviewed 2020-11-29 13:30:09 -06:00

git_bundled_static.go

nmeum (Migrated from github.com) commented

2020-11-29 13:30:09 -06:00

[…] tweaking the minor version ranges for the previous release branches is palatable for us.

Sure, but it still annoying for downstream consumers to update git2go all the time ;)

that way we won't have the build breaking for the opposite problem: a version of libgit2 1.x that should have been compatible that for some reason isn't.

I guess I just have more faith in the semantic versioning scheme here, breakage introduced in a minor revision should IMHO be considered a bug in libgit2 and not in git2go. That's really the beauty of correctly executed semantic versioning, but as you pointed out there is no guarantee that a minor revision might not introduce a regression, but then again there is also no guarantee that a minor version didn't introduce a regression just because the git2go test suite passes.

regarding adding an option for disabling the version check, how would it look like?

Best I came up with so far would be something like:

#if (LIBGIT2_VER_MAJOR != 1 || LIBGIT2_VER_MINOR < 0 || LIBGIT2_VER_MINOR > 1) && !defined(GIT2GO_SKIP_VER_CHECK)
# error "Invalid libgit2 version; this git2go supports libgit2 between v1.0.0 and v1.1.0".
#endif

and then invoke go get as:

CGO_CFLAGS=-DGIT2GO_SKIP_VER_CHECK  go get <…>

Would be even better if a package could set CFLAGS for other packages it is importing, thereby eliminating the need to manually set the environment variable, but I don't think that doing so is possible.

We can also turn the #error into a #warning and add a !defined(…) to disable the warning 🤔

> […] tweaking the minor version ranges for the previous release branches is palatable for us. Sure, but it still annoying for downstream consumers to update git2go all the time ;) > that way we won't have the build breaking for the opposite problem: a version of libgit2 1.x that should have been compatible that for some reason isn't. I guess I just have more faith in the semantic versioning scheme here, breakage introduced in a minor revision should IMHO be considered a bug in libgit2 and not in git2go. That's really the beauty of correctly executed semantic versioning, but as you pointed out there is no guarantee that a minor revision might not introduce a regression, but then again there is also no guarantee that a minor version didn't introduce a regression just because the git2go test suite passes.  > regarding adding an option for disabling the version check, how would it look like? Best I came up with so far would be something like: ```C #if (LIBGIT2_VER_MAJOR != 1 || LIBGIT2_VER_MINOR < 0 || LIBGIT2_VER_MINOR > 1) && !defined(GIT2GO_SKIP_VER_CHECK) # error "Invalid libgit2 version; this git2go supports libgit2 between v1.0.0 and v1.1.0". #endif ``` and then invoke `go get` as: ``` CGO_CFLAGS=-DGIT2GO_SKIP_VER_CHECK go get <…> ``` Would be even better if a package could set `CFLAGS` for other packages it is importing, thereby eliminating the need to manually set the environment variable, but I don't think that doing so is possible. We can also turn the `#error` into a `#warning` and add a `!defined(…)` to disable the warning :thinking:

lhchavez (Migrated from github.com) reviewed 2020-11-29 18:32:37 -06:00

git_bundled_static.go

lhchavez (Migrated from github.com) commented

2020-11-29 18:32:36 -06:00

Something else to consider is that Go's versioning philosophy is not entirely in-line with semantic versioning, aside from the fact that breaking incompatibilities are marked with a major version bump, but still recognizes that people will want to use semantic versioning, so it arrived at a sort of trade-off for supporting parts of semver. One of Go's language creators wrote an amazing series of articles about Go's versioning philosophy and the rationales behind it (https://research.swtch.com/vgo-principles is the last one and is a sort of TL;DR of the whole thing), and in one argues that in order to maintain build repeatability, when multiple libraries depend on different version of another library, the minimum version that satisfies all constraints should be chosen, instead of the more usual choice of picking the largest possible that satisfies semantic versioning or using SAT solvers. There's also the fact that even if the API doesn't change between minor versions, semantics might inadvertently change even when just fixing a bug (see Hyrum's Law).

One consequence of this is that library authors are expected to bump their dependency versions (and thus test their software with that particular version combination) more often, so that the minimum version advances as much as possible. libgit2 is not subject to such strict rules, so breaking Go's compatibility expectations is definitely not a bug in libgit2. Therefore, this wrapper library needs to be a bit more careful about it and provide an abstraction that respects the conventions of Go and therefore the expectations of users. That requirement to bump versions more often is definitely a burden, but overall the way versioning works does have its benefits.

I think the solution achieved in this change is a pretty good trade-off. It still goes with Go's versioning philosophy's spirit of explicitly checking for compatibility (although as you point out, it's not perfect, nor can we expect it to ever be) while providing some amount of flexibility and ease of use that was previously not possible. Even with the trade-offs, it's definitely a net-positive for everyone, so thanks for doing it!

w.r.t. the option to disable the version check, since it's explicitly opt-in, it might be okay to consider in the future as an escape hatch if the current solution ends up not panning out for one reason or another (no #warning needed: users are expected to be familiar with how the build works by opting into using non-standard flags in case an inscrutable build error appears).

Once again, thanks for this change! :D

Something else to consider is that Go's versioning philosophy is not _entirely_ in-line with semantic versioning, aside from the fact that breaking incompatibilities are marked with a major version bump, but still recognizes that people will want to use semantic versioning, so it arrived at a sort of trade-off for supporting parts of semver. One of Go's language creators wrote an amazing series of articles about Go's versioning philosophy and the rationales behind it (https://research.swtch.com/vgo-principles is the last one and is a sort of TL;DR of the whole thing), and in one argues that in order to maintain [build repeatability](https://research.swtch.com/vgo-repro), when multiple libraries depend on different version of another library, the [minimum version](https://research.swtch.com/vgo-mvs) that satisfies all constraints should be chosen, instead of the more usual choice of picking the largest possible that satisfies semantic versioning or using SAT solvers. There's also the fact that even if the API doesn't change between minor versions, semantics might inadvertently change even when just fixing a bug (see [Hyrum's Law](http://hyrumslaw.com/)). One consequence of this is that library authors are expected to bump their dependency versions (and thus test their software with that particular version combination) more often, so that the minimum version advances as much as possible. libgit2 is not subject to such strict rules, so breaking Go's compatibility expectations is definitely not a bug in libgit2. Therefore, this wrapper library needs to be a bit more careful about it and provide an abstraction that respects the conventions of Go and therefore the expectations of users. That requirement to bump versions more often is definitely a burden, but overall the way versioning works does have its benefits. I think the solution achieved in this change is a pretty good trade-off. It still goes with Go's versioning philosophy's spirit of explicitly checking for compatibility (although as you point out, it's not perfect, nor can we expect it to ever be) while providing some amount of flexibility and ease of use that was previously not possible. Even with the trade-offs, it's definitely a net-positive for everyone, so thanks for doing it! w.r.t. the option to disable the version check, since it's explicitly opt-in, it might be okay to consider in the future as an escape hatch if the current solution ends up not panning out for one reason or another (no `#warning` needed: users are expected to be familiar with how the build works by opting into using non-standard flags in case an [inscrutable build error](https://github.com/libgit2/git2go/issues/617) appears). Once again, thanks for this change! :D