Update to LibGit2 v0.26.0 by omus · Pull Request #22614 · JuliaLang/julia

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

 -{
-    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

+            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

@@ -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

-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

+ 	// 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

            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

                        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

-	$(LIBGIT2_SRC_PATH)/libgit2-mbedtls-writer-fix.patch-applied \
 	$(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 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

		@@ -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

Conversation

omus commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkelman Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkelman Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omus commented Jun 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omus commented Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omus commented Jul 1, 2017

Uh oh!

omus commented Jul 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omus commented Jul 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

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