LibGit2: build libgit2 with libssh2 support #17391

wildart · 2016-07-13T00:40:21Z

Build libgit2 with libssh2 support

Linux: SSL: OpenSSL, SSH: libssh2 +mbedTLS
macOS: SSL: Native, SSH: libssh2 + mbedTLS
Windows: SSL: Native, SSH: libssh2 +Native cryptolib

Fixes #16041, see also #17246

@Keno @tkelman

Keno · 2016-07-13T00:46:41Z

Love it! Thanks @wildart.

tkelman · 2016-07-13T00:47:59Z

deps/libssh2.mk

@@ -0,0 +1,56 @@
+## libssh2
+
+LIBSSH2_GIT_URL := git://github.com/wildart/libssh2.git


can we structure this as a patch file instead of using your repo?

tkelman · 2016-07-13T00:55:45Z

Great, this does look promising. ~~We'll need to mention libssh and mbedtls in the readme and license along with the other deps.~~ done

wildart · 2016-07-13T19:11:08Z

For some reason mbedtls linking fails on CI services. Any thoughts?

Keno · 2016-07-13T19:20:59Z

Can reproduce locally by doing -j8. Missing dependency somewhere?

tkelman · 2016-07-13T19:28:25Z

deps/libssh2.version

@@ -0,0 +1,2 @@
+LIBSSH2_BRANCH=master
+LIBSSH2_SHA1=7934c9ce2a029c43e3642a492d3b9e494d1542be


can we use https://github.com/libssh2/libssh2/releases/tag/libssh2-1.7.0 or is there something on master we need?

nope, I developed mdebtls support on master

can the patch be rebased and still work?

i'll look at it

I can also probably take a look at rebasing it but I'm not sure what to test

wildart · 2016-07-14T12:58:44Z

mdebtls does not have any dependency.

tkelman · 2016-07-14T15:27:43Z

deps/mbedtls.mk

@@ -15,7 +15,7 @@ MBEDTLS_OBJ_TARGET := $(build_shlibdir)/libmbedtls.$(SHLIB_EXT) \
 			$(build_shlibdir)/libmbedcrypto.$(SHLIB_EXT)

 MBEDTLS_OPTS := $(CMAKE_COMMON) -DUSE_SHARED_MBEDTLS_LIBRARY=ON \
-		-DENABLE_PROGRAMS=OFF -DCMAKE_BUILD_TYPE=Release \
+		-DENABLE_PROGRAMS=OFF -DENABLE_TESTING=OFF -DCMAKE_BUILD_TYPE=Release \


would be ideal if we can get these to work, but I guess that can be done during the RC period if it isn't working yet

- Linux: OpenSSL & mbedTLS cryptolib for libssh2 - macOS: Native SSL & mbedTLS cryptolib for libssh2 - Windows: Native SSL & cryptolib

tkelman · 2016-07-14T15:28:14Z

deps/mbedtls.mk

@@ -49,7 +49,7 @@ $(MBEDTLS_OBJ_SOURCE): $(BUILDDIR)/mbedtls-$(MBEDTLS_VER)/Makefile

 $(BUILDDIR)/mbedtls-$(MBEDTLS_VER)/checked: $(MBEDTLS_OBJ_SOURCE)
 ifeq ($(OS),$(BUILD_OS))
-	$(MAKE) -C $(dir $@) test
+	-$(MAKE) -C $(dir $@) test


I don't think this rule gets run by default unless you specifically request it, so please don't prefix it with -

I'm testing mbedtls build: test programs where linked with errors

so we should see the errors instead of skipping them if you manually run make -C deps test-mbedtls, no?

tkelman · 2016-07-14T15:30:21Z

deps/patches/libssh2-mbedtls.patch

+ the revised BSD license.
+
+-Web site: http://www.libssh2.org/
+Web site: https://www.libssh2.org/


we should really make this patch as small as it can be and still work, but that's more "after it works" cleanup we can do during RC's

hopefully it will be merged into libssh2 master

