Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

upgrade of libgit2 0.27.1 #27241

Merged
merged 2 commits into from
Jun 6, 2018
Merged

upgrade of libgit2 0.27.1 #27241

merged 2 commits into from
Jun 6, 2018

Conversation

simonbyrne
Copy link
Contributor

No description provided.

@ararslan ararslan added libgit2 The libgit2 library or the LibGit2 stdlib module external dependencies Involves LLVM, OpenBLAS, or other linked libraries labels May 24, 2018
@ararslan ararslan requested a review from omus May 24, 2018 19:06
@simonbyrne
Copy link
Contributor Author

I was way too ambitious here: I'm going to need some help with the various patches.

@simonbyrne simonbyrne changed the title WIP: upgrade of libgit2 0.27.0 upgrade of libgit2 0.27.0 Jun 5, 2018
simonbyrne
simonbyrne commented Jun 5, 2018
View reviewed changes
deps/patches/libgit2-bundled_zlib.patch
@@ -1,55 +0,0 @@
From 637f75b2a341df6e308f8a20827dea938a39bd35 Mon Sep 17 00:00:00 2001
From: Valentin Churavy <v.churavy@gmail.com>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vchuravy From what I can tell, the zlib issue now appears to now be fixed upstream (libgit2/libgit2#4403). Could you check this?

simonbyrne
simonbyrne commented Jun 5, 2018
View reviewed changes
deps/patches/libgit2-ssh.patch
@@ -1,23 +0,0 @@
--- CMakeLists.txt 2016-05-26 23:13:48.000000000 -0400
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only one I'm not sure about. I don't quite understand what this patch is trying to do, and the CMake files have changed considerably (some of this has been moved to src/CMakeLists.txt).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

windows ssh clones. test a source build or clean from the buildbot, or this may have broken libgit2 linking to libssh2 and working on private repos

omus
omus reviewed Jun 5, 2018
View reviewed changes
stdlib/LibGit2/src/types.jl
@@ -45,6 +45,9 @@ Matches the [`git_time`](https://libgit2.github.com/libgit2/#HEAD/type/git_time)
struct TimeStruct
time::Int64 # time in seconds from epoch
offset::Cint # timezone offset in minutes
@static if LibGit2.VERSION >= v"0.27.0"
sign::Cchar
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just drop the version check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess theoretically we support multiple libgit2 versions? But yes, we could certainly drop the checks and require 0.27

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the code divergence is small, I think it's better to be more compatible. Building from-source should always grab the version we have in deps/libgit2.version, but distributions are probably quite a ways behind.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me

@omus
Copy link
Member

omus commented Jun 5, 2018

It would be good to add this (libgit2/libgit2#4525) as a patch. That PR should fix an issue which currently emits a warning from the LibGit2 test suite.

@simonbyrne
Copy link
Contributor Author

I'll try it out, but unfortunately it isn't strictly a compatible change since it changes the header files, and libgit2 doesn't seem to provide a way of querying patches.

@simonbyrne
Copy link
Contributor Author

Which warning is it supposed to fix? The only warning which I currently see is

julia/stdlib/LibGit2/test/libgit2.jl

Lines 1835 to 1836 in 2c5aec9

println(stderr, "The following 'Resetting the helper list...' warning is expected:")
@test_broken LibGit2.credential_helpers(cfg, GitCredential("https", "mygithost")) == expected[2]

which this doesn't change.

@simonbyrne simonbyrne changed the title upgrade of libgit2 0.27.0 upgrade of libgit2 0.27.1 Jun 5, 2018
staticfloat
staticfloat approved these changes Jun 6, 2018
View reviewed changes
stdlib/LibGit2/src/types.jl
@@ -45,6 +45,9 @@ Matches the [`git_time`](https://libgit2.github.com/libgit2/#HEAD/type/git_time)
struct TimeStruct
time::Int64 # time in seconds from epoch
offset::Cint # timezone offset in minutes
@static if LibGit2.VERSION >= v"0.27.0"
sign::Cchar
end
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the code divergence is small, I think it's better to be more compatible. Building from-source should always grab the version we have in deps/libgit2.version, but distributions are probably quite a ways behind.

@omus
Copy link
Member

omus commented Jun 6, 2018

With the iteration order patch it should cause the test you mentioned to no longer be broken if this code is removed.

Since this is slightly more involved that I originally expected we can defer this to another PR if you prefer.

@simonbyrne
Copy link
Contributor Author

I think that would be better as another PR.

omus
omus approved these changes Jun 6, 2018
View reviewed changes
Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this. I'm happy that we'll be using their peer reviewed MbedTLS changes even if it is still as a patch.

@simonbyrne simonbyrne merged commit 4a303b3 into master Jun 6, 2018
@simonbyrne simonbyrne deleted the sb/libgit2/0.27 branch June 6, 2018 16:19
@vtjnash
Copy link
Sponsor Member

vtjnash commented Jun 8, 2018

Broke the buildbots: https://build.julialang.org/#/builders/83/builds/1443

Snippets from log that may be relevant:

CMake Warning in tests/CMakeLists.txt:
  The object file directory

    /home/Administrator/buildbot/worker/package_win64/build/deps/scratch/libssh2-30e9c1347e3b8baa2951db612f05e6d87fc8e2f2/tests/CMakeFiles/test_keyboard_interactive_auth_fails_with_wrong_response.dir/

  has 196 characters.  The maximum full path to an object file is 250
  characters (see CMAKE_OBJECT_PATH_MAX).  Object file

    test_keyboard_interactive_auth_fails_with_wrong_response.c.obj

  cannot be safely placed under this directory.  The build may not work
  correctly.
...
-- Checking for module 'libssh2'
--   Found libssh2, version 1.7.0_DEV
...
/usr/bin/ccache x86_64-w64-mingw32-gcc -m64  -D_GNU_SOURCE  -Wall -Wextra -Wno-missing-field-initializers -Wstrict-aliasing -Wstrict-prototypes -Wdeclaration-after-statement -Wshift-count-overflow -Wno-unused-const-variable -Wno-unused-function -O3 -DNDEBUG  -shared -o ../libgit2.dll -Wl,--out-implib,../libgit2.dll.a -Wl,--major-image-version,0,--minor-image-version,27 -Wl,--whole-archive CMakeFiles/git2.dir/objects.a -Wl,--no-whole-archive @CMakeFiles/git2.dir/linklibs.rsp
...
/usr/lib/gcc/x86_64-w64-mingw32/6.4.0/../../../../x86_64-w64-mingw32/bin/ld: cannot find -lssh2
collect2: error: ld returned 1 exit status

(note that the version we built that it should find is 1.8.0)

staticfloat added a commit that referenced this pull request Jun 8, 2018
@staticfloat
Revert "upgrade of libgit2 0.27.1 (#27241)"
7eb8439 
This reverts commit 4a303b3.

This broke the buildbots as it introduced a dependency upon `libssl.so`
again, see discussions:
  * #27241 (comment)
  * 4a303b3#commitcomment-29274618
@staticfloat staticfloat mentioned this pull request Jun 8, 2018
Merged
@staticfloat
Copy link
Sponsor Member

Here's a quick reversion PR to get us back to working nightlies: #27493

staticfloat added a commit that referenced this pull request Jun 8, 2018
@staticfloat
Revert "upgrade of libgit2 0.27.1 (#27241)"
e1872bf 
This reverts commit 4a303b3.

This broke the buildbots as it introduced a dependency upon `libssl.so`
again, see discussions:
  * #27241 (comment)
  * 4a303b3#commitcomment-29274618
@simonbyrne simonbyrne mentioned this pull request Jun 12, 2018
Merged
staticfloat pushed a commit that referenced this pull request Jun 12, 2018
@simonbyrne @staticfloat
upgrade of libgit2 0.27.1 (#27241)
a279a0b 
* upgrade libgit2 to 0.27.1
* fix for API changes
staticfloat pushed a commit that referenced this pull request Jun 12, 2018
@simonbyrne @staticfloat
upgrade of libgit2 0.27.1 (#27241)
1d879be 
* upgrade libgit2 to 0.27.1
* fix for API changes
staticfloat pushed a commit that referenced this pull request Jun 12, 2018
@simonbyrne @staticfloat
upgrade of libgit2 0.27.1 (#27241)
0474363 
* upgrade libgit2 to 0.27.1
* fix for API changes
staticfloat pushed a commit that referenced this pull request Jun 13, 2018
@simonbyrne @staticfloat
upgrade of libgit2 0.27.1 (#27241)
0541bca 
* upgrade libgit2 to 0.27.1
* fix for API changes
@c42f c42f mentioned this pull request Oct 11, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkelman tkelman tkelman left review comments

@staticfloat staticfloat staticfloat approved these changes

@omus omus omus approved these changes

Assignees
No one assigned
Labels
external dependencies Involves LLVM, OpenBLAS, or other linked libraries libgit2 The libgit2 library or the LibGit2 stdlib module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@simonbyrne @omus @vtjnash @staticfloat @tkelman @ararslan