libgit2 / objective-git

Objective-C bindings to libgit2
MIT License
1.16k stars 280 forks source link

Update libgit2 to 0.27 #645

Closed tiennou closed 6 years ago

tiennou commented 6 years ago

libgit2 0.27 was released earlier today, so let's update !! πŸŽ‰

tiennou commented 6 years ago

For the record, it seems CMake is borking the SSH tests because it doesn't bring OpenSSL into the linking mix. Or maybe the blame lies within libgit2's CMakeLists ?

@pks-t Maybe you've got an opinion/advice about how to fix it ?

pks-t commented 6 years ago

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.

pks-t commented 6 years ago

Also, the bundled version of openssl is heavily out of date and susceptible to various CVEs. Is that thing even used?

tiennou commented 6 years ago

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 libssh2_userauth_publickey_frommemory function, but since the libgit2 CMakeFile is unaware that we're static-linking against OpenSSL it always fails. Hence the question I had : is this a limitation of how we're embedding libssh2 from libgit2 ? To be frank, I'm still trying to wrap my head around it, but maybe we're also not specifying libssh2 CRYPTO_BACKEND correctly, maybe ? And maybe also we're not finding the correct OpenSSL .pc file, which IIUC should have the required link lines ? I'm also confused as to what the build prefix is at each step (it's different for each lipo build and each dependency, which might not help finding things).

The included OpenSSL is there for the SSH support #648, so "oh-my-god", "yes", are the replies 😒.

pks-t commented 6 years ago

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 PKG_CHECK_MODULES(LIBSSH2 libssh2), all linker flags are retrieved from the libssh2.pc file. So you should probably adjust the PKG_CONFIG_PATH environment variable such that the correct libssh2.pc file is found.

pks-t commented 6 years ago

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.

tiennou commented 6 years ago

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 ?

pietbrauer commented 6 years ago

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.

pks-t commented 6 years ago

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 ;)

ethomson commented 6 years ago

I'm happy to do an 0.26 security release.

ethomson commented 6 years ago

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.

tiennou commented 6 years ago

@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 pkg-config --static wrapper + your absolute path fix branch on libgit2's side (IIRC pks/cmake-resolve-pkgconfig). AFAICT this is the only fix needed on libgit2's side so we can workaround the build issue here.

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.

pks-t commented 6 years ago

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

savyajha commented 6 years ago

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?

tiennou commented 6 years ago

✨