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

Bump LibGit2 to 1.0.0 #638

Merged
merged 3 commits into from
Jul 13, 2020
Merged

Bump LibGit2 to 1.0.0 #638

merged 3 commits into from
Jul 13, 2020

Conversation

nalimilan
Copy link
Member

This version uses an internal library for HTTP proxies by default rather than LibCURL.

@fredrikekre
Copy link
Member

This version uses an internal library for HTTP proxies

FYI, a number of users have reported this to be buggy see this comment and below: JuliaLang/julia#33111 (comment)

@nalimilan
Copy link
Member Author

OK. We can force using LibCURL if it's better.

The failures are due to this:

[09:32:40] In file included from /workspace/srcdir/libgit2/deps/ntlmclient/crypt.h:15:0,
[09:32:40]                  from /workspace/srcdir/libgit2/deps/ntlmclient/ntlm.h:14,
[09:32:40]                  from /workspace/srcdir/libgit2/deps/ntlmclient/ntlm.c:20:
[09:32:40] /workspace/srcdir/libgit2/deps/ntlmclient/crypt_mbedtls.h:12:24: fatal error: mbedtls/md.h: No such file or directory
[09:32:40]  #include "mbedtls/md.h"
[09:32:40]                         ^
[09:32:40] compilation terminated.
[09:32:40] make[2]: *** [deps/ntlmclient/CMakeFiles/ntlmclient.dir/build.make:63: deps/ntlmclient/CMakeFiles/ntlmclient.dir/ntlm.c.o] Error 1
[09:32:40] make[2]: Leaving directory '/workspace/srcdir/libgit2/build'
[09:32:40] make[1]: *** [CMakeFiles/Makefile2:365: deps/ntlmclient/CMakeFiles/ntlmclient.dir/all] Error 2

@diabonas
Copy link

The Julia issue with LibGit2 0.99.0 should be fixed by JuliaLang/julia#35232.

@giordano
Copy link
Member

I honestly don't know how this is going to work: if we merge this, all JLL packages depending on libgit2 will pull v0.99.0, but the fix in JuliaLang/julia#35232 has a static check on whatever libgit2 version was used to build julia, not the runtime one. Maybe @staticfloat have some insights on this, especially given his work in JuliaLang/julia#35193

@diabonas
Copy link

For the Julia PR, I used the same version check structure for LibGit2 that was already present elsewhere in that file, e.g. in line 48. If there is a better way to do this, please feel free to weigh in in the comments of this PR: I honestly don't know enough about Julia to gauge whether modifying the struct based on the runtime version check would be possible, I'm just trying to get Julia working with LibGit2 0.99.0 in Arch Linux.

@giordano
Copy link
Member

The failures are due to this:

I fixed that, but now it'll fail on the final step because build script doesn't link to libmbedtls. I couldn't find an easy way to tell cmake to link the library to mbedtls, will need to dig deeper 😞

@giordano
Copy link
Member

macOS and Windows worked, but we need to fix this warning for macOS:

┌ Warning: Depending on /usr/lib/libiconv.2.dylib is known to cause problems at runtime, make sure to link against the JLL library instead
└ @ BinaryBuilder /depot/packages/BinaryBuilder/4fMlX/src/Auditor.jl:299

@nalimilan
Copy link
Member Author

nalimilan commented Mar 24, 2020

Apparently we can set the MBEDTLS_ROOT_DIR environment variable to point to MbedTLS. But I'm not familiar enough with BinaryBuidler to know what's the path... Also I wonder why it wasn't needed in previous releases.

@nalimilan
Copy link
Member Author

Any idea what could be done?

@giordano
Copy link
Member

It's not that MbedTLS is not found, but none of the libraries in our MbedTLS_jll package defines the function mbedtls_md4_init.

@giordano
Copy link
Member

Ok, MbedTLS doesn't build MD4 by default: https://github.com/ARMmbed/mbedtls/blob/e62bdefce164cbdd44bcc8cacff5ee7d92819c40/include/mbedtls/config.h#L2684-L2699
The question is: why libgit2 insists on using it?

# If we're on Linux or FreeBSD, explicitly ask for mbedTLS instead of OpenSSL.
# Disable NTLM because it requires MD4, which is not provided by our build
# of MbedTLS, as it's considered unsafe.
BUILD_FLAGS+=(-DUSE_HTTPS=mbedTLS -DSHA1_BACKEND=CollisionDetection -DCMAKE_INSTALL_RPATH="\$ORIGIN" -DUSE_NTLMCLIENT=OFF)
Copy link
Member

Choose a reason for hiding this comment

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

I'm disabling NTLM for Linux and FreeBSD for the reason explained above, should we disable it everywhere for consistency?

Copy link
Member

Choose a reason for hiding this comment

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

@aviks pointed out that ntlm might be necessary for some corporate users behind proxy

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 guess it could make sense to have it on Windows since most users of NTLM are probably there.

Anyway let's make a decision as it's too bad to block this PR just because of this. If we're unsure, we can also disable NTLM on all OSes since it wasn't supported in previous releases: there would be no regression.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should enable MD4 and NTLM on all platforms for consistency. If someone uses NTLM auth then yes, it is likely they are using Windows, but we also want them to be able to use their Linux CI servers to auth in the same way, for instance.

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, I've tried doing that at #911. MbedTLS appears to have a nonstandard configuration system where you need to edit a header file directly... :-/

@nalimilan
Copy link
Member Author

Thanks! I have no idea about whether NTLM is necessary.

Actually, libgit2 1.0.0 was published two weeks ago, so we should probably use that.

@staticfloat
Copy link
Member

if we merge this, all JLL packages depending on libgit2 will pull v0.99.0, but the fix in JuliaLang/julia#35232 has a static check on whatever libgit2 version was used to build julia, not the runtime one.

The version of LibGit2 that Julia uses will always be the one that ships with Julia, so the static build-time checks like this fix will always work. Until the JLL stdlib work is merged, it is possible for the JLL version you download for a dependency (like HTTP.jl) to not match what was used to build Julia. We can define version bounds (e.g. LibGit2_jll 1.0.0 is only compatible with Julia 1.4+) to help restrict that a bit, but in general we kind of just hope that they are compatible enough that it won't matter.

@giordano giordano mentioned this pull request May 28, 2020
@giordano giordano changed the title Bump LibGit2 to 0.99.0 Bump LibGit2 to 1.0.0 May 28, 2020
@giordano giordano force-pushed the nl/LibGit2-0.99.0 branch 3 times, most recently from 61c52fb to 4d5daca Compare June 1, 2020 18:43
nalimilan and others added 3 commits July 4, 2020 01:15
This version uses an internal library for HTTP proxies by default rather than LibCURL.
@giordano giordano force-pushed the nl/LibGit2-0.99.0 branch from 4d5daca to a611301 Compare July 4, 2020 00:16
@nalimilan
Copy link
Member Author

Can this be merged now? :-)

@giordano giordano merged commit 542eabe into master Jul 13, 2020
@giordano giordano deleted the nl/LibGit2-0.99.0 branch July 13, 2020 12:31
@giordano
Copy link
Member

For the time being I've restricted LibGit2_jll v1 to Julia v1.7: JuliaRegistries/General#17854. If it'll be added to v1.6 already, please remember to update the compat bound accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants