Build failures with recent embedded libssh2 #4303
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pks-t
force-pushed
the
pks/cmake-embedded-libssh2
branch
from
0c271bb to
0f3969d
Compare
Member
Author
|
Rebased due to #4282. I'm just sitting at the train station and got no Windows VM ready, so I cannot test this right now. So please only merge if you've confirmed this works on Windows. Otherwise I'll try to test this in a few days, hopefully. |
Member
|
In this branch, specifying |
pks-t
force-pushed
the
pks/cmake-embedded-libssh2
branch
from
0f3969d to
fc2ccf6
Compare
Member
Author
|
That is... strange. At least that's why I'm adding the bindir parameter to |
Member
Author
|
@carlosmn: have you actually specified |
pks-t
force-pushed
the
pks/cmake-embedded-libssh2
branch
from
fc2ccf6 to
dd81940
Compare
Member
Author
|
Refactored this PR to not introduce a new variable "USE_EMBEDDED_LIBSSH2". I've also added a new change to allow using an embedded libssh2 not only on Windows systems, but also on all other systems. @tiennou, this might be of interest to you for iOS. |
pks-t
force-pushed
the
pks/cmake-embedded-libssh2
branch
from
dd81940 to
77d3953
Compare
Member
Author
|
I've rebased this PR on top of latest master to fix conflicts. I've also converted the USE_SSH option to work exactly like USE_HTTPS works, so that one can explicitly choose backends. |
pks-t
force-pushed
the
pks/cmake-embedded-libssh2
branch
2 times, most recently
from
c71f845 to
8aba1a2
Compare
pks-t
force-pushed
the
pks/cmake-embedded-libssh2
branch
from
8aba1a2 to
ef4a1ce
Compare
Member
Author
|
Anybody cares enough to review? I'd really like to get this landed :) |
Contributor
|
A Ninja build reported the following (note the detection of v1.8.2 in
|
Member
Author
|
Thanks for testing, @tiennou.
The detection is perfectly normal and is part of the auto-detection similar to how the HTTPS selection works. As you've set USE_SSH=libssh2_embedded it's not used in the end.
Right, I see that, too. Didn't notice earlier because I also had system libssh2 available and thus it still found the correct header. Will investigate. |
Due to recent changes to some required defines in libssh2, we now fail to build the library correctly. This results from the fact that we're manually including libssh2 sources and its include directory into our own build infrastructure, which may not be configured as required by libssh2. As libssh2 is driven by CMake as well, we can simply include it as a subdirectory and stop caring for its configuration. Instead, we will now just link against the generated library and be done with it, without caring about the library internals.
Following the HTTPS backend selection, let's make the SSH backend selection more generic. Instead of choosing "-DUSE_SSH=(ON|OFF)", one can now also set it to an explicit backend. Currently, only two backends "libssh2" and "libssh2_embedded" are supported.
When setting the STRINGS property for cache variables, CMake GUI will present a dropdown box so that the developer may choose one of supplied values. Use this feature for HTTPS, SHA1 and SSH backends, which all support a fixed set of values, only.
Member
Author
|
Fixed the issue @tiennou noticed. |
pks-t
force-pushed
the
pks/cmake-embedded-libssh2
branch
from
ef4a1ce to
da35922
Compare
Member
Author
Contributor
|
Sorry, forgot to say that I don't mind, and both have been dwarfed by your #5284 since 😜. I usually rebase on top of whatever master is reflexively, only to stop if there's any conflicts… If this one goes it quickly, I'll have to adapt the other on top anyways. |
Member
Author
Indeed. I guess it's going to take some time until that one lands, though, as I still need to see how to make it backwards compatible with older versions of CMake. |
Member
|
I don't want to embed libssh2. Going to close this; let's stop embedding that library entirely. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Another day, another build infrastructure tweak. So yet again, please do not merge until #4282 is merged.
This PR improves how we build libssh2. In recent commits, they have changed some things regarding their build infrastructure, which would require us to add additional hacks to compile libssh2 directly as part of our own sources. As libssh2 uses CMake, though, we should simply be including it as a subdirectory and be done about it.
Note that this means that there is an additional dynamic library that has to be distributed. I don't know yet whether it is possible to statically link it instead of generating a dynamic one to avoid this, but I'll take another stab at this later. Regardless of this it is definitly the right thing to do, as we do not want to fiddle with their build instructions. Doing so may lead to subtle breakages due to the build environment not matching what the libssh2 project expects, which is critical to avoid in a library directly related to the user's security.
This fixes #4302.