Add OpenSSH_jll as a direct dependency, and force our Git to use OpenSSH_jll (instead of the system ssh)

[DilumAluthge]

Fixes #51, fixes julia-actions/julia-docdeploy#40.

[DilumAluthge]

cc: @mortenpi

[DilumAluthge]

Some of the Cirrus CI jobs are passing in the package test stage, but then failing in the Codecov submission. That should go away once we merge #56.

[DilumAluthge]

The FreeBSD Cirrus CI failure will (hopefully) be fixed by #57.

[giordano]

Locally I already had

diff --git a/Project.toml b/Project.toml
index 3ebb083..512cffa 100644
--- a/Project.toml
+++ b/Project.toml
@@ -1,19 +1,21 @@
 name = "Git"
 uuid = "d7ba0133-e1db-5d97-8f8c-041e4b3a1eb2"
+version = "1.3.2"
 authors = ["Dilum Aluthge", "contributors"]
-version = "1.3.1"
 
 [deps]
 Git_jll = "f8c6e375-362e-5223-8a59-34ff63f689eb"
+JLLWrappers = "692b3bcd-3c85-4b1f-b108-f13ce0eb3210"
+OpenSSH_jll = "9bd350c2-7e96-507f-8002-3f2e150b4e1b"
 
 [compat]
 Git_jll = "2.44"
 JLLWrappers = "1.1"
+OpenSSH_jll = "9, 10"
 julia = "1.6"
 
 [extras]
-JLLWrappers = "692b3bcd-3c85-4b1f-b108-f13ce0eb3210"
 Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
 
 [targets]
-test = ["JLLWrappers", "Test"]
+test = ["Test"]
diff --git a/src/git_function.jl b/src/git_function.jl
index 867f697..a218928 100644
--- a/src/git_function.jl
+++ b/src/git_function.jl
@@ -1,3 +1,6 @@
+using OpenSSH_jll: OpenSSH_jll
+using JLLWrappers: pathsep, LIBPATH_env
+
 """
     git()
 
@@ -18,8 +21,8 @@ julia> run(git(["clone", "https://github.com/JuliaRegistries/General"]))
 to bypass the parsing of the command string.
 """
 function git(; adjust_PATH::Bool = true, adjust_LIBPATH::Bool = true)
-    @static if Sys.iswindows()
-        return Git_jll.git(; adjust_PATH, adjust_LIBPATH)::Cmd
+    git_cmd = @static if Sys.iswindows()
+        Git_jll.git(; adjust_PATH, adjust_LIBPATH)::Cmd
     else
         root = Git_jll.artifact_dir
 
@@ -45,8 +48,24 @@ function git(; adjust_PATH::Bool = true, adjust_LIBPATH::Bool = true)
         end
 
         original_cmd = Git_jll.git(; adjust_PATH, adjust_LIBPATH)::Cmd
-        return addenv(original_cmd, env_mapping...)::Cmd
+        addenv(original_cmd, env_mapping...)::Cmd
     end
+
+    # Use OpenSSH from the JLL: <https://github.com/JuliaVersionControl/Git.jl/issues/51>.
+    if OpenSSH_jll.is_available()
+        path = split(get(ENV, "PATH", ""), pathsep)
+        libpath = split(get(ENV, LIBPATH_env, ""), pathsep)
+
+        path = vcat(dirname(OpenSSH_jll.ssh_path), path)
+        libpath = vcat(OpenSSH_jll.LIBPATH_list, libpath)
+
+        unique!(filter!(!isempty, path))
+        unique!(filter!(!isempty, libpath))
+
+        git_cmd = addenv(git_cmd, "PATH" => join(path, pathsep), LIBPATH_env => join(libpath, pathsep))
+    end
+
+    return git_cmd
 end
 
 function git(args::AbstractVector{<:AbstractString}; kwargs...)

which looks cleaner to me, but I was trying to figure out how to test this (always needs a key pair, which we can't ensure)

[DilumAluthge]

I would probably bump the minor version number, because this feels a bit like a new feature to me, but other than that LGTM.

For testing, can we just generate a new SSH keypair on the fly, and then discard it at the end of our tests? Or does it need to be an SSH keypair that is known to GitHub?

[giordano]

Or does it need to be an SSH keypair that is known to GitHub?

I presume the latter.

[DilumAluthge]

We could add a test that we only do on CI (skip locally).

We can generate a new SSH keypair, and add it to this repo as a deploy key with read-only permissions, and add the private key as a GitHub Actions secret. And then have a CI-only test that uses this SSH deploy key to clone this repo?

[DilumAluthge]

BTW, your diff applies this "prepend OpenSSH_jll" behavior to both Windows and non-Windows. Is this necessary? I was assuming we could just bypass this whole thing on Windows, and just apply this patch for the non-Windows codepath.

[giordano]

Is this necessary?

I don't see why Windows should be any different (besides the fact it's slightly less likely users may have ssh lying around in PATH)

We can generate a new SSH keypair, and add it to this repo as a deploy key with read-only permissions, and add the private key as a GitHub Actions secret. And then have a CI-only test that uses this SSH deploy key to clone this repo?

That could work.

[DilumAluthge]

@giordano You can force-push your version to my branch, and take over this PR (or you can open a new PR if you've already pushed your branch).

[giordano]

The test I had in mind was

@testset "OpenSSH integration" begin
    if # something....
        withtempdir() do dir
            @test !isdir("Git.jl")
            @test !isfile(joinpath("Git.jl", "Project.toml"))
            @test success(`$(git()) clone --depth=1 git@github.com:JuliaVersionControl/Git.jl.git`)
            @test isdir("Git.jl")
            @test isfile(joinpath("Git.jl", "Project.toml"))
        end
    end
end

Would you be able to finish up the testing part?

[DilumAluthge]

I don't see why Windows should be any different (besides the fact it's slightly less likely users may have ssh lying around in PATH)

My thinking is:

1. We (Yggdrasil) don't build Git binaries for Git_jll on Windows (we just re-ship the tarballs from upstream https://github.com/git-for-windows/git/releases, if I understand correctly). So on Windows, there's no reason that the Git binaries would be built against our OpenSSH_jll.
2. If I understand correctly, Git for Windows ships with its own SSH client (as part of the Git Bash that is shipped with Git for Windows). So I assume that the Git for Windows binaries are automatically configured out-of-the-box to use their own SSH client.

[DilumAluthge]

I just checked on a Windows machine that I have. When I open Git Bash, the first SSH client in the PATH is the SSH that ships built-in to Git Bash.

[DilumAluthge]

Would you be able to finish up the testing part?

Yep. I will create the deploy key and push the tests shortly.

[DilumAluthge]

I have:

1. Created the keypair, added the public key as deploy key (read-only), added the private key as secret.
2. Pushed a commit with the tests.

[DilumAluthge]

Ubuntu CI is failing with:

/home/runner/.julia/artifacts/9bfc066f45d9299c72b5f6928c8618c066904c83/bin/git: error while loading shared libraries: libiconv.so.2: cannot open shared object file: No such file or directory

[giordano]

I don't see ssh.exe in /bin in https://github.com/JuliaBinaryWrappers/Git_jll.jl/releases/download/Git-v2.49.0%2B0/Git.v2.49.0.x86_64-w64-mingw32.tar.gz, so I don't understand how that should be picked up.

[DilumAluthge]

Ah, I see, so maybe we aren't including it when we re-bundle the upstream Git for Windows?

[DilumAluthge]

I don't see ssh.exe in /bin

I do see it in /usr/bin FWIW

[DilumAluthge]

On a separate note, on Julia 1.6, Windows tests are failing:

IOError: could not spawn setenv('C:\Users\runneradmin\.julia\artifacts\9148fbb39a57f9ad39d560c68427a823b4a8afb1\bin\git.exe' clone --quiet [https://github.com/JuliaVersionControl/Git.jl," ...

And this is a non-SSH URL.

[DilumAluthge]

Of note, Windows is passing on Julia 1.11, but failing on Julia 1.6.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.96%. Comparing base (f818f5a) to head (472fdff).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
+ Coverage   95.45%   96.96%   +1.51%     
==========================================
  Files           1        1              
  Lines          22       33      +11     
==========================================
+ Hits           21       32      +11     
  Misses          1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[giordano]

I do see it in /usr/bin FWIW

I have no idea how the git executable finds ssh.exe. It's not in the same directory, unless it somehow sets PATH internally or manually reference the relative path to that one I don't think that executable is ever called at any point. Existing != being called.

[DilumAluthge]

Alright, so I did some testing on a Windows machine, and here is what I found.

First, I opened Task Manager, and I made sure that there were no ssh processes running.

Then, I opened Julia, and did using Git: git (where I have Git.jl version v1.3.1, with Julia 1.11.5).

Then I did:

using Git: git

run(`$(git()) clone git@github.com:JuliaRegistries/General.git`)

I don't have any known_hosts file yet, so this second line hangs and waits for me to accept or reject the fingerprint for github.com. I leave the Julia REPL as-is, and I go back to Task Manager. I'm now able to see the ssh process running. In Task Manager, when I pull up the details for that ssh process, here's what I see:

Screenshot 1 of 3:

Screenshot 2 of 3:

Screenshot 3 of 3 (I'm hovering over the "path" so that the full path is shown):

So, from this testing, it looks to me that when I use Git.jl (latest registered version) on Windows, by default it will use the ssh.exe that is bundled inside ./usr/bin inside our artifacts.

I assume Git-for-Windows does some magic to make sure that their own SSH gets picked up, instead of a system SSH.

[fingolfin]

Could we get this into a release?

[giordano]

It's in 1.4.0, isn't it?

[fingolfin]

Sorry I only looked at https://github.com/JuliaVersionControl/Git.jl/releases and it is not listed there. So I assume TagBot integration here is broken?

Indeed, your commit fc28591 likely broke it. Indeed, the permissions block has to be commented out. And then the TagBot documentation ought to be adjusted to not suggest to people to use this, as it doesn't work (I found this out the hard work when I recently worked on https://github.com/JuliaCollections/DataStructures.jl )

[giordano]

And then the TagBot documentation ought to be adjusted to not suggest to people to use this, as it doesn't work

Yeah, well, I copied from there, if that's wrong it's broken for everyone.

[fingolfin]

it is indeed broken for "everyone". I'll report to TagBot

[fingolfin]

ah actually I did report it in March (phew! I felt bad because I thought "why the heck did I not report this when I run into it).

Unfortunately I got no reactions by anyone "in charge". I have no idea who that would be ... :-(