Update to LibGit2 v0.26.0 #22614

omus · 2017-06-29T17:14:05Z

Updates the version of LibGit2 to version 0.26.0 which includes various fixes. The change caused a few changes to the state of the libgit2 patches:

libgit2-mbedtls.patch: revised to be based off of (mbedTLS support libgit2/libgit2#4173)
libgit2-mbedtls-writer-fix.patch: deleted, now included in libgit2-mbedtls.patch
libgit2-gitconfig-symlink.patch: deleted, fix included in libgit2 v0.26.0 (libgit2/libgit2@86a8cd9)
libgit2-free-config.patch: deleted, fix included in libgit2 v0.26.0 (libgit2/libgit2@5552237)
libgit2-remote-push-NULL.patch: deleted, fix included in libgit2 v0.26.0 (libgit2/libgit2@90cdf44)
libgit2-mbedtls-verify.patch: updated to work with new libgit2-mbedtls.patch. I should try to get this included in mbedTLS support libgit2/libgit2#4173
libgit2-mbedtls-hotfix.patch: various fixes to mbedTLS support libgit2/libgit2#4173

Additionally the new libgit2-mbedtls.patch addresses libgit2/libgit2#3935 (comment) and should also fix #19135 (comment) and #20439

TODOs:

Fix issue with test validating self-signed certificates
Report bugs in mbedTLS support libgit2/libgit2#4173
Get libgit2-mbedtls-verify.patch included in mbedTLS support libgit2/libgit2#4173

ararslan · 2017-06-29T17:20:31Z

deps/patches/libgit2-mbedtls-verify.patch

 }

 -static int check_host_name(const char *name, const char *host)
 -{
-    if (!strcasecmp(name, host))
-        return 0;
+-	if (!strcasecmp(name, host))


Looks like GitHub diff can't handle diffing diffs. The original patch removed the check_host_name function and used the mbedtls function for doing cert verification instead of rolling our own in libgit2

To elaborate on my question mark, it appears the changes in the patch change the indentation. Seems unnecessary and kind of bloats the patch. Am I missing something?

Ah, is it just that the diff diffing is weird?

It is totally possible the indentation was changed from spaces into tabs. libgit2/libgit2#4173 made the indentation into tabs so when I revised this patch there are now differences in the whitespace.

Gotcha. Thanks.

tkelman · 2017-06-29T17:20:48Z

base/libgit2/libgit2.jl

+            ENV["SSL_CERT_FILE"]
+        else
+            # If we have a bundled ca cert file, point libgit2 at that so SSL connections work.
+            abspath(ccall(:jl_get_julia_home, Any, ()),Base.DATAROOTDIR,"julia","cert.pem")


[minor style point] few more spaces after the commas here?

Sure. I didn't modify the styling when I moved this.

tkelman · 2017-06-29T17:21:46Z

deps/libgit2.mk

@@ -78,29 +78,14 @@ $(LIBGIT2_SRC_PATH)/libgit2-agent-nonfatal.patch-applied: $(LIBGIT2_SRC_PATH)/so
 		patch -p1 -f < $(SRCDIR)/patches/libgit2-agent-nonfatal.patch


we didn't upstream this one?

Hmm, I'll double check

Doesn't look like we did https://github.com/libgit2/libgit2/blob/master/src/transports/ssh.c#L300-L301

#17459 (comment)

tkelman · 2017-06-29T17:23:24Z

deps/libgit2.version

-LIBGIT2_BRANCH=v0.25.1
-LIBGIT2_SHA1=2fcb8705e584ca61f6c4657525c9d2713f6a39d2
+LIBGIT2_BRANCH=v0.26.0
+LIBGIT2_SHA1=15e119375018fba121cf58e02a9f17fe22df0df8


should delete the old checksum files, and add the new ones

I haven't interacted with our checksum files before. What's the standard procedure?

git rm -r deps/checksums/libgit2-2fcb8705e584ca61f6c4657525c9d2713f6a39d2.tar.gz git add deps/checksums/libgit2-15e119375018fba121cf58e02a9f17fe22df0df8.tar.gz

Oh interesting. I was thinking I would have to retrieve the checksum myself.

if you locally build a new dependency version and the checksum files aren't already present, it'll create them the first time - we add them to git so they get checked for everyone else

omus · 2017-06-29T17:38:31Z

deps/patches/libgit2-mbedtls-hotfix.patch

+ 	// OPTIONAL because REQUIRED drops the certificate as soon as the check
+ 	// is made, so we can never see the certificate and override it.
+ 	mbedtls_ssl_conf_authmode(git__ssl_conf, MBEDTLS_SSL_VERIFY_OPTIONAL);
+	//mbedtls_ssl_conf_authmode(git__ssl_conf, MBEDTLS_SSL_VERIFY_NONE);


This was me trying to get the cert tests to co-operate. I'll remove this yet.

tkelman · 2017-06-29T22:29:34Z

test/libgit2.jl

@@ -369,7 +369,7 @@ mktempdir() do dir
                error("unexpected")
            catch e
                @test typeof(e) == LibGit2.GitError
-                @test startswith(sprint(show,e),"GitError(Code:ENOTFOUND, Class:OS, Failed to resolve path")
+                @test startswith(sprint(show,e),"GitError(Code:ENOTFOUND, Class:OS, failed to resolve path")


looks like this makes the windows CI sad, since it uses libgit2 from the nightly binary instead of compiling from source

for the sake of distro packagers who might not be able to upgrade libgit2 versions right away, how about making this conditional on libgit2 version?

Sounds good

omus · 2017-06-30T18:44:52Z

One of my patches isn't applying on Travis. Attempting to reproduce locally.

tkelman · 2017-06-30T19:16:32Z

test/libgit2.jl

@@ -1910,8 +1921,11 @@ mktempdir() do dir
                            deserialize(f)
                        end
                        @test err.code == LibGit2.Error.ERROR
-                        @test err.msg == "Invalid Content-Type: text/plain"
+                        @test err.msg == "invalid Content-Type: text/plain"


this should probably be version dependent as well

These tests only run on Linux so they should always be using the specified version. I could and the conditional if it is still preferable

unless a distro packager uses a different version of libgit2

😱 fair enough

I think I'll update this and #22614 (comment) to be case insensitive

omus · 2017-06-30T20:12:13Z

One of my patches isn't applying on Travis. Attempting to reproduce locally.

I can't seem to reproduce the issue locally. I think I'll push these commits individually to isolate the issue.

omus · 2017-07-01T04:58:28Z

Looks like it was an issue with the Travis CI cache

omus · 2017-07-01T19:09:03Z

Trying a new PR to see if I can work around the Travis CI caching issues I'm encountering.

omus · 2017-07-02T18:57:49Z

Should be ready for final review. I've commented on libgit2/libgit2#4173 with the new changes this PR includes in the patches.

tkelman · 2017-07-03T01:53:05Z

deps/libgit2.mk

 	$(LIBGIT2_SRC_PATH)/libgit2-mbedtls-verify.patch-applied \
-	$(LIBGIT2_SRC_PATH)/libgit2-gitconfig-symlink.patch-applied \
-	$(LIBGIT2_SRC_PATH)/libgit2-free-config.patch-applied \


should this file be deleted?

Yes it should. Good catch

ararslan added domain:building Build system, or building Julia or its dependencies libgit2 The libgit2 library or the LibGit2 stdlib module labels Jun 29, 2017

ararslan reviewed Jun 29, 2017

View reviewed changes

tkelman reviewed Jun 29, 2017

View reviewed changes

omus commented Jun 29, 2017

View reviewed changes

tkelman reviewed Jun 29, 2017

View reviewed changes

omus force-pushed the cv/libgit2-0.26.0 branch from 15274dd to 268d157 Compare June 30, 2017 16:57

tkelman reviewed Jun 30, 2017

View reviewed changes

omus force-pushed the cv/libgit2-0.26.0 branch 2 times, most recently from 6700ba7 to 82a1229 Compare June 30, 2017 21:56

Update to LibGit2 v0.26.0

158d655

omus mentioned this pull request Jul 1, 2017

Update to LibGit2 v0.26.0 (Part II) #22652

Closed

omus force-pushed the cv/libgit2-0.26.0 branch from 9233148 to 158d655 Compare July 2, 2017 15:29

omus changed the title ~~WIP: Update to LibGit2 v0.26.0~~ Update to LibGit2 v0.26.0 Jul 2, 2017

tkelman reviewed Jul 3, 2017

View reviewed changes

Remove libgit2-free-config patch

5492792

tkelman merged commit 8d47a31 into master Jul 3, 2017

tkelman deleted the cv/libgit2-0.26.0 branch July 3, 2017 17:25

omus mentioned this pull request Jul 4, 2017

libgit2 test: use a valid user/pass parameter to test EAUTH instead of OS:ERROR #22681

Merged

nalimilan mentioned this pull request Jul 9, 2017

Fix libgit2 test failure with system libgit2 and OpenSSL #22722

Merged

This was referenced Nov 21, 2017

Libgit2 Segfault #24625

Closed

WIP: Backports for 0.6.2 #24519

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to LibGit2 v0.26.0 #22614

Update to LibGit2 v0.26.0 #22614

omus commented Jun 29, 2017 •

edited

Loading

ararslan Jun 29, 2017

omus Jun 29, 2017

ararslan Jun 29, 2017

ararslan Jun 29, 2017

omus Jun 29, 2017

ararslan Jun 29, 2017

tkelman Jun 29, 2017

omus Jun 29, 2017

tkelman Jun 29, 2017

omus Jun 29, 2017

omus Jun 29, 2017

tkelman Jun 29, 2017

tkelman Jun 29, 2017

omus Jun 29, 2017

tkelman Jun 29, 2017

omus Jun 29, 2017

tkelman Jun 29, 2017 •

edited

Loading

omus Jun 30, 2017

omus Jun 29, 2017

tkelman Jun 29, 2017 •

edited

Loading

omus Jun 30, 2017

omus commented Jun 30, 2017

tkelman Jun 30, 2017

omus Jun 30, 2017

tkelman Jun 30, 2017

omus Jun 30, 2017

omus Jun 30, 2017

omus commented Jun 30, 2017 •

edited

Loading

omus commented Jul 1, 2017

omus commented Jul 1, 2017 •

edited

Loading

omus commented Jul 2, 2017

tkelman Jul 3, 2017

omus Jul 3, 2017

		@@ -78,29 +78,14 @@ $(LIBGIT2_SRC_PATH)/libgit2-agent-nonfatal.patch-applied: $(LIBGIT2_SRC_PATH)/so
		patch -p1 -f < $(SRCDIR)/patches/libgit2-agent-nonfatal.patch

Update to LibGit2 v0.26.0 #22614

Update to LibGit2 v0.26.0 #22614

Conversation

omus commented Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus commented Jun 30, 2017 • edited Loading

omus commented Jul 1, 2017

omus commented Jul 1, 2017 • edited Loading

omus commented Jul 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus commented Jun 29, 2017 •

edited

Loading

tkelman Jun 29, 2017 •

edited

Loading

tkelman Jun 29, 2017 •

edited

Loading

omus commented Jun 30, 2017 •

edited

Loading

omus commented Jul 1, 2017 •

edited

Loading