-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Allow combining GIT_SSH_EXEC and GIT_SSH_LIBSSH2 in a single build #6993
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.
By clicking “Sign up for GitHub”, you agree to our terms of service and pri 8000 vacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This introduces CMake flag `-DUSE_SSH=exec+libssh2` which lets you compile libgit2 with support for both exec and libssh2. When both backends are compiled in, libssh2 is the default, and you can switch to exec with `git_libgit2_opts(GIT_OPT_SET_SSH_BACKEND, GIT_SSH_BACKEND_EXEC)`. When running test.sh, you can specify multiple backends to test like this: `SSH_BACKENDS="libssh2 exec" ./ci/test.sh`. To preserve compatibility with existing CI jobs, SSH_BACKENDS is "default" when omitted.
😢 I understand your motivation for this, but... it's not exactly the way the library thinks about feature implementation (there's a single backend that implements a feature) and we've been moving even more in that direction (literally, I'm changing the cmake options to be more skewed into I'm not saying no to this, not at all; like I said, I understand your motivation. But I do need to give some thought to how it fits in with the mental model of feature -> provider mapping that I've been working on. (Are there other features that could have multiple providers that are changed by an option? If not, why not? etc.) |
Okay, after sleeping on this, the concept makes a bit more sense to me. A couple of notes: Instead of hardcoding I had considered adding Finally, I think that we should have a selector for any backend, not just SSH backends. In other words, I would want to be able to do something like:
I can imagine a world where we'd want to compile in both schannel and WinHTTP support on Windows and let people decide which they want to use dynamically. Or compile against multiple zlib implementations to do benchmarking very easily. 🤔 |
(To be clear, I'm not suggesting that we blow up the scope here and add changeable backends for all the things, but I think that we should shape the API as if we would, in the future.) |
Hi @ethomson and thank you for the review! The last 3 commits address the points that you raised:
The last commit in the PR might be a bit overkill just for SSH backends, but it sets up a framework for changeable backends for any feature. Features that support changeable backends can register their backends like so (the first backend registered for a feature becomes the default for that feature):
You can then change backends with:
You can query the active backend with GIT_OPT_GET_BACKEND. It'll also work with features that don't support changeable backends; in this case it'll return
Let me know if you'd like me to rework anything! |
(Rationale for this PR: I'd like to add OpenSSH support to pygit2, which currently configures libgit2 to use libssh2, without having to distribute custom builds.)
This PR introduces CMake flag
-DUSE_SSH=exec+libssh2
which lets you compile libgit2 with support for both exec and libssh2. This way, bindings like pygit2 can offer both options without requiring separate builds.When both backends are compiled in, libssh2 is the default, and you can switch to exec with
git_libgit2_opts(GIT_OPT_SET_SSH_BACKEND, GIT_SSH_BACKEND_EXEC)
.When running test.sh, you can specify multiple backends to test like this:
SSH_BACKENDS="libssh2 exec" ./ci/test.sh
. To preserve compatibility with existing CI jobs, SSH_BACKENDS is "default" when omitted (which will test just the "default" backend).