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.

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

LibGit2: build libgit2 with libssh2 support #17391

Merged
merged 12 commits into from
Jul 16, 2016
Merged

Conversation

wildart
Copy link
Member

@wildart wildart commented Jul 13, 2016

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

@wildart 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 wildart added this to the 0.5.0 milestone Jul 13, 2016
@Keno
Copy link
Member

Keno commented Jul 13, 2016

Love it! Thanks @wildart.

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

LIBSSH2_GIT_URL := git://github.com/wildart/libssh2.git
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@tkelman
Copy link
Contributor

tkelman commented Jul 13, 2016

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
Copy link
Member Author

wildart commented Jul 13, 2016

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

@Keno
Copy link
Member

Keno commented Jul 13, 2016

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, I developed mdebtls support on master

Copy link
Contributor

Choose a reason for hiding this comment

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

can the patch be rebased and still work?

Copy link
Member Author

Choose a reason for hiding this comment

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

i'll look at it

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@wildart
Copy link
Member Author

wildart commented Jul 14, 2016

mdebtls does not have any dependency.

@@ -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 \
Copy link
Contributor

Choose a reason for hiding this comment

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

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

wildart added 5 commits July 14, 2016 11:28
- Linux: OpenSSL & mbedTLS cryptolib for libssh2
- macOS: Native SSL & mbedTLS cryptolib for libssh2
- Windows: Native SSL & cryptolib
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

-Web site: http://www.libssh2.org/
+Web site: https://www.libssh2.org/
Copy link
Contributor

@tkelman tkelman Jul 14, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

hopefully it will be merged into libssh2 master

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@wildart wildart Jul 14, 2016

Choose a reason for hiding this comment

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

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.

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

tkelman commented Jul 15, 2016

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
Copy link
Member Author

wildart commented Jul 15, 2016

@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

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

tkelman commented Jul 15, 2016

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 tkelman added the needs docs Documentation for this change is required label Jul 16, 2016
@tkelman
Copy link
Contributor

tkelman commented Jul 16, 2016

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
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether we should add CMAKE_PREFIX_PATH to CMAKE_COMMON

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

@wildart
Copy link
Member Author

wildart commented Jul 16, 2016

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
Copy link
Contributor

tkelman commented Jul 16, 2016

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

tkelman added 2 commits July 15, 2016 23:58
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 tkelman force-pushed the art/pkg-lib-openssl branch from de22afc to 90c296d Compare July 16, 2016 07:00
@tkelman tkelman removed the needs docs Documentation for this change is required label Jul 16, 2016
$(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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

tkelman commented Jul 16, 2016

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ping?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@tkelman tkelman mentioned this pull request Jul 16, 2016
@davidanthoff
Copy link
Contributor

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("[email protected]:davidanthoff/Judyp.jl.git")
INFO: Cloning Judyp from [email protected]:davidanthoff/Judyp.jl.git
Private key location for '[email protected]' [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
Copy link
Member

Keno commented Jul 16, 2016

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
Copy link
Member

Keno commented Jul 16, 2016

I'll try to make the agent thing work.

@tkelman
Copy link
Contributor

tkelman commented Jul 16, 2016

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

@Keno
Copy link
Member

Keno commented Jul 16, 2016

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.

@tkelman tkelman merged commit 9b8e9d4 into master Jul 16, 2016
@davidanthoff
Copy link
Contributor

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 tkelman deleted the art/pkg-lib-openssl branch July 16, 2016 21:37
@tkelman
Copy link
Contributor

tkelman commented Jul 16, 2016

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

@davidanthoff
Copy link
Contributor

@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
Copy link
Contributor

tkelman commented Jul 18, 2016

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building Build system, or building Julia or its dependencies libgit2 The libgit2 library or the LibGit2 stdlib module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants