relax version check #696

2020-11-28T11:49:51-06:00

nmeum commented

2020-11-28 11:49:51 -06:00

(Migrated from github.com)

Fixes: #695

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

lhchavez (Migrated from github.com) left a comment

thanks for doing this, it looks good overall! we just need to make the CI green and we're good to merge 🚀

thanks for doing this, it looks good overall! we just need to make the CI green and we're good to merge :rocket:

.github/workflows/ci.yml

lhchavez (Migrated from github.com) commented

2020-11-28 12:20:40 -06:00

        libgit2: [ '1.1.0' ]

we probably only need to have a matrix with at most two versions: the current branch's version (in this case 1.1.0) and the largest known version number (in this case 1.1.0). patch versions only carry bugfixes with no API changes, so we can ignore those.

when this is cherry-picked to other branches (say, v30), it will need to be changed to

        libgit2: [ '1.0.0', '1.1.0' ]

```suggestion libgit2: [ '1.1.0' ] ``` we probably only need to have a matrix with at most two versions: the current branch's version (in this case 1.1.0) and the largest known version number (in this case 1.1.0). patch versions only carry bugfixes with no API changes, so we can ignore those. when this is cherry-picked to other branches (say, v30), it will need to be changed to ```yaml libgit2: [ '1.0.0', '1.1.0' ] ```

git_bundled_static.go

lhchavez (Migrated from github.com) commented

2020-11-28 12:21:34 -06:00

the test results show that this combination is broken, so we need to have this be:

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

and since we need to update the CI anyways every time there's a new release to explicitly change the ref, might as well have this be:

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

which is... exactly the same as what we have right now. BUT the difference is that in the other branches (say, v30) it'll be:

#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"

the test results show that this combination is broken, so we need to have this be: ```cpp #if LIBGIT2_VER_MAJOR != 1 || LIBGIT2_VER_MINOR < 1 # error "Invalid libgit2 version; this git2go supports libgit2 >= v1.1" ``` and since we need to update the CI anyways every time there's a new release to explicitly change the ref, might as well have this be: ```suggestion #if LIBGIT2_VER_MAJOR != 1 || LIBGIT2_VER_MINOR < 1 || LIBGIT2_VER_MINOR > 1 # error "Invalid libgit2 version; this git2go supports libgit2 between v1.1.0 and v1.1.0" ``` which is... exactly the same as what we have right now. BUT the difference is that in the other branches (say, v30) it'll be: ```cpp #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-28 12:49:26 -06:00

git_bundled_static.go

nmeum (Migrated from github.com) commented

2020-11-28 12:49:26 -06:00

I don't think that an upper bound on the supported minor version is really strictly needed right now, but committed it anyhow as you suggested.

the test results show that this combination is broken

Yep, only had that in for testing if the build would fail.

I don't think that an upper bound on the supported minor version is really strictly needed right now, but committed it anyhow as you suggested. > the test results show that this combination is broken Yep, only had that in for testing if the build would fail.

nmeum (Migrated from github.com) reviewed 2020-11-28 12:49:38 -06:00

.github/workflows/ci.yml

nmeum (Migrated from github.com) commented

2020-11-28 12:49:38 -06:00

Changed the version matrix.

lhchavez (Migrated from github.com) approved these changes 2020-11-28 13:09:54 -06:00

nmeum commented

2020-11-28 13:16:05 -06:00

(Migrated from github.com)

How does backporting this to other branches work? Even if the commit is cherry-picked the supported libgit2 version in the CI matrix needs to be adjusted for the other release branches.

lhchavez commented

2020-11-28 14:29:56 -06:00

(Migrated from github.com)

How does backporting this to other branches work? Even if the commit is cherry-picked the supported libgit2 version in the CI matrix needs to be adjusted for the other release branches.

normally that's done through a GitHub action (https://github.com/libgit2/git2go/runs/1468017967?check_suite_focus=true), but it seems like the merge conflicts prevented that from happening :/ so it'll need to be done by hand. i can do that tomorrow, or you can give it a try by running git cherry-pick -x 1fabe95fb7275df980ff6ab03fb85eac91c5849d from the release-1.0 branch and submit a PR for that.

> How does backporting this to other branches work? Even if the commit is cherry-picked the supported libgit2 version in the CI matrix needs to be adjusted for the other release branches. normally that's done through a GitHub action (https://github.com/libgit2/git2go/runs/1468017967?check_suite_focus=true), but it seems like the merge conflicts prevented that from happening :/ so it'll need to be done by hand. i can do that tomorrow, or you can give it a try by running `git cherry-pick -x 1fabe95fb7275df980ff6ab03fb85eac91c5849d` from the `release-1.0` branch and submit a PR for that.

Sign in to join this conversation.

No reviewers

lhchavez