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

Source build of curl to enable proxy support for Pkg #17783

Merged
merged 4 commits into from
Aug 7, 2016
Merged

Conversation

ViralBShah
Copy link
Member

@ViralBShah ViralBShah commented Aug 3, 2016

Using libcurl with libgit2 will allow the use of proxy servers for those behind firewalls.

Should fix #13472 #10754

@ViralBShah ViralBShah added the packages Package management and loading label Aug 3, 2016
$(BUILDDIR)/curl-$(CURL_VER)/config.status: $(SRCDIR)/srccache/curl-$(CURL_VER)/configure
mkdir -p $(dir $@)
cd $(dir $@) && \
$< $(CONFIGURE_COMMON) --includedir=$(build_includedir) --with-mbedtls=$(build_shlibdir) CFLAGS="$(CFLAGS) $(CURL_CFLAGS)" LDFLAGS="$(LDFLAGS) $(CURL_LDFLAGS)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you may need to also do --disable-ssl or something like that to get it to not link against openssl?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't seem to be linking against openssl. See my otool output below.

@tkelman tkelman added the building Build system, or building Julia or its dependencies label Aug 3, 2016
@tkelman
Copy link
Contributor

tkelman commented Aug 3, 2016

Do we have a test system with a proxy set up somewhere that we can run this on?

@ViralBShah
Copy link
Member Author

Seems like everything is building and linking ok:

viral-laptop 07:43:29 (vs/curl) ~/julia/usr/lib$ otool -L libgit2.dylib 
libgit2.dylib:
    @rpath/libgit2.dylib (compatibility version 24.0.0, current version 0.24.0)
    @rpath/libcurl.dylib (compatibility version 9.0.0, current version 9.0.0)
    /usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.5)
    /System/Library/Frameworks/Security.framework/Versions/A/Security (compatibility version 1.0.0, current version 57337.60.2)
    /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1258.1.0)
    @rpath/libssh2.dylib (compatibility version 1.0.0, current version 1.0.1)
    /usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1226.10.1)
viral-laptop 07:43:44 (vs/curl) ~/julia/usr/lib$ otool -L libcurl.dylib 
libcurl.dylib:
    @rpath/libcurl.dylib (compatibility version 9.0.0, current version 9.0.0)
    @rpath/libssh2.dylib (compatibility version 1.0.0, current version 1.0.1)
    @rpath/libmbedtls.dylib (compatibility version 10.0.0, current version 2.3.0)
    @rpath/libmbedx509.dylib (compatibility version 0.0.0, current version 2.3.0)
    @rpath/libmbedcrypto.dylib (compatibility version 0.0.0, current version 2.3.0)
    /System/Library/Frameworks/LDAP.framework/Versions/A/LDAP (compatibility version 1.0.0, current version 2.4.0)
    /usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.5)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1226.10.1)

@ViralBShah
Copy link
Member Author

@mdpradeep I believe you had a test system with proxy setup. Could you test this PR?

I also believe I can setup a SOCKS proxy using ssh -D and try. Does anyone have thoughts if this could be a good way to test?

@ViralBShah
Copy link
Member Author

@tkelman Does this PR need anything further before asking others to try it out?

@@ -272,6 +276,7 @@ include $(SRCDIR)/patchelf.mk
include $(SRCDIR)/mbedtls.mk
include $(SRCDIR)/libssh2.mk
include $(SRCDIR)/libgit2.mk
include $(SRCDIR)/curl.mk
Copy link
Contributor

Choose a reason for hiding this comment

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

should be done before libgit2, and libgit2 will need to declare a dependency on it

Copy link
Member Author

@ViralBShah ViralBShah Aug 3, 2016

Choose a reason for hiding this comment

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

Good point. I will move it above.

libgit2 and curl are in different build stages. Shouldn't that be good enough? I do think that libssh2 should probably be stage 1 (currently in 2), curl in stage 2 and libgit2 in stage 3.

@tkelman
Copy link
Contributor

tkelman commented Aug 3, 2016

I don't think we should rush merging this into rc1, but people can certainly test the branch.

Minor things where it'll need mentioning:

  • license
  • readme
  • JL_PRIVATE_LIBS
  • the getall target
  • the list of # autoconf configure-driven scripts:
  • tests, libgit2 features probably?

@ViralBShah
Copy link
Member Author

Yes, this is definitely not for rc1, but if we get all the testing done, I hope we can merge it into rc2. If we can confirm success on all the platforms, then doing the rest of these items should be straightforward.

