libgit2: Add support for OpenSSH instead of libssh2

[bnjmnt4n]

This PR 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"] }

This PR is stacked on top of #1032.

Closes #1028.

[bnjmnt4n]

Hmm, let me try to see if I can figure out why the tests are failing...

[Nemo157]

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 pkg-config version range too).

[bnjmnt4n]

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.

[ehuss]

Let me try to summarize the possible problems with the issues of the system libgit2 being out of sync with the features here:

System	git2-rs	Result
neither	neither	attempts to use ssh is runtime error
neither	libssh2	attempts to use ssh is runtime error, libssh2-sys gets initialized which should have no impact
libssh2	neither	libgit2 calls libssh2_init(0), possibly causing issues with openssl on Unix (which wants LIBSSH2_INIT_NO_CRYPTO)
exec	neither	should "just work" with exec
neither	exec	attempts to use ssh is runtime error
exec	libssh2	initializes ssh2, but uses exec, should be OK
libssh2	exec	ssh2 may not be initialized properly (same as libssh2/neither above). libssh2::init() isn't called from the rust side, but is called by libgit2's init, which initializes OpenSSL on unix (because of missing LIBSSH2_INIT_NO_CRYPTO)

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 ssh-openssh feature enabled, but if the system git2 is not built with that, then it has no effect which could be confusing.

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?

[ehuss]

Can this (and libgit2-sys/Cargo.toml) comment that the openssh support is experimental with a comment here? Perhaps something like:

Suggested change

	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 ssh from 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.

[bnjmnt4n]

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.

[bnjmnt4n]

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?

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

[ethomson]

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 GIT_FEATURE_SSH) and then also which HTTPS is enabled (IOW, add GIT_FEATURE_SSH_EXEC to disambiguate)?

[extrawurst]

@bnjmnt4n can you link this issue from when you opened one on the libgit2 repo, so we can track progress on this?

[bnjmnt4n]

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 GIT_FEATURE_SSH flag and adding additional flags for SSH backend type e.g. GIT_FEATURE_SSH_EXEC) would work, but someone with more knowledge on how to do this might want to chime in.

[samueltardieu]

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 pkg-config --cflags libgit2 but that seems much more hackier than adding link-time symbols.