8000 Allow combining GIT_SSH_EXEC and GIT_SSH_LIBSSH2 in a single build by jorio · Pull Request #6993 · libgit2/libgit2 · GitHub
[go: up one dir, main page]

Skip to content

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jorio
Copy link
Contributor
@jorio jorio commented Dec 30, 2024

(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).

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.
@ethomson
Copy link
Member

😢 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 -DUSE_FEATURE=provider at this moment when I saw the notification for this PR).

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.)

@ethomson
Copy link
Member

Okay, after sleeping on this, the concept makes a bit more sense to me. A couple of notes:

Instead of hardcoding exec+libssh2, I think that we should take a list of options at compile-time, and in fact, use that as the preference for implementation. One can imagine that you would want libssh2,exec or exec,libssh2 depending on circumstances. This would require a bit more cmake trickery, and it's sort of ideating a feature that I don't know that we actually have a need for. So I don't think that we need to make those changes in this PR. But let's imagine that we want to do that in the future, and make this configuration option libssh2,exec so that we're a bit future-proofed.

I had considered adding git_ssh_backend_t in #6971 but went with strings instead, as I thought that would allow for a bit more growth, especially since it covered all the backends, not just ssh. So I think that matching what we get out of that API (strings) with what we put in to selection would be useful.

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:

git_libgit2_opts(GIT_OPT_SET_BACKEND, GIT_FEATURE_SSH, "exec");

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. 🤔

@ethomson
Copy link
Member

(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.)

@jorio
Copy link
Contributor Author
jorio commented Jan 7, 2025

Hi @ethomson and thank you for the review! The last 3 commits address the points that you raised:

  • let's imagine that we want to do that in the future, and make this configuration option libssh2,exec
  • matching what we get out of that API (strings) with what we put in to selection would be useful.
  • we should have a selector for any backend, not just SSH backends.

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):

git_backend__register(GIT_FEATURE_SOMETHING, "backendname", install_cb, uninstall_cb, &user_payload);

You can then change backends with:

git_libgit2_opts(GIT_OPT_SET_BACKEND, GIT_FEATURE_SSH, "exec");

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 git_libgit2_feature_backend() (except it'll return an empty string instead of NULL):

git_libgit2_opts(GIT_OPT_GET_BACKEND, GIT_FEATURE_SSH, &git_buf);

Let me know if you'd like me to rework anything!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0