@@ -7,7 +7,7 @@ $(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) -DCMAKE_BUILD_TYPE=Release -DTHREADSAFE=ON
LIBGIT2_OPTS := $(CMAKE_COMMON) -DCMAKE_BUILD_TYPE=Release -DTHREADSAFE=ON -DCURL_INCLUDE_DIRS=$(build_includedir) -DCURL_LIBRARIES="-L$(build_shlibdir) -lcurl"
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 entirely works

  Manually-specified variables were not used by the project:

    CMAKE_CXX_COMPILER
    CMAKE_CXX_COMPILER_ARG1
    CURL_INCLUDE_DIRS
    CURL_LIBRARIES

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it getting auto-detected? How do I provide those variables to the build?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/libgit2/libgit2/blob/152efee20b74ea261cf8e05410a110687e17376e/CMakeLists.txt#L277-L280 - it uses PKG_CHECK_MODULES which means it's relying on pkg-config to do the finding. I don't think we've been explicitly requiring or checking pkg-config as a build dependency though.

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 believe pkg-config is getting used in many other places. For now, should I just remove these variables and we should check if everything works on other OSes.

Copy link
Contributor

Choose a reason for hiding this comment

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

almost entirely optionally though. I've had to remove it in the past to get a multilib build of 32 bit linux Julia to work from 64 bit linux

Copy link
Member

@wildart wildart Aug 3, 2016

Choose a reason for hiding this comment

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

pkg-config should work if libcurl pc-file is correctly installed in lib/pkg-config, no need for CURL_INCLUDE_DIRS and CURL_LIBRARIES.
However, pkg-config incorrectly works on msys2, thus the above patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we need pkg-config to be present to get a correct build, then we should be checking for it and failing the build if it isn't (and documenting this). Can we get away without it?

Copy link
Member

Choose a reason for hiding this comment

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

no, unless libcurl generates cmake installation configs as libssh2 does.

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 should mention that I do not get the unused variables during cmake that you posted above. I just get the first two but not the curl ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's on windows - libgit2 won't use curl there if it's using winhttp

@ViralBShah ViralBShah added this to the 0.5.x milestone Aug 3, 2016
@mdpradeep
Copy link
Contributor

Will check it.

On 03-Aug-2016, at 8:26 PM, Viral B. Shah [email protected] wrote:

@mdpradeep https://github.com/mdpradeep I believe you had a test system with proxy setup. Could you test this PR?

I also believe I can setup a SOCKS proxy using ssh -D and try. Does anyone have thoughts if this could be a good way to test?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #17783 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AMbi43aQImm5LIzl3a0rge_HRBqdYheTks5qcKwYgaJpZM4Jbq1o.

@ViralBShah
Copy link
Member Author

ViralBShah commented Aug 3, 2016

I set up a local socks proxy with ssh -v -D localhost:2222 localhost, and then exported ALL_PROXY=socks5h://localhost:2222. Starting Julia and doing Pkg.update() then did show connections being routed through the proxy - which suggests that this works at least on the mac.

Killing the proxy while still keeping the environment variable gives me the right expected error:

julia> Pkg.update()
INFO: Updating METADATA...
ERROR: METADATA cannot be updated. Resolve problems manually in /Users/viral/.julia/v0.5/METADATA.
GitError(Code:ERROR, Class:Net, curl error: Failed to connect to localhost port 2222: Connection refused
)

Getting rid of the environment variable then again gets me everything working without the proxy.

@ViralBShah ViralBShah modified the milestones: 0.5.0, 0.5.x Aug 3, 2016
@ViralBShah ViralBShah added the priority This should be addressed urgently label Aug 3, 2016
@ViralBShah
Copy link
Member Author

Seems like we need 4 build stages:

libgit2, which needs libcurl, which needs libssh2, which needs mbedtls.

@ViralBShah ViralBShah force-pushed the vs/curl branch 2 times, most recently from 985b193 to f75194f Compare August 3, 2016 18:07
@ViralBShah ViralBShah changed the title WIP: Source build of curl linked to libmbedtls WIP: Source build of curl to enable proxy support for Pkg Aug 3, 2016
@ViralBShah ViralBShah added libgit2 The libgit2 library or the LibGit2 stdlib module and removed packages Package management and loading labels Aug 3, 2016
@ViralBShah ViralBShah changed the title WIP: Source build of curl to enable proxy support for Pkg Source build of curl to enable proxy support for Pkg Aug 3, 2016
@ViralBShah
Copy link
Member Author

I prefer not mucking with cmake stuff. If this will get the right builds out, I prefer merging this as it is and cleaning up with your PR later.

If we use libcurl for transport, will the ssh connections also go through it, or does libgit2 handle them directly?

