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

Update libgit2 to 0.27 #645

Merged
merged 10 commits into from
Jun 15, 2018
Merged

Update libgit2 to 0.27 #645

merged 10 commits into from
Jun 15, 2018

Conversation

tiennou
Copy link
Contributor

@tiennou tiennou commented Mar 1, 2018

libgit2 0.27 was released earlier today, so let's update !! 🎉

@tiennou
Copy link
Contributor Author

tiennou commented Mar 26, 2018

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
Copy link
Member

pks-t commented Mar 27, 2018

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
Copy link
Member

pks-t commented Mar 27, 2018

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

@tiennou
Copy link
Contributor Author

tiennou commented Mar 27, 2018

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
Copy link
Member

pks-t commented Mar 27, 2018

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.

tiennou added a commit to tiennou/libgit2 that referenced this pull request Mar 29, 2018
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
tiennou added a commit to tiennou/libgit2 that referenced this pull request Mar 29, 2018
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
tiennou added a commit to tiennou/libgit2 that referenced this pull request Apr 9, 2018
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
tiennou added a commit to tiennou/libgit2 that referenced this pull request Apr 9, 2018
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
@tiennou tiennou mentioned this pull request Apr 9, 2018
tiennou added a commit to tiennou/libgit2 that referenced this pull request Apr 19, 2018
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
tiennou added a commit to tiennou/libgit2 that referenced this pull request Apr 19, 2018
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
@tiennou tiennou force-pushed the update/libgit2-0.27 branch 2 times, most recently from 67e8b75 to 96410f8 Compare April 23, 2018 09:12
tiennou added a commit to tiennou/libgit2 that referenced this pull request Apr 23, 2018
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
tiennou added a commit to tiennou/libgit2 that referenced this pull request Apr 23, 2018
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
@tiennou tiennou force-pushed the update/libgit2-0.27 branch 4 times, most recently from 2097ba4 to f518936 Compare April 23, 2018 23:19
@pks-t
Copy link
Member

pks-t commented May 4, 2018

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.
@tiennou
Copy link
Contributor Author

tiennou commented May 29, 2018

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
Copy link
Member

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
Copy link
Member

pks-t commented Jun 1, 2018

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
Copy link
Member

ethomson commented Jun 1, 2018

I'm happy to do an 0.26 security release.

@ethomson
Copy link
Member

ethomson commented Jun 1, 2018

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
Copy link
Contributor Author

tiennou commented Jun 1, 2018

@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
Copy link
Member

pks-t commented Jun 1, 2018

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
Copy link

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
Copy link
Contributor Author

tiennou commented Jun 15, 2018

@tiennou tiennou merged commit 13c3213 into libgit2:master Jun 15, 2018
@tiennou tiennou deleted the update/libgit2-0.27 branch June 15, 2018 17:42
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