-
Notifications
You must be signed in to change notification settings - Fork 280
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
Update libgit2 to 0.27 #645
Conversation
c3fc124
to
228bc8b
Compare
I honestly have no idea about how the Objective-Git build system works. What sticks out to me though is the symlinks to libcrypto.a and libssl.a in the External directory. I think Homebrew has changed the default location of where it installs files to "/usr/local/Cellar", if I'm not mistaken, so these symlinks seem to be out of date. I don't have any idea whether they are important to the build process at all, but it's the first thing I noticed. I can dig into that issue a bit further though if you need me to. |
Also, the bundled version of openssl is heavily out of date and susceptible to various CVEs. Is that thing even used? |
We're doing crazy shenanigans to build like 6 (2 x86/x64 simulators, 4 ARM variants) architectures on iOS and lipo them together so there's only one binary, which incidentally are those .a files (the final FAT libraries for the various dependencies, as we can't use Homebrew). The issue I'm seeing is that the 2nd SSH check performs a compile-link step against the The included OpenSSL is there for the SSH support #648, so "oh-my-god", "yes", are the replies 😢. |
Holy moly. Sounds like building Objective-Git is a lot of fun. We should in fact be upgrading openssl to at least version 1.0.2n, which is not breaking the API in any way. What we're doing is directly using the pkg-config infrastructure via |
As per the docs[1], another set of variables will be provided by PKG_CHECK_MODULES when static linking (with the additional libraries required for static links). We need to pass those libraries to CHECK_LIBRARY_EXISTS so *its* link succeeds. 1 - https://cmake.org/cmake/help/v3.1/module/FindPkgConfig.html#command:pkg_check_modules Ref: libgit2/objective-git#645
As per the docs[1], another set of variables will be provided by PKG_CHECK_MODULES when static linking (with the additional libraries required for static links). We need to pass those libraries to CHECK_LIBRARY_EXISTS so *its* link succeeds. 1 - https://cmake.org/cmake/help/v3.1/module/FindPkgConfig.html#command:pkg_check_modules Ref: libgit2/objective-git#645
As per the docs[1], another set of variables will be provided by PKG_CHECK_MODULES when static linking (with the additional libraries required for static links). We need to pass those libraries to CHECK_LIBRARY_EXISTS so *its* link succeeds. 1 - https://cmake.org/cmake/help/v3.1/module/FindPkgConfig.html#command:pkg_check_modules Ref: libgit2/objective-git#645
As per the docs[1], another set of variables will be provided by PKG_CHECK_MODULES when static linking (with the additional libraries required for static links). We need to pass those libraries to CHECK_LIBRARY_EXISTS so *its* link succeeds. 1 - https://cmake.org/cmake/help/v3.1/module/FindPkgConfig.html#command:pkg_check_modules Ref: libgit2/objective-git#645
As per the docs[1], another set of variables will be provided by PKG_CHECK_MODULES when static linking (with the additional libraries required for static links). We need to pass those libraries to CHECK_LIBRARY_EXISTS so *its* link succeeds. 1 - https://cmake.org/cmake/help/v3.1/module/FindPkgConfig.html#command:pkg_check_modules Ref: libgit2/objective-git#645
As per the docs[1], another set of variables will be provided by PKG_CHECK_MODULES when static linking (with the additional libraries required for static links). We need to pass those libraries to CHECK_LIBRARY_EXISTS so *its* link succeeds. 1 - https://cmake.org/cmake/help/v3.1/module/FindPkgConfig.html#command:pkg_check_modules Ref: libgit2/objective-git#645
67e8b75
to
96410f8
Compare
As per the docs[1], another set of variables will be provided by PKG_CHECK_MODULES when static linking (with the additional libraries required for static links). We need to pass those libraries to CHECK_LIBRARY_EXISTS so *its* link succeeds. 1 - https://cmake.org/cmake/help/v3.1/module/FindPkgConfig.html#command:pkg_check_modules Ref: libgit2/objective-git#645
As per the docs[1], another set of variables will be provided by PKG_CHECK_MODULES when static linking (with the additional libraries required for static links). We need to pass those libraries to CHECK_LIBRARY_EXISTS so *its* link succeeds. 1 - https://cmake.org/cmake/help/v3.1/module/FindPkgConfig.html#command:pkg_check_modules Ref: libgit2/objective-git#645
2097ba4
to
f518936
Compare
Hope you don't mind me pushing to your branch. This gets the build green by providing a pkg-config wrapper which simply forces the "--static" build. Supporting static libraries in CMake is a pain in the a**, which we'll have to tackle properly at some point in the hopefully not so far future in libgit2. |
This changes our various scripts to dump their build artifacts in External/build instead of various per-external directories (or /tmp for OpenSSL), which makes it easier to link them to each other when we finally get to building libgit2 on iOS.
e17c6e7
to
443abfe
Compare
Great many thanks for the fix @pks-t ! @pietbrauer I'm in a bit of a conundrum here : 0.14 has broken SSH support, but libgit2 v0.27.1 is a security release, so we can't wait too much… I'm tempted to temporarily switch our upstream to v0.27.1 + the static link fixes to my fork and release that as 0.14.1 (unless I can convince someone from libgit2 to bring in that branch in their repo). What do you think ? |
I think asking for a merge isn't something we should hesitate doing. If this is not an option for libgit2 we can discuss a different solution. But for now I would love to stick to the official source. |
Just to make sure: the pkg-config fixes on libgit2 master with regards to using absolute paths don't yet fix the issue you're seeing, do they? So you'd still need some additional patches landing in libgit2? Did you try the workaround with a pkg-config wrapper? As an alternative, I'd be open to backporting the security fix to v0.26 and doing another release there. I didn't discuss this with either @ethomson or @carlosmn, but I don't think they're going to argue against it as long as I'm the one who does it ;) |
I'm happy to do an 0.26 security release. |
Let me rephrase: I'd be happy if the libgit2 project did an 0.26 security release. I'm sorry that I don't have any time to help this week or next. |
@pks-t For context (as the "submodule update" commit message is wrong and I just fixup-ed everything before repushing to make sure everything is still green), this tests the Note that OCGit's current master uses libgit2-0.27-rc1, because then I got swamped with the SSH problems which we had no "integration" tests for. In all, I don't really have a need for a 0.26 update on libgit2, but a 0.27.\d with the absolute path changes would be 😍. Same here, work will be heavily impeding my ability to participate for the next couple weeks. |
Thanks for the clarification. The pkg-config fix is already backported to the will-be v0.27.2. I'm now just waiting for the duplicated submodules PR to be reviewed and merged to backport that one and then we're good to release v0.27.2 |
0.27.2 has been released. Might I ask for libgit2 to be updated to 0.27.2 and/or this PR to be merged? |
443abfe
to
6e54d41
Compare
✨ |
libgit2 0.27 was released earlier today, so let's update !! 🎉