Yes hopefully it will, we haven't heard much there though right? If I had to guess, maybe the reason I wasn't able to get it to pass tests was because I was trying to use stock unmodified mbedtls instead of applying the config patch?

yes, by default, mbedtls internal buffers for bignum manipulations are small. This need to be tested thoroughly and we would need proper custom configuration as well.

tkelman · 2016-07-15T01:30:31Z

If / when this is ready, let me know and I'll trigger a buildbot run off the branch to see if it works there too.

wildart · 2016-07-15T15:12:48Z

@tkelman Travis directory caching screws this PR build. ~~It looks like x86 and x86_64 builds use the same $TRAVIS_BUILD_DIR/deps/build and $TRAVIS_BUILD_DIR/deps/srccache folders~~

tkelman · 2016-07-15T17:51:21Z

you crossed it out, but 32 and 64 bit use different caches because they set different values for compiler. We may need to occasionally clear the cache for this pr if it has uploaded its own, to make it go back to using the master cache. Please don't delete the master cache though, that would take an hour or two on travis to rebuild.

tkelman · 2016-07-16T05:58:02Z

I can never get ssh to work with git so don't personally use it and can't easily test with it here (testing help appreciated), but at least the existing functionality seems to work even in a minimal docker container so maybe we don't need to worry about the zlib thing on Linux.

I'll add the readme changes. If you had to guess, what do you think the minimum versions of libssh and mbedtls would be? In case some distro were to try building against something earlier? Of course if you're using system libssh you wouldn't need to use mbedtls, but maybe you could try to use our source build of libssh against an older system mbedtls?

not being used yet in this PR

tkelman · 2016-07-16T06:45:56Z

deps/libgit2.mk

@@ -7,9 +7,9 @@ $(eval $(call git-external,libgit2,LIBGIT2,CMakeLists.txt,build/libgit2.$(SHLIB_
 LIBGIT2_OBJ_SOURCE := $(BUILDDIR)/$(LIBGIT2_SRC_DIR)/libgit2.$(SHLIB_EXT)
 LIBGIT2_OBJ_TARGET := $(build_shlibdir)/libgit2.$(SHLIB_EXT)

-LIBGIT2_OPTS := $(CMAKE_COMMON) -DTHREADSAFE=ON
+LIBGIT2_OPTS := $(CMAKE_COMMON) -DCMAKE_BUILD_TYPE=Release -DTHREADSAFE=ON -DCMAKE_PREFIX_PATH=$(build_prefix)


I wonder whether we should add CMAKE_PREFIX_PATH to CMAKE_COMMON

wildart · 2016-07-16T06:50:20Z

libssh 1.7.0, mbedtls 2.2.1, libgit2 0.24 - we need to build them all, because there is no guaranty that system libraries will provide required feature set.

tkelman · 2016-07-16T06:53:22Z

what's the required feature set that wouldn't be in libssh 1.6 or mbedtls 2.1?

oops, reason to care about msys2 = appveyor, will fix

CMake Error at tests/CMakeLists.txt:116 (message):
  Could not create symbolic link for:
  C:/projects/julia/deps/srccache/mbedtls-2.2.1-gpl/tests/data_files -->
  Invalid switch - "projects".
-- Configuring incomplete, errors occurred!
See also "C:/projects/julia/deps/build/mbedtls-2.2.1/CMakeFiles/CMakeOutput.log".
/c/projects/julia/deps/mbedtls.mk:36: recipe for target 'build/mbedtls-2.2.1/Makefile' failed
make[1]: *** [build/mbedtls-2.2.1/Makefile] Error 1
make[1]: *** Waiting for unfinished jobs....

make cleaning and installation a bit more uniform Move CMAKE_PREFIX_PATH to CMAKE_COMMON

add 1.7 and 2.2 as minimum libssh2 and mbedtls versions, respectively

tkelman · 2016-07-16T07:04:19Z

deps/mbedtls.mk

-			$(build_shlibdir)/libmbedx509.$(SHLIB_EXT) \
-			$(build_shlibdir)/libmbedcrypto.$(SHLIB_EXT)
+MBEDTLS_OBJ_SOURCE := $(BUILDDIR)/mbedtls-$(MBEDTLS_VER)/library/libmbedcrypto.$(SHLIB_EXT)
+MBEDTLS_OBJ_TARGET := $(build_shlibdir)/libmbedcrypto.$(SHLIB_EXT)


just for clarification purposes, is this the only one of the three that libssh2 needs, or does it actually need all 3?

ping. not critical, but for the record would be good to have answers written down to these last minor clarifications while it's fresh in memory

I left only one, because if I put three and use this variable as dependency in other Makefiles (libssh2) then make builds it 3 times.

tkelman · 2016-07-16T07:11:03Z

As long as my latest little tweaks are green on CI, I think this is done. If someone can independently confirm that using a package over ssh works with this branch or the binaries posted above, I think we can merge.

tkelman · 2016-07-16T07:21:20Z

deps/patches/libgit2-ssh.patch

+ # Optional external dependency: libssh2
+ IF (USE_SSH)
+-	PKG_CHECK_MODULES(LIBSSH2 libssh2)
+	FIND_PACKAGE(Libssh2)


what was the story and/or link on whether upstream needs or wants this patch?

same thing - MSYS2, pkg-config incorrectly reports installation location of libssh2

thanks. if this is yet another msys2 specific issue I'm going to have to migrate appveyor over to cross building from cygwin.

davidanthoff · 2016-07-16T21:15:01Z

I just tried this with julia-0.5.0-90c296dbb9-win64.exe from above. This is what I get:

   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.5.0-dev+5445 (2016-07-16 06:59 UTC)
 _/ |\__'_|_|_|\__'_|  |  master/90c296d (fork: 12 commits, 2 days)
|__/                   |  x86_64-w64-mingw32

julia> Pkg.clone("git@github.com:davidanthoff/Judyp.jl.git")
INFO: Cloning Judyp from git@github.com:davidanthoff/Judyp.jl.git
Private key location for 'git@github.com' [C:\Users\david\.ssh\id_rsa]:
Passphrase for C:\Users\david\.ssh\id_rsa:INFO: Computing changes...
INFO: No packages to install, update or remove

julia> Pkg.update()
INFO: Updating METADATA...
INFO: Computing changes...
INFO: No packages to install, update or remove
ERROR: Update finished with errors.
=> Package Judyp cannot be updated.
GitError(Code:ERROR, Class:SSH, error authenticating: failed connecting agent)
 in macro expansion at .\libgit2\error.jl:98 [inlined]
 in #fetch#52(::Base.LibGit2.FetchOptions, ::String, ::Function, ::Base.LibGit2.GitRemote, ::Array{AbstractString,1}) at .\libgit2\remote.jl:70
 in (::Base.LibGit2.#kw##fetch)(::Array{Any,1}, ::Base.LibGit2.#fetch, ::Base.LibGit2.GitRemote, ::Array{AbstractString,1}) at .\<missing>:0
 in #fetch#91(::String, ::String, ::Array{AbstractString,1}, ::Nullable{Base.LibGit2.CachedCredentials}, ::Function, ::Base.LibGit2.GitRepo) at .\libgit2\libgit2.jl:159
 in (::Base.LibGit2.#kw##fetch)(::Array{Any,1}, ::Base.LibGit2.#fetch, ::Base.LibGit2.GitRepo) at .\<missing>:0
 in (::Base.Pkg.Entry.##39#45)(::Base.LibGit2.GitRepo) at .\pkg\entry.jl:425
 in with(::Base.Pkg.Entry.##39#45, ::Base.LibGit2.GitRepo) at .\libgit2\types.jl:660
 in update(::String, ::Set{String}) at .\pkg\entry.jl:415
 in (::Base.Pkg.Dir.##2#3{Array{Any,1},Base.Pkg.Entry.#update,Tuple{String,Set{String}}})() at .\pkg\dir.jl:31
 in cd(::Base.Pkg.Dir.##2#3{Array{Any,1},Base.Pkg.Entry.#update,Tuple{String,Set{String}}}, ::String) at .\file.jl:48
 in #cd#1(::Array{Any,1}, ::Function, ::Function, ::String, ::Vararg{Any,N}) at .\pkg\dir.jl:31
 in update() at .\pkg\pkg.jl:210
 in eval(::Module, ::Any) at .\boot.jl:234
 in eval_user_input(::Any, ::Base.REPL.REPLBackend) at .\REPL.jl:62
 in macro expansion at .\REPL.jl:92 [inlined]
 in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at .\event.jl:46
 in update(::String, ::Set{String}) at .\pkg\entry.jl:459
 in (::Base.Pkg.Dir.##2#3{Array{Any,1},Base.Pkg.Entry.#update,Tuple{String,Set{String}}})() at .\pkg\dir.jl:31
 in cd(::Base.Pkg.Dir.##2#3{Array{Any,1},Base.Pkg.Entry.#update,Tuple{String,Set{String}}}, ::String) at .\file.jl:48
 in #cd#1(::Array{Any,1}, ::Function, ::Function, ::String, ::Vararg{Any,N}) at .\pkg\dir.jl:31
 in update() at .\pkg\pkg.jl:210
 in eval(::Module, ::Any) at .\boot.jl:234
 in macro expansion at .\REPL.jl:92 [inlined]
 in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at .\event.jl:46

julia>

Apart from the error, it also shouldn't ask for a passphrase, my ssh key doesn't have one. And is the private key location cached somewhere?

And there should be a line break after I enter the passphrase and the next log message (i.e. the line Passphrase for C:\Users\david\.ssh\id_rsa:INFO: Computing changes... should be split in two).

I didn't test anything else yet.

Keno · 2016-07-16T21:30:30Z

This basically works for me, except that instead of prompting for my ssh key, it should respect SSH_AUTH_SOCK and let the operating system handle it.

Keno · 2016-07-16T21:32:56Z

I'll try to make the agent thing work.

tkelman · 2016-07-16T21:36:09Z

what's better, merging and revising it on master, or tweaking inside this pr?

Keno · 2016-07-16T21:36:56Z

Merging and revisiting on master probably. I do consider not having ssh agent support blocking for the release though, since otherwise you have to enter your password every time.

davidanthoff · 2016-07-16T21:37:29Z

This PR doesn't break anything, i.e. SSH doesn't work on master in any case, right? Then I'd vote for merging and then fixing, is probably easier for people to try out in that case because they can just use the nightly build etc.

tkelman · 2016-07-16T21:40:20Z

though now it counts as a bugfix and not a new feature so we aren't blocked from feature freeze

davidanthoff · 2016-07-16T21:40:25Z

@Keno, plus https also doesn't work, minimally on windows (#17423, #17422), so at this moment there is no way to use private repos, regardless of how often one is willing to type in passwords.

tkelman · 2016-07-18T09:01:03Z

Kind of a bummer that the mbedtls config patch makes its tests fail (mpi-suite and rsa-suite), and the libssh2 tests fail without it. Would we need to patch mbedtls' tests too to account for the changes?

wildart added building Build system, or building Julia or its dependencies libgit2 The libgit2 library or the LibGit2 stdlib module labels Jul 13, 2016

wildart added this to the 0.5.0 milestone Jul 13, 2016

tkelman reviewed Jul 13, 2016
View reviewed changes

tkelman reviewed Jul 14, 2016
View reviewed changes

wildart added 5 commits July 14, 2016 11:28

build libgit2 with libssh2 support

5365943

- Linux: OpenSSL & mbedTLS cryptolib for libssh2 - macOS: Native SSL & mbedTLS cryptolib for libssh2 - Windows: Native SSL & cryptolib

fix typo

d1d80e4

get libssh2 from official repo & patch it

c148d43

rename mbedtls patch

8ea2a3c

use release tag for libssh2

e272e1e

tkelman reviewed Jul 14, 2016
View reviewed changes

wildart force-pushed the art/pkg-lib-openssl branch from 248b7d5 to d732b1f Compare July 14, 2016 15:28

tkelman reviewed Jul 14, 2016
View reviewed changes

wildart force-pushed the art/pkg-lib-openssl branch from d732b1f to 7f70ecc Compare July 14, 2016 16:40

tkelman mentioned this pull request Jul 14, 2016

Checklist of steps towards 0.5 release [candidates] #17418

Closed

16 tasks

fix mbedtls build

ea3cfb4

wildart force-pushed the art/pkg-lib-openssl branch from 7f70ecc to ea3cfb4 Compare July 14, 2016 17:51

wildart force-pushed the art/pkg-lib-openssl branch 3 times, most recently from c918839 to ec79f6f Compare July 15, 2016 17:18

Remove many unrelated changes from libssh2-mbedtls.patch

2f2a715

tkelman added the needs docs Documentation for this change is required label Jul 16, 2016

Remove libgit2-custom-tls.patch for now

ff3f12d

not being used yet in this PR

tkelman reviewed Jul 16, 2016
View reviewed changes

tkelman added 2 commits July 15, 2016 23:58

Minor makefile cleanups

442fb0a

make cleaning and installation a bit more uniform Move CMAKE_PREFIX_PATH to CMAKE_COMMON

Add libssh2 and mbedtls to LICENSE and README

90c296d

add 1.7 and 2.2 as minimum libssh2 and mbedtls versions, respectively

tkelman force-pushed the art/pkg-lib-openssl branch from de22afc to 90c296d Compare July 16, 2016 07:00

tkelman removed the needs docs Documentation for this change is required label Jul 16, 2016

tkelman reviewed Jul 16, 2016
View reviewed changes

tkelman mentioned this pull request Jul 16, 2016

SSH support in Pkg #16041

Closed

tkelman merged commit 9b8e9d4 into master Jul 16, 2016

tkelman deleted the art/pkg-lib-openssl branch July 16, 2016 21:37

rsrock mentioned this pull request Jul 17, 2016

build fails on Mac OS X #17460

Closed

wildart mentioned this pull request Jul 17, 2016

Fix ssh-agent use by libgit2 #17459

Merged

tkelman mentioned this pull request Jul 18, 2016

Make libssh stack more robust to rebuilds #17475

Merged

tkelman mentioned this pull request Aug 5, 2016

Julia release-0.5 builds the GPL version of mbedtls vs Apache #17839

Closed

		@@ -0,0 +1,56 @@
		## libssh2

		LIBSSH2_GIT_URL := git://github.com/wildart/libssh2.git

		@@ -0,0 +1,2 @@
		LIBSSH2_BRANCH=master
		LIBSSH2_SHA1=7934c9ce2a029c43e3642a492d3b9e494d1542be

LibGit2: build libgit2 with libssh2 support #17391

LibGit2: build libgit2 with libssh2 support #17391

Conversation

wildart commented Jul 13, 2016 • edited by tkelman Loading

Keno commented Jul 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Jul 13, 2016 • edited Loading

wildart commented Jul 13, 2016

Keno commented Jul 13, 2016

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

wildart commented Jul 14, 2016

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 Jul 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wildart Jul 14, 2016 • edited Loading

Choose a reason for hiding this comment

tkelman commented Jul 15, 2016

wildart commented Jul 15, 2016 • edited Loading

tkelman commented Jul 15, 2016

tkelman commented Jul 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wildart commented Jul 16, 2016

tkelman commented Jul 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Jul 16, 2016

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

davidanthoff commented Jul 16, 2016

Keno commented Jul 16, 2016

Keno commented Jul 16, 2016

tkelman commented Jul 16, 2016

Keno commented Jul 16, 2016

davidanthoff commented Jul 16, 2016

tkelman commented Jul 16, 2016

davidanthoff commented Jul 16, 2016

tkelman commented Jul 18, 2016

wildart commented Jul 13, 2016 •

edited by tkelman

Loading

tkelman commented Jul 13, 2016 •

edited

Loading

tkelman Jul 14, 2016 •

edited

Loading

wildart Jul 14, 2016 •

edited

Loading

wildart commented Jul 15, 2016 •

edited

Loading