libgit2: Add support for OpenSSH instead of libssh2 #1031
Conversation
|
Hmm, let me try to see if I can figure out why the tests are failing... |
bnjmnt4n
force-pushed
the
openssh
branch
2 times, most recently
from
43cad8d
to
e38111c
Compare
bnjmnt4n
force-pushed
the
openssh
branch
2 times, most recently
from
b8411c4
to
a0235c5
Compare
|
One issue I've noticed with this is when linking against a system library it doesn't detect whether it was compiled with openssh enabled, so the feature only really works when forcing the vendored library. (EDIT: Well, also it shouldn't be allowing the system library since it's only 1.7.2, the 1.8.0 bump needs to update the |
|
I can update the version range detected, but I'm not too sure how to proceed with detecting which version of the SSH backend libgit2 was compiled with, would appreciate some guidance on that. |
bnjmnt4n
force-pushed
the
openssh
branch
2 times, most recently
from
6ad326a
to
813cb98
Compare
bnjmnt4n
force-pushed
the
openssh
branch
3 times, most recently
from
8fd23a4
to
3ab6e09
Compare
|
Let me try to summarize the possible problems with the issues of the system libgit2 being out of sync with the features here:
Would it be possible to dig into those two cases where the system has libssh2, but git2-rs thinks it is something else (exec or not enabled)? What is the consequence of libssh2 initializing OpenSSL a second time? Does that break assumptions in the rust-openssl crate's own initialization? I assume another issue is just the user confusion. They may have the Are these cargo features only valid when using the vendored copy? In other words, they don't really have a meaningful behavior with a system libgit2? Would it be helpful at all if libgit2 provided a function to indicate which ssh backend it was compiled with at runtime? |
| ssh = ["libgit2-sys/ssh"] | ||
| ssh = ["ssh-libssh2"] | ||
| ssh-libssh2 = ["libgit2-sys/ssh-libssh2"] | ||
| ssh-openssh = ["libgit2-sys/ssh-openssh"] |
There was a problem hiding this comment.
Can this (and libgit2-sys/Cargo.toml) comment that the openssh support is experimental with a comment here? Perhaps something like:
| ssh-openssh = ["libgit2-sys/ssh-openssh"] | |
| # Support for ssh-openssh is experimental. | |
| ssh-openssh = ["libgit2-sys/ssh-openssh"] |
Also, I'm a little uncertain about the interaction if both ssh-libssh2 and ssh-openssh is set. From what I can gather, ssh-libssh2 takes precedence, is that correct? That seems like it could potentially be awkward or confusing or difficult to get it configured properly, particularly with ssh-libssh2 in the default set, and with cargo's feature unification. I suspect it won't be obvious to most people who just see this list of features in Cargo.toml.
Some options for how this could be handled:
- Document the interaction.
- Remove
sshfrom the default set, and make it more explicit. - Detect if both are set and raise a compile-time error.
Unfortunately none of these seem like great options.
There was a problem hiding this comment.
If both features are set, I think libssh2 will take precedence, because we are just adding #define instructions, and the #ifdef in the libgit2 codebase appears to always check for libssh2 first. (example).
If we do (3), my understanding is that we would need to add more environment variables similar to LIBGIT2_NO_VENDOR to allow explicit disabling of one of the features? (Correct me if I'm wrong.) This makes it seems like doing (1) + (2) would be better.
It seems so to me. There isn't any existing build-time detection of whether an enabled Cargo feature (HTTPS or SSH) matches the system libgit2's compiled features. So this is already an existing problem, although I suspect mitigated by the fact that most system libgit2s would be compiled with HTTPS + SSH. I guess the simplest way would just be to detect during build-time if the SSH feature enabled by the user matches the SSH backend on system libgit2, and compile and use the vendored libgit2 if they don't match. libgit2 can probably be modified to add the SSH backend type to https://github.com/libgit2/libgit2/blob/6c5520f334e5652d5f0476c00a3188d1d97754e7/src/libgit2/libgit2.c#L81-L97, but I'm not too sure how to compile the C code at build time to do this detection... |
I can help here, can you open an issue in https://github.com/libgit2/libgit2 and describe the outcome that you want? In other words, do you want to know if some HTTPS is enabled (IOW, keep |
|
@bnjmnt4n can you link this issue from when you opened one on the libgit2 repo, so we can track progress on this? |
|
I haven't created an issue, because as mentioned I'm not very familiar with the Rust build process and what would be the best way to detect if OpenSSH/libssh2 is enabled at build-time. I'm guessing that what @ethomson suggested (keep existing |
This commit changes the original `ssh` feature into two new ones:
`ssh-libssh2` and `ssh-openssh`. By default, the `ssh-libssh2` feature
is enabled for backwards compatibility.
To use OpenSSH instead, the following listing in `Cargo.toml` can be
used:
git2-rs = { version = "...", default-features = false, features = ["https", "ssh-openssh"] }
|
As far as I can see, adding a flag to libgit2's feature function will require executing code to check the flag. This might be a problem when cross-compiling because libgit2, and git2-rs, might not be targeting the build platform. Maybe we can add external symbols to libgit2 to indicate what features have been linked in, and import the required symbols in git2-rs. This way, we could not link against a libgit2 configured without the required options. Another option would be to add some defines to |
This PR changes the original
sshfeature into two new ones:ssh-libssh2andssh-openssh. By default, thessh-libssh2feature is enabled for backwards compatibility.To use OpenSSH instead, the following listing in
Cargo.tomlcan be used:This PR is stacked on top of #1032.
Closes #1028.