-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Conversation
Love it! Thanks @wildart. |
@@ -0,0 +1,56 @@ | |||
## libssh2 | |||
|
|||
LIBSSH2_GIT_URL := git://github.com/wildart/libssh2.git |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Great, this does look promising. |
For some reason |
Can reproduce locally by doing |
@@ -0,0 +1,2 @@ | |||
LIBSSH2_BRANCH=master | |||
LIBSSH2_SHA1=7934c9ce2a029c43e3642a492d3b9e494d1542be |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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 \ |
There was a problem hiding this comment.
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
- 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 |
There was a problem hiding this comment.
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 -
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
248b7d5
to
d732b1f
Compare
the revised BSD license. | ||
|
||
-Web site: http://www.libssh2.org/ | ||
+Web site: https://www.libssh2.org/ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
d732b1f
to
7f70ecc
Compare
7f70ecc
to
ea3cfb4
Compare
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. |
@tkelman Travis directory caching screws this PR build. |
c918839
to
ec79f6f
Compare
you crossed it out, but 32 and 64 bit use different caches because they set different values for |
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
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. |
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
|
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
de22afc
to
90c296d
Compare
$(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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
I just tried this with julia-0.5.0-90c296dbb9-win64.exe from above. This is what I get:
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 I didn't test anything else yet. |
This basically works for me, except that instead of prompting for my ssh key, it should respect |
I'll try to make the agent thing work. |
what's better, merging and revising it on master, or tweaking inside this pr? |
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. |
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. |
though now it counts as a bugfix and not a new feature so we aren't blocked from feature freeze |
Kind of a bummer that the mbedtls config patch makes its tests fail ( |
Build
libgit2
withlibssh2
supportFixes #16041, see also #17246
@Keno @tkelman