libgit2 version check #695

New Issue

2020-11-28T04:50:10-06:00

nmeum commented

2020-11-28 04:50:10 -06:00

(Migrated from github.com)

During compilation git2go performs the following libgit2 version check:

ad3ec3664d/git_system_dynamic.go (L10-L12)

Suppose I want to install a go software which uses git2go through go get on both Ubuntu 20.04 and Arch Linux I would run into the following problem: These Linux distributions ship different versions of libgit2. Since the version check performed by git2go is very strict (requiring an exact match of both minor and major version) the aforementioned go software would only compile with one specific version of libgit2, according to the version suffix I used for the git2go import.

IMHO it would be desirable if the git2go version check would be less strict. If I am informed correctly, libgit2 uses semantic versioning. As such, the API should not change in backwards incompatible ways without a major version bump. For instance, github.com/libgit2/git2go/v30 (which currently requires libgit2 1.0.0) should IMHO compile with any libgit2 version where LIBGIT2_VER_MAJOR == 1 and LIBGIT2_VER_MINOR >= 0 (e.g. should compile fine with both libgit2 1.1.0 and libgit2 1.0.0).

During compilation git2go performs the following libgit2 version check: https://github.com/libgit2/git2go/blob/ad3ec3664d54779c4c2e49e41f85e886fbff343c/git_system_dynamic.go#L10-L12 Suppose I want to install a go software which uses git2go through `go get` on both Ubuntu 20.04 and Arch Linux I would run into the following problem: These Linux distributions ship different versions of libgit2. Since the version check performed by git2go is very strict (requiring an exact match of both minor and major version) the aforementioned go software would only compile with one specific version of libgit2, according to the version suffix I used for the git2go import. IMHO it would be desirable if the git2go version check would be less strict. If I am informed correctly, libgit2 uses semantic versioning. As such, the API should not change in backwards incompatible ways without a major version bump. For instance, `github.com/libgit2/git2go/v30` (which currently requires libgit2 `1.0.0`) should IMHO compile with any libgit2 version where `LIBGIT2_VER_MAJOR == 1` and `LIBGIT2_VER_MINOR >= 0` (e.g. should compile fine with both libgit2 `1.1.0` and libgit2 `1.0.0`).

lhchavez commented

2020-11-28 07:27:10 -06:00

(Migrated from github.com)

sounds reasonable, although the important word here is should (as in all v1.x.w should work with any other v1.y.z where x < y).

I guess that if there's enough coverage for this in the CI, there's no reason not to give it a try. if we encounter a scenario where this is not true, we can replace the LIBGIT2_VER_MINOR >= X with (LIBGIT2_VER_MINOR >= X && LIBGIT_VER_MINOR < Y), which still allows for some amount of leeway.

sounds reasonable, although the important word here is _should_ (as in all v1.x.w _should_ work with any other v1.y.z where x < y). I guess that if there's enough coverage for this in the CI, there's no reason not to give it a try. if we encounter a scenario where this is not true, we can replace the `LIBGIT2_VER_MINOR >= X` with `(LIBGIT2_VER_MINOR >= X && LIBGIT_VER_MINOR < Y)`, which still allows for some amount of leeway.

nmeum commented

2020-11-28 08:57:01 -06:00

(Migrated from github.com)

I think the required change is also very straightforward. I can just send a corresponding PR if you want me to do. Should the version check just be changed on the master branch for now or also on the release branches?

lhchavez commented

2020-11-28 09:01:53 -06:00

(Migrated from github.com)

if you can do the CI change (the most important although difficult and time-consuming part) together with the corresponding code change it would be preferable!

don't worry about the release branches: there's a CI script that automatically cherry-picks all changes to master.

if you can do the CI change (the most important although difficult and time-consuming part) together with the corresponding code change it would be preferable! don't worry about the release branches: there's a CI script that automatically cherry-picks all changes to master.

nmeum commented

2020-11-28 09:10:52 -06:00

(Migrated from github.com)

Unfortunately I am not familiar with GitHub CI.

lhchavez commented

2020-11-28 09:35:20 -06:00

(Migrated from github.com)

they're pretty easy to work with! here's our full config: https://github.com/libgit2/git2go/blob/master/.github/workflows/ci.yml

essentially most of the work needed is to create / modify some shell scripts and then invoke the right Makefile command to run the tests.

they're pretty easy to work with! here's our full config: https://github.com/libgit2/git2go/blob/master/.github/workflows/ci.yml essentially most of the work needed is to create / modify some shell scripts and then invoke the right Makefile command to run the tests.

nmeum commented

2020-11-28 10:52:35 -06:00

(Migrated from github.com)

And essentially the CI would need to be modified in a way that it uses different versions of libgit2 for testing?

lhchavez commented

2020-11-28 11:26:14 -06:00

(Migrated from github.com)

yeah: add one more job (say, called forwards-compatibility) that checks out libgit2 (or downloads a tarball or even a pre-built package, don't know if there's an easy way to do so) to the latest minor version that we know of, install them as system library and tries to run the tests against that.

yeah: add one more job (say, called `forwards-compatibility`) that checks out libgit2 (or downloads a tarball or even a pre-built package, don't know if there's an easy way to do so) to the latest minor version that we know of, install them as system library and tries to run the tests against that.

nmeum commented

2020-11-28 11:52:22 -06:00

(Migrated from github.com)

My initial idea was to simply add a new matrix build variable with different libgit2 version to the existing build-static job.

lhchavez commented

2020-11-28 11:54:54 -06:00

(Migrated from github.com)

it's probably better to do this with the systemwide library (either the static or dynamic versions are fine, but odds are that folks are going to use the dynamic version), since that's how folks are going to use it (we've been bitten by this small difference in the past ^^;; ). build-static should be reserved to using exactly the version that's vendored to make things easier to understand.

it's probably better to do this with the systemwide library (either the static or dynamic versions are fine, but odds are that folks are going to use the dynamic version), since that's how folks are going to use it (we've been bitten by this small difference in the past ^^;; ). `build-static` should be reserved to using exactly the version that's vendored to make things easier to understand.

nmeum commented

2020-11-28 11:58:14 -06:00

(Migrated from github.com)

Sure, I will experiment a bit in the PR to get that working.

nmeum commented

2020-11-28 12:10:03 -06:00

(Migrated from github.com)

I am using a matrix variable for the build-system-dynamic job now, seems to work in general. Only problem regarding testing: The master branch currently only compiles with libgit2 >= 1.1.0 (due to GIT_BLAME_IGNORE_WHITESPACE) since 1.1.0 is the most recent version there is really no other version to successfully test this with on the master branch.

I am using a matrix variable for the `build-system-dynamic` job now, seems to work in general. Only problem regarding testing: The master branch currently only compiles with libgit2 >= `1.1.0` (due to `GIT_BLAME_IGNORE_WHITESPACE`) since `1.1.0` is the most recent version there is really no other version to successfully test this with on the master branch.

Sign in to join this conversation.