libssh2 iOS link #4604
libssh2 iOS link #4604
Conversation
src/CMakeLists.txt
Outdated
| @@ -292,7 +292,18 @@ IF (LIBSSH2_FOUND) | |||
| LIST(APPEND LIBGIT2_PC_LIBS ${LIBSSH2_LDFLAGS}) | |||
| #SET(LIBGIT2_PC_LIBS "${LIBGIT2_PC_LIBS} ${LIBSSH2_LDFLAGS}") | |||
|
|
|||
| CHECK_LIBRARY_EXISTS("${LIBSSH2_LIBRARIES}" libssh2_userauth_publickey_frommemory "${LIBSSH2_LIBRARY_DIRS}" HAVE_LIBSSH2_MEMORY_CREDENTIALS) | |||
| # We might need to provide additional libraries so libssh2 links | |||
| SET(CMAKE_REQUIRED_LIBRARIES_OLD ${CMAKE_REQUIRED_LIBRARIES}) | |||
There was a problem hiding this comment.
CMAKE_REQUIRED_LIBRARIES is only used for CMake's internals such that it can successfully link test executables, right? If so, I'd expect this dance around CMAKE_REQUIRED_LIBRARIES_OLD to be useless. Just set CMAKE_REQUIRED_LIBRARIES and unset it afterwards again. Or does it have a default value?
There was a problem hiding this comment.
Looks like it is there for that express purpose, yes. I'll just unconditionally unset.
src/CMakeLists.txt
Outdated
| SET(CMAKE_REQUIRED_LIBRARIES_OLD ${CMAKE_REQUIRED_LIBRARIES}) | ||
| IF(BUILD_SHARED_LIBS) | ||
| SET(CMAKE_REQUIRED_LIBRARIES ${LIBSSH2_LIBRARIES}) | ||
| SET(LIBSSH2_TMP_LIBRARY_DIRS ${LIBSSH2_LIBRARY_DIRS}) |
There was a problem hiding this comment.
Do we need this in case of shared libraries? If not, we could probably make this much simpler:
IF(BUILD_SHARED_LIBS)
CHECK_LIBRARY_EXISTS(...) # The old code previous to this commit
ELSE()
SET(CMAKE_REQUIRED_LIBRARIES ${LIBSSH2_STATIC_LIBRARIES})
CHECK_LIBRARY_EXISTS(ssh2 libssh2_userauth_publickey_frommemory "${LIBSSH2_STATIC_LIBRARY_DIRS}" HAVE_LIBSSH2_MEMORY_CREDENTIALS)
UNSET(CMAKE_REQUIRED_LIBRARIES)
ENDIF()
There was a problem hiding this comment.
True, I was trying to keep a one-and-only-one CHECK_LIBRARY_EXISTS call.
Note that I feel like it means something is wrong w.r.t our pkg-config setup in that static-link case (which I'm ignoring because macOS doesn't actually depend on the built pc file). Also, licensing 😟.
There was a problem hiding this comment.
Yeah, I'm all for avoiding code duplication. But in this case, it's a lot easier to read with the duplicated code, so I'd say in this case duplication is preferable
"warning: values of type 'OSStatus' should not be used as format arguments; add an explicit cast to 'int' instead [-Wformat]"
6447e85 to
16a6e21
Compare
|
Rebased, should be 👍. Thanks for reining in my duplication frenzy @pks-t, as it sure looks nicer that way ! |
|
Small nit: would be nice to move the unset into the conditional block, as well. Looks good otherwise, thanks! |
16a6e21 to
d0ee33f
Compare
|
Unset moved ! |
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
d0ee33f to
860d31e
Compare
|
@pks-t It would seem I completely botched the change you had asked me, so now it's working again. I'll give a spin to the OCGit build as soon as I can get rid of the Swift issue I'm facing… |
|
This should hopefully be green everywhere (here and in OCGit). At least locally it's green 🤷♂️ . So, for a little bit of OCGit-related context about that last commit : the issue was that running Here are the respective link commands from CMake's log : The only difference is thus the On top of that, here are the variables set when pkg-finding static libssh2 : If you compare that to the above, you'll see CMake almost tries to use the first part in both cases (openssl), but the 2nd part is nowhere to be found. I heavily suspect this is a bug in CMake, not expecting a list or something, but passing Hopefully just above this line was the Shiny Sun Of Salvation, hence the fix : activate Maybe there's a better way 🙄… |
src/CMakeLists.txt
Outdated
| SET(CMAKE_EXE_LINKER_FLAGS ${EXE_LINKER_FLAGS}) | ||
| CHECK_LIBRARY_EXISTS(ssh2 libssh2_userauth_publickey_frommemory "${LIBSSH2_STATIC_LIBRARY_DIRS}" HAVE_LIBSSH2_MEMORY_CREDENTIALS) | ||
| UNSET(CMAKE_EXE_LINKER_FLAGS) | ||
| UNSET(CMAKE_REQUIRED_LIBRARIES) |
There was a problem hiding this comment.
Wow, that's a gross hack. If necessary I'll merge that, but I'm not yet convinced the problem isn't actually us.
Previously, we've been using CHECK_LIBRARY_EXISTS("${LIBSSH2_LIBRARIES}" ...), which works just right with dynamic libraries. Sure, we don't need to pass in dependencies of libssh2, as they're picked up by the linker anyway. That's why ${LIBSSH2_LIBRARIES} will only contain the path to the libssh2.so.
Now the case with static archives. We have to link not only against libssh2, but also explicitly against its dependencies. That's why ${LIBSSH2_LIBRARIES} will be something like "ssh2;ssl;crypto;z". Passing that one again to CHECK_LIBRARY_EXISTS is obviously not gonna work: the first parameter of that function shall be the library we're searching in, not a list of libraries. So what we'd have to do is to only pass ssh2 into CHECK_LIBRARY_EXISTS, but set CMAKE_REQUIRED_LIBRARIES up accordingly to contain the other libraries, as well.
The old commits of yours arent available anymore, so I can't check and don't remember if you did that. I know you used CMAKE_REQUIRED_LIBRARIES, but did you also modify CHECK_LIBRARY_EXISTS to only pass in "ssh2" as first argument?
There was a problem hiding this comment.
Yes, it's unsightly 😭. IIRC the old code did the same SET(CMAKE_REQUIRED_LIBRARIES…) as it is now (ie. direct passthrough of whatever PKG_CHECK_MODULE found for the static case), with only ssh2 as the first argument (lest CMake fails anyway, with Looking for libssh2_userauth_publickey_frommemory in ssh2;ssl;crypto;z - not found)
For reference, here's the implementation of CHECK_LIBRARY_EXISTS. As you can see, it will stringify whatever is passed as the LOCATION argument, which will output as a -L"${STATIC_LIBDIRS}", erroring because of file not found (which is one of the issues I encountered the 1st time).
- Passing
STATIC_LIBDIRSinCMAKE_REQUIRED_LIBRARIESwill result in :
-- Looking for libssh2_userauth_publickey_frommemory in ssh2
WARNING: Target "cmTC_8106a" requests linking to directory "/usr/local/Cellar/openssl/1.0.2o_1/lib". Targets may link only to libraries. CMake is dropping the item.
WAR
8000
NING: Target "cmTC_8106a" requests linking to directory "/usr/local/Cellar/libssh2/1.8.0/lib". Targets may link only to libraries. CMake is dropping the item.
WARNING: Target "cmTC_8106a" requests linking to directory "/usr/local/Cellar/openssl/1.0.2o_1/lib". Targets may link only to libraries. CMake is dropping the item.
WARNING: Target "cmTC_8106a" requests linking to directory "/usr/local/Cellar/libssh2/1.8.0/lib". Targets may link only to libraries. CMake is dropping the item.
-- Looking for libssh2_userauth_publickey_frommemory in ssh2 - not found
Everything results int the same underlying error (ld: library not found for -lssh2). What's maddening is that the last few arguments passed to ld are -L/usr/local/Cellar/openssl/1.0.2o_1/lib -Wl,-rpath,/usr/local/Cellar/openssl/1.0.2o_1/lib -lssh2 -lssh2 -lssl -lcrypto -lz so it sees the openssl libdir coming from LOCATION, but it only processes the first item (the CMake "bug").
There are a bunch of alternatives :
- (OCGit) Make
cmakeforget it's running in an Xcode build environment (that-isysrootis IMHO the curx of the issue). That would be the cleanest IMHO, but I haven't been able to find what causes it to grab this environment. - (OCGit) Build a Mac version of openssl/libssh2 and link against that instead of depending on Homebrew (which is a bad idea anyway because it only builds for $LATEST_OSX and the linker warns that 1) it's bad 2) it will become an error Soon™). Cons are Travis build times.
- (libgit2) Paper over the issue here.
|
Took another stab at it. I got rid of the argument building with some stringification. I've also moved the policy check to where it's relevant (CMP0056 is CMake 3.2+), so I can use it if available, and revert to the "next best thing" if not. I don't think it can be fixed without CMP0056 : a static link of libssh2 using OpenSSL as its backend will always fail, because of the truncated LIBDIRS (thus this should impact static on Linux and friends). |
|
I just tried to reproduce the issue on Linux, but I'm unable to do so. I've changed all the So it does even work when passing in a list of static libraries, which is unexpected. And I didn't have to set/unset So they're getting concatenated in the CheckLibraryExists module anyway. So let's just scrap that and pass in So why doesn't this still work... your workaround will pass the linker flags directly to the compiler. So the reason has to be that the compiler is still unable to correctly locate the library. We're already passing in ${LIBSSH2_LIBRARIES} in a correct way, so I doubt that's the issue. As I'm not seeing the issue on Linux, where the library is in one of the standard link directories, but you're seeing it with Homebrew where it's not, I strongly suspect that we're mishandling the link directory. So we're passing in the location of the library via This should be rather easy to fix, I hope: This is all an (educated) shot in the dark. Xcode is installing in the background in case this does not solve your issue. |
|
The difference is caused by the apparition of -isysroot in the compiler/linker arguments, pointing to the local SDK. I doubt -isysroot is useful enough on Linux that you would be able to reproduce its "environment", but the crux of the issue is that we're not re-adding / afterward… Your reasoning about what is happening try_compile-wise is the same as mine. CMakeError.log of your shot in the dark : |
|
Okay, so my analysis (ignoring -isysroot) is in fact correct. So one of actual issues is that CMake's try_compile cannot handle multiple link directories |
|
Yes, at least using the obvious ways. As for your analysis, it might be possible to cause Linux to fail by doing source-installs of dependencies in non-standard, separate locations (like the /usr/opt/local/$dep brew does). |
|
Could you please give my branch at https://github.com/pks-t/libgit2/tree/pks/cmake-resolve-pkgconfig a go? The fix in there needs to be merged anyway, disregarding if it fixes your issue or not. We are currently plain mishandling libraries found by pkg-config -- we have to resolve them against the found library dirs instead of only resolving them at the end, when linking libgit2. |
|
Running that branch against my OCGit buildsys change gets me an SSH-enabled macOS framework, but a SSH-missing iOS one (here's the CMakeError.log relevant excerpt for one of the archs). |
|
And doing the |
|
Okay, that's good news, thanks for testing. I'll open a PR for that. I wonder about that static-dance, though. I seem to recall that @carlosmn told me once that |
|
As per FindPkgConfig, it unconditionally does both checks. So maybe the fix is to try to use the "dynamic" variables first, and fallback to If that looks correct, I say the best way would be to DRY all those LIST calls via a |
As a library, we don't know what's supposed to happen around us. It's the final binary which defines what should get linked together. There might be other dependencies that want to use libssh2 or whatever else, and we don't want to conflict. The application might also only want to link libgit2 statically but link to everything else dynamically, so they can ship their build (or whatever) artifact around but still make use of their system's update mechanisms for more general or stable dependencies. |
|
But we'd have to provide some way to the user of libgit2 to have libgit2 use either static or dynamic dependencies. Otherwise, we'll just always link libgit2.so against the found set of shared libraries. Or is there a mechanism here that I am not thinking of? |
|
Hm... so we're trying to build libgit2 dynamically so the app can link dynamically against libgit2, but then have our dependencies be static? I don't think I know of a pattern for doing that but it ends up having the same issues as if we were static. The rest of the code could be trying to link against the same libraries again leading again to symbol conflicts. Admittedly we kinda do that with our vendored libraries but we only use them (by default) if we don't find them on the system. This seems to be pretty specific for 🍎 ☎️ so I think overall it might be more elegant for the project dealing with that to create an intermediate |
|
I'm more interested in the other case. Let's say we are building libgit2 as static only, and the system has both dynamic and static libs for all dependencies. Our build system will only ever pick up the dynamic dependencies in this case. As we're only generating a static archive, we won't link those dynamic dependencies in, but only write them into the libgit2.pc file. This is the problem, though: the user of libgit2 is now unable to easily do a completely static build. When using dynamic deps, we only put in the immediate library dependencies into the pkgconfig. If the users wants to do a static link, he'd also need to link against transitive dependencies. Okay, that now just made clear to me that the real culprit is us not setting up the pkg-config file correctly. We'd also have to add a Requires.private variable that lists tranisitve dependencies. So the other problem that is kind of unrelated is how we determine symbol availability in case there are no dynamic archives. This seems to be the case for iOS builds of Objective-Git, where we need to link against a specific libssh2. Right now, we just use LIBSSH2_LIBRARIES, ignoring LIBSSH2_STATIC_LIBRARIES, and thus fail to see that libssh2_userauth_publickey_frommemory would in fact be available if we used that static lib. So should we just fall back to the static one in case no dynamic deps exist? |
|
The funny thing is here that we cannot even distinguish between transitive and non-transitive dependencies via CMake. It will only set a |
|
I amended my PR to do what @tiennou proposed: by default, we're trying to use |
|
I gave a spin to the changes. The macOS build is ok, but the iOS builds failed : CMake output : CMakeError.log : This is somewhat like the state before I added the CMAKE_REQUIRED_LIBRARIES change, apart from the missing |
|
Thanks for your feedback. I've now finally had the ability to test on macOS. The issue here is simply that There's two ways to go forward here: The first way is to tell the user (objective-git) to use a workaround: simply set PKG_CONFIG=/path/to/wrapper.sh, where wrapper.sh contains: So we simply force pkg-config to always return static libraries. This should just work, but is simply working around an issue in our own build scripts. So this does convince me that there should be a second way, which is a new switch called e.g. -DUSE_STATIC_DEPS, that allows the user to have us use static dependencies only. This would get passed on to |
|
Okay, so whenever I start tackling the issue of linking against static libraries in our build instructions I'm getting aware of the issues it brings along:
In short, we just aren't ready right now to introduce static library support, it'd need some more time. I'm not opposed, but I don't want to have a half-hearted solution here, and doing it right takes time I don't have right now. That's why I went forward and pushed the previously mentioned hack of using a pkg-config wrapper to your OCGit pull request, which causes the CI to pass. I hope we can address this shortcoming soon, though. Several people have been asking for this over the years, and we never had a real solution to this situation. |
|
I'm closing this, as the underlying iOS-specific issue has been fixed (though I haven't been able to find a "static build" issue I could #ref it from, because there's helpful context in here). |
This is the changes needed on libgit2's side for libgit2/objective-git#645.
Note that I'm basing this on master so it's mergeable, but I would really appreciate if someone with push access could bring
fix/libssh2-ios-link-v0.27in the main repo (which is those 2 commits, on top ofmaint/v0.27), just so I could update OCGit's submodule without switching to my fork (which would be unfortunate and "weird").