@tkelman
Copy link
Contributor

tkelman commented Aug 5, 2016

If we use libcurl for transport, will the ssh connections also go through it, or does libgit2 handle them directly?

I don't know the answer to that. Do ssh remotes go through a proxy? To test this, you could try the following

make -C deps distclean-libgit2 distclean-mbedtls distclean-libssh2 distclean-curl
make -C deps install-curl # do first so it doesn't link against ssh2, check its configure output to be sure
make -C deps install-libgit2

@ViralBShah
Copy link
Member Author

ViralBShah commented Aug 5, 2016

ssh remotes are not expected to go through proxies. I think this addresses the case at hand, of using http(s) and socks proxies. Let's get it in for the next RC.

@ViralBShah ViralBShah force-pushed the vs/curl branch 2 times, most recently from 480bcea to 6b11fd5 Compare August 6, 2016 09:12
@@ -267,7 +267,7 @@ Input AST
``a:b`` ``(: a b)``
``a:b:c`` ``(: a b c)``
``a,b`` ``(tuple a b)``
``a==b`` ``(comparison a == b)``
``a==b`` ``(call == a b)``
Copy link
Contributor

Choose a reason for hiding this comment

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

messed up rebase, why is this in here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Crap. How do I fix this?

Copy link
Contributor

Choose a reason for hiding this comment

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

git rebase -i origin/master and deselect spencer's commit, then force push over this vs/curl branch

@@ -297,7 +299,7 @@ For a longer overview of Julia's dependencies, see these [slides](https://github
[gcc]: http://gcc.gnu.org
[clang]: http://clang.llvm.org
[gfortran]: https://gcc.gnu.org/fortran/
[curl]: http://curl.haxx.se
[cURL]: http://curl.haxx.se
Copy link
Contributor

Choose a reason for hiding this comment

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

link should probably be in lower case all 3 places otherwise the "automatically download external libraries" might not link correctly

@ViralBShah
Copy link
Member Author

Fixed the rpath stuff. Simplified the curl build dependency in libgit2 - and only gets done on non-windows. The Curl build now depends on mbedtls and libssh2.

Now parallel make correctly builds all the dependencies in the right after.

@ViralBShah
Copy link
Member Author

The CURL_LDFLAGS := $(RPATH_ORIGIN) works fine on os x, but on linux, I get:

/usr/bin/ld: cannot find origin: No such file or directory

@ViralBShah
Copy link
Member Author

RPATH_ESCAPED_ORIGIN works.

The curl build depends on mbedtls and libssh2
Patch for mbedtls to allow curl to build with mbedtls support
Add curl to LICENSE.md, README.md and all the other various locations
Enforce curl as a dependency for the libgit2 build.
Fix the libgit2 clean target
Build curl only on linux and osx.
Add pkg-config to build dependencies in README.md. Required to detect curl in libgit2 build.
@ViralBShah
Copy link
Member Author

Got rid of the extra commit that had snuck in.

Also tested successfully again on linux and os x.

@tkelman tkelman merged commit 174dc5f into master Aug 7, 2016
@tkelman tkelman deleted the vs/curl branch August 7, 2016 11:11
@ViralBShah
Copy link
Member Author

Do we have the nightlies with curl yet? We should ping all those who reported the proxy issue to test the nightlies and confirm, and that should be good enough to get it into the RC2.

tkelman pushed a commit that referenced this pull request Aug 20, 2016
(cherry picked from commit a434002)
ref #17783
tkelman pushed a commit that referenced this pull request Aug 20, 2016
The curl build depends on mbedtls and libssh2
Patch for mbedtls to allow curl to build with mbedtls support
Add curl to LICENSE.md, README.md and all the other various locations

(cherry picked from commit 046c6eb)
ref #17783
tkelman pushed a commit that referenced this pull request Aug 20, 2016
Enforce curl as a dependency for the libgit2 build.
Fix the libgit2 clean target
Build curl only on linux and osx.
Add pkg-config to build dependencies in README.md. Required to detect curl in libgit2 build.

(cherry picked from commit 3687527)
ref #17783
tkelman pushed a commit that referenced this pull request Aug 20, 2016
vchuravy pushed a commit to JuliaLang/LazyArtifacts.jl that referenced this pull request Oct 2, 2023
The curl build depends on mbedtls and libssh2
Patch for mbedtls to allow curl to build with mbedtls support
Add curl to LICENSE.md, README.md and all the other various locations

(cherry picked from commit 9e9e143)
ref JuliaLang/julia#17783
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 needs tests Unit tests are required for this change priority This should be addressed urgently
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libcurl problems with libgit2 on master
8 participants