8000 libssh2 iOS link by tiennou · Pull Request #4604 · libgit2/libgit2 · GitHub
[go: up one dir, main page]
Skip to content

libssh2 iOS link #4604

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

libssh2 iOS link #4604

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dd09dd3
stransport: fix a warning on iOS
tiennou Apr 9, 2018
860d31e
cmake: setup required libraries before checking function
tiennou Apr 19, 2018
6d39c81
cmake: manually pass link flags when checking libssh2 function
tiennou Apr 23, 2018
d647a07
fixup! use the ldflags instead of building arguments manually
tiennou Apr 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ ENDIF()
IF (POLICY CMP0042)
CMAKE_POLICY(SET CMP0042 NEW)
ENDIF()
IF(POLICY CMP0056)
CMAKE_POLICY(SET CMP0056 NEW)
ENDIF()

# Add find modules to the path
SET(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${libgit2_SOURCE_DIR}/cmake/Modules/")
Expand Down
17 changes: 16 additions & 1 deletion src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,22 @@ 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
IF(BUILD_SHARED_LIBS)
CHECK_LIBRARY_EXISTS(ssh2 libssh2_userauth_publickey_frommemory "${LIBSSH2_LIBRARY_DIRS}" HAVE_LIBSSH2_MEMORY_CREDENTIALS)
ELSE()
SET(CMAKE_REQUIRED_LIBRARIES ${LIBSSH2_STATIC_LIBRARIES})
# CMake will fail to recognize more than one item in the LOCATION
# parameter of CHECK_LIBRARY_FLAGS. We thus manually build the correct link
# flags and pass them through CMAKE_EXE_LINKER_FLAGS.
FOREACH(lib ${LIBSSH2_STATIC_LIBRARY_DIRS})
SET(EXE_LINKER_FLAGS "-L${lib} ${EXE_LINKER_FLAGS}")
ENDFOREACH()
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)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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_LIBDIRS in CMAKE_REQUIRED_LIBRARIES will 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.
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.
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 cmake forget it's running in an Xcode build environment (that -isysroot is 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.

ENDIF()
IF (HAVE_LIBSSH2_MEMORY_CREDENTIALS)
SET(GIT_SSH_MEMORY_CREDENTIALS 1)
ENDIF()
Expand Down
2 changes: 1 addition & 1 deletion src/streams/stransport.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ static int stransport_connect(git_stream *stream)

ret = SSLHandshake(st->ctx);
if (ret != errSSLServerAuthCompleted) {
giterr_set(GITERR_SSL, "unexpected return value from ssl handshake %d", ret);
giterr_set(GITERR_SSL, "unexpected return value from ssl handshake %d", (int)ret);
return -1;
}

Expand Down
0