Skip to content

Fix finding and linking libssh2 #6602

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix finding and linking libssh2 #6602

wants to merge 1 commit into from

Conversation

jeroen
Copy link
Contributor

@jeroen jeroen commented Jul 19, 2023 •
edited
Loading

This reverts the regression from #6586, and addresses the real issue.

On Windows, libssh2 depends on a few win32 system dlls such as ws2_32 that we need to pass to the linker:

# pkg-config --libs libssh2
-L/ucrt64/lib -lssh2 -lssl -lcrypto -lws2_32 -lgdi32 -lcrypt32

The linker understands this, but somehow the cmake check find_library does not find these windows dlls and fails:

-- Checking for module 'libssh2'
--   Found libssh2, version 1.11.0
CMake Error at cmake/FindPkgLibraries.cmake:17 (message):
  could not resolve ws2_32
Call Stack (most recent call first):
  cmake/SelectSSH.cmake:3 (find_pkglibraries)
  src/CMakeLists.txt:45 (include)

The change in #6586 masks the problem by not finding libssh2 at all anymore (by looking for the wrong lib name). This makes no sense, the .pc name is libssh2 and not ssh2, so we need to revert this.

This address the underlying problem, perhaps we can just warn instead of fail if cmake is unable to validate one of the libs.

Reverts #6586. Fixes #5981 for real.
@Faless

@jeroen jeroen mentioned this pull request Jul 19, 2023
Merged
@Faless
Copy link
Contributor

Faless commented Jul 19, 2023

Trying to use this PR, I'm getting:

2023-07-19T12:47:32.0057133Z -- Checking for module 'libssh2'
2023-07-19T12:47:32.8930031Z --   Found libssh2, version 1.8.2
2023-07-19T12:47:35.3480444Z CMake Warning at cmake/FindPkgLibraries.cmake:17 (message):
2023-07-19T12:47:35.3481324Z   could not resolve ssh2
2023-07-19T12:47:35.3481914Z Call Stack (most recent call first):
2023-07-19T12:47:35.3482440Z   cmake/SelectSSH.cmake:3 (find_pkglibraries)
2023-07-19T12:47:35.3482890Z   src/CMakeLists.txt:45 (include)
2023-07-19T12:47:35.3483199Z

and then the build fails with:

2023-07-19T12:48:57.9177831Z D:\a\godot-git-plugin\godot-git-plugin\thirdparty\git2\libgit2\src\libgit2\transports\ssh.c(11): fatal error C1083: Cannot open include file: 'libssh2.h': No such file or directory

While building with:

cmake -B bin\thirdparty\git2\windows\x86_64 -DCMAKE_C_COMPILER=cl -DCMAKE_SYSTEM_NAME=Windows -G "NMake Makefiles" -DCMAKE_BUILD_TYPE=Release -DOPENSSL_USE_STATIC_LIBS=1 -DOPENSSL_INCLUDE_DIR=D:\a\godot-git-plugin\godot-git-plugin\bin\thirdparty\openssl\windows\x86_64\dest\include -DOPENSSL_SSL_LIBRARY=bin\thirdparty\openssl\windows\x86_64\libssl.lib -DOPENSSL_CRYPTO_LIBRARY=bin\thirdparty\openssl\windows\x86_64\libcrypto.lib -DOPENSSL_ROOT_DIR=D:\a\godot-git-plugin\godot-git-plugin\bin\thirdparty\openssl\windows\x86_64 -DBUILD_TESTS=0 -DBUILD_CLI=0 -DBUILD_EXAMPLES=0 -DBUILD_FUZZERS=0 -DUSE_SSH=1 -DUSE_HTTPS=OpenSSL -DUSE_SHA1=OpenSSL -DUSE_BUNDLED_ZLIB=1 -DUSE_HTTP_PARSER=builtin -DREGEX_BACKEND=builtin -DBUILD_SHARED_LIBS=0 -DLIBSSH2_INCLUDE_DIR=D:\a\godot-git-plugin\godot-git-plugin\thirdparty\ssh2\libssh2\include -DLIBSSH2_LIBRARY=bin\thirdparty\ssh2\windows\x86_64\src\libssh2.lib -DSTATIC_CRT=0 thirdparty\git2

It seems to ignore LIBSSH2_LIBRARY and LIBSSH2_INCLUDE_DIR:

2023-07-19T12:47:37.4799970Z   Manually-specified variables were not used by the project:
2023-07-19T12:47:37.4800393Z 
2023-07-19T12:47:37.4801000Z     LIBSSH2_INCLUDE_DIR
2023-07-19T12:47:37.4801336Z     LIBSSH2_LIBRARY

ethomson
ethomson reviewed Jul 30, 2023
View reviewed changes
cmake/FindPkgLibraries.cmake
@@ -14,13 +14,13 @@ function(FIND_PKGLIBRARIES prefix package)
foreach(LIBRARY ${${prefix}_LIBRARIES})
find_library(${LIBRARY}_RESOLVED ${LIBRARY} PATHS ${${prefix}_LIBRARY_DIRS})
if(${${LIBRARY}_RESOLVED} STREQUAL "${LIBRARY}_RESOLVED-NOTFOUND")
message(FATAL_ERROR "could not resolve ${LIBRARY}")
message(WARNING "could not resolve ${LIBRARY}")
Copy link
Member

Choose a reason for hiding this comment

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

This is my only question - why are we changing this from a fatal to a warning? What's the operational change that this makes? If - for example - I asked for libssh2 on the command line and we couldn't find it, that should be a hard fail, shouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to explain this in the post above: on Windows we need to link some Windows SDK dlls such as ws2_32.lib. Cmake does not find these on the command line, so it fails, but the mingw-w64 linker can find them just fine.

See: https://cmake.org/pipermail/cmake/2010-July/037875.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps another solution would be to skip-list known windows sdk libraries.

@ethomson ethomson mentioned this pull request Aug 10, 2023
Merged
@jeroen jeroen closed this by deleting the head repository Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethomson ethomson ethomson left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Windows - CMake Error: could not resolve ssh2
3 participants
@jeroen @Faless @ethomson