Description
I have alternatives for fixes but need advice before making a pull request.
Reproduction steps
VC++
cmake .. -A x64 -DBUILD_EXAMPLES=ON -DEMBED_SSH_PATH="D:/src/AAThirdparty/C,C++/libssh2-1.10.0" -DCMAKE_INSTALL_PREFIX="D:/temp/lg2" -DDEPRECATE_HARD=ON -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON
cmake --build .
MingW
cmake .. -G "MinGW Makefiles" -DBUILD_EXAMPLES=ON -DCMAKE_BUILD_TYPE=Release -DUSE_BUNDLED_ZLIB=ON -DEMBED_SSH_PATH=/d/src/AAThirdparty/C,C++/libssh2-1.10.0 -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON -DDEPRECATE_HARD=ON
cmake --build .
Expected behavior
Build completes successfully as it did for 1.3.0.
Actual behavior
Build fails. See below for diagnosis and details.
Version of libgit2 (release number or SHA1)
1.4.x, main (1.5.x)
Operating system(s) tested
Windows, MingW and VC++
Details
There are multiple issues with the EMBED_SSH_PATH option. The first is that the libssh is built twice - once directly via src/CMakeLists.txt and then again via the include of SelectSSH.
Clearly one of these has to be removed. Both alternatives lead to further issues as below.
Alternative 1 - keep the include SelectSSH
The cleaner alternative would seem to be to keep the include SelectSSH and remove fragment at src/CMakeLists.txt. This requires some additional fixes to SelectSSH.cmake as otherwise it picks up libssh headers from the MingW system directory instead of from the
EMBED_SSH_PATH directory. This breaks the build with undefines symbols if (as in my case) the latter is a newer version than the one with MingW. Also, need the bcrypt library to be added.
@@ -34,8 +34,12 @@ if(WIN32 AND EMBED_SSH_PATH)
list(APPEND LIBGIT2_DEPENDENCY_OBJECTS ${SSH_SRC})
list(APPEND LIBGIT2_DEPENDENCY_INCLUDES "${EMBED_SSH_PATH}/include")
+ include_directories("${EMBED_SSH_PATH}/include")
+ list(APPEND LIBGIT2_SYSTEM_LIBS "crypt32" "bcrypt")
+ list(APPEND LIBGIT2_PC_LIBS "-lcrypt32" "-lbcrypt")
file(WRITE "${EMBED_SSH_PATH}/src/libssh2_config.h" "#define HAVE_WINCNG\n#define LIBSSH2_WINCNG\n#include \"../win32/libssh2_config.h\"")
set(GIT_SSH 1)
endif()
With this change and deletion of the src/CMakeLists.txt fragment building libssh, mingw builds work correctly and all tests pass.
However, Visual Studio builds fail with errors I cannot decipher :-( due to being lost in cmake and visual studio build environments. It's quite possibly something simple in configuration but having spent most of the day, I can't spend more time on it.
...just a sampling of many similar errors...
D:\src\AAThirdparty\C,C++\libssh2-1.10.0\src\scp.c(742): error C2039: 'st_mtim': is not a member of '_stat64' [D:\src\libgit2\vc64\tests\libgit2_tests.vcxproj
"D:\src\libgit2\vc64\src\git2.vcxproj" (default target) (7) ->
(ClCompile target) ->
D:\src\AAThirdparty\C,C++\libssh2-1.10.0\src\agent.c : fatal error C1083: Cannot open precompiled header file: 'git2.dir\Debug\git2.pch': No such file or directory [D:\src\libgit2\vc64\src\git2.vcxproj]
The only place I see st_mtim is in win32_compat.h but cannot understand how that impacts libssh. The error with git2.pch is because that file is not being generated (or should it really be looking for git2internal.h?)
Alternative 2 - use the fragment in Cmakelists.txt, and eliminate SelectSSH
(This would be only if EMBED_SSH_PATH was defined on WIN32). This needs the following changes:
The line set_target_properties(git2internal PROPERTIES C_EXTENSIONS OFF) has to be removed (again under a EMBED_SSH_PATH define). Turning extensions off causes the compiler --std=gnu90 to --std=c90 which causes libssh build to fail because it uses the inline keyword which is not part of the C90 standard.
As above the bcrypt library also has to be added.
With these changes both mingw and VC++ builds are successful. The former has no test errors. The latter has 3 test errors, all of which are because it expects current drive to be C: and I'm running on D:.
I need further guidance as to which of the above approaches (or some other) is a better fix and be a suitable candidate for a pull request. Alternative 1 seems cleaner but I would need some help with diagnosing further as I'm a little lost in how that st_atim #define is getting picked up. For alternative 2, how does one limit the -std compiler setting to only impact libssh? Or is it ok to switch back to gnu90 mode?