vcpkg_from_git: Add support for git over ssh by marcrambo · Pull Request #9446 · microsoft/vcpkg

marcrambo · 2019-12-26T10:28:28Z

vcpkg_from_git currently restricts urls to start with https. This PR removes the restriction and adds support for git over SSH, which is often preferred or even needed for private repos.

I do understand the restriction of https in vcpkg_from_github / vcpkg_from_bitbucket but from my understanding in vcpkg_from_git both protocols should be allowed and work the same way.

The tricky part is to make git over SSH work inside sandboxed vcpkg environment on windows:

users may set GIT_SSH and / or GIT_SSH_COMMAND to point to a custom ssh executable / comand.
users may use ssh-agent in order to facilitate authentications, in which case environment variables SSH_AUTH_SOCK and SSH_AGENT_PID need to be provided.
if GIT_SSH / GIT_SSH_COMMAND is not set, then git will assume ssh.exe is on the PATH. Currently this fails because the PATH is not forwarded to vcpkg. Hence we search for the ssh executable in a sibling directory of the git executable and prepend this directory to the PATH variable. I don't know a better way to do this. I have tested it with git that ships with vcpkg and also with git-for-windows, and it works in both cases. I do not know any other (popular) git clients for windows but I am happy to test with other clients as well.

I have tested all the above combinations of set / unset environment variables and git clients on windows. I have also tested on MacOS to verify that ssh works as expected, but this was already working out of the box, since there is no sandboxing on non-windows platforms.

What does your PR fix? Fixes issues vcpkg_from_git does not work with SSH URL #6126, Allow more transports (e.g. ssh, http) for vcpkg_from_git() #9046
Which triplets are supported/not supported? Have you updated the CI baseline?
This change should be triplet independent. I tested it with x64-windows and x64-macOS
Does your PR follow the maintainer guide?
AFAIK yes.

JackBoosY · 2020-01-14T02:51:14Z

/azp run

JackBoosY · 2020-02-01T05:50:43Z

Maybe we should choose a port as an example to test?

marcrambo · 2020-03-07T13:35:51Z

I rebased and resolved the conflicts. Anything else I can do on my side? Would be nice to merge this one.

strega-nil · 2020-03-10T23:37:42Z

Does this work on Linux/macOS?

marcrambo · 2020-03-11T18:48:07Z

Yes this also works on macos / linux. Actually on these platforms git over ssh was already working, I didn't have to add anything. The only problematic platform was windows because of the clean_environment step.

marcrambo · 2020-03-11T18:52:55Z

As @JackBoosY suggested we could also modify one of the ports to use ssh to verify the ci passes. We probably want to do that on another MR though which we close afterwards. As an alternative I can also push to this branch, and then revert the commit.

strega-nil · 2020-03-11T21:04:22Z

@marcrambo I might just test it locally, that's all it really needs, I think.

I'm going to bring it up at the PR review meeting tomorrow :)

marcrambo · 2020-03-13T18:07:01Z

@strega-nil sounds good. Thanks!

BillyONeal · 2020-03-13T20:06:20Z

toolsrc/src/vcpkg/build.cpp

Should probably "usr/bin/ssh.exe" rather than going through the operator/= logic so many times.

As the path separator is \ on windows this would then look like this:

fs::path git_ssh_exe_path = git_ssh_search_path / "usr\\bin\\ssh.exe"

To be honest I would rather leave it like it is. After all std::filesystem was designed to elegantly handle these platform specific details. I'd favor clarity over performance here and it looks like this is not performance critical code anyways.

You can also do git_ssh_search_path / "usr/bin/ssh.exe"

Right, @strega-nil 's suggestion is what I meant.

strega-nil · 2020-03-13T20:13:00Z

Could we modify the code to append to path, as opposed to prepending to path? There's a concern about overriding some utilities with the path to ssh.

marcrambo · 2020-03-14T16:51:30Z

@strega-nil I have changed the code to append the ssh path as you suggested which is probably safer. Unfortunately the pipeline seems to be broken again, but this seems unrelated to my changes.

JackBoosY · 2020-03-16T02:28:27Z

/azp run

JackBoosY · 2020-03-17T01:23:41Z

/azp run

ras0219-msft · 2020-03-17T19:09:01Z

/azp run

ras0219-msft · 2020-03-18T18:23:56Z

/azp run

christophe-calmejane · 2020-03-27T16:54:37Z

Hi,
I've encountered a critical issue when using the commits from this PR, as described here.
Basically when this PR is merged into the current master, the openssl-windows port is no longer compiling.

marcrambo · 2020-03-27T17:40:55Z

Hi,
I'm having an issue using this patch.
If the remote host is not yet in the ssh known_hosts file, the operation will fail with Host key verification failed.

Thanks for reporting this issue. I haven't tested it but I would say the failure is expected behavior unless you are using StrictHostKeyChecking=no (which I wouldn't recommend) https://askubuntu.com/questions/123072/ssh-automatically-accept-keys. When the host is not listed in the known_hosts file yet, git / ssh will ask the user to accept the key. This requires user interaction which cannot be handled in that case as vcpkg runs unattended.

christophe-calmejane · 2020-03-27T17:48:47Z

Hi,
I'm having an issue using this patch.
If the remote host is not yet in the ssh known_hosts file, the operation will fail with Host key verification failed.

Thanks for reporting this issue. I haven't tested it but I would say the failure is expected behavior unless you are using StrictHostKeyChecking=no (which I wouldn't recommend) https://askubuntu.com/questions/123072/ssh-automatically-accept-keys. When the host is not listed in the known_hosts file yet, git / ssh will ask the user to accept the key. This requires user interaction which cannot be handled in that case as vcpkg runs unattended.

Hi,

yes I imagine it's expected behavior but looking from end-user perspective, it would be nice if we could find a solution for him to know what to do (as the error message is not clear, and he just expects things to just work).
Maybe if it's possible, detect user interaction is required and print a nice message... I don't know (for other people, I do have the solution now :) )

marcrambo · 2020-03-27T18:16:28Z

Hi,
I've encountered a critical issue when using the commits from this PR, as described here.
Basically when this PR is merged into the current master, the openssl-windows port is no longer compiling.

Hi, thanks for reporting. I can reproduce the issue. I guess it has to do with additional binaries that are now visible next to the ssh executable. I'll try to come up with a solution. Probably the only way will be to overwrite the GIT_SSH environment variable.

MVoz · 2020-03-27T21:58:56Z

all you had to do was add a folder to ENV PATH and it's already done

vcpkg/scripts/cmake/vcpkg_find_acquire_program.cmake

Line 98 in 9b44e47

"${DOWNLOADS}/tools/${SUBDIR}/mingw32/bin"

dan-shaw · 2020-03-28T04:11:59Z

@marcrambo This PR looks good and ready to merge. I don't know if you wanted to make any more changes, but let me know

christophe-calmejane · 2020-03-28T09:55:08Z

@dan-shaw What about the critical issue I talked about a few messages above, that breaks compilation?
Hasn't automatic build caught this? (if not there is probably a test missing in unit tests)

This reverts commit 0d608ee.

This reverts commit 377ce3f.

This partially reverts commit 9b81b16.

marcrambo · 2020-03-28T16:20:04Z

all you had to do was add a folder to ENV PATH and it's already done

vcpkg/scripts/cmake/vcpkg_find_acquire_program.cmake

Line 98 in 9b44e47

"${DOWNLOADS}/tools/${SUBDIR}/mingw32/bin"

Yes that's what I thought too. However adding the ssh directory to the path also pulls in quite a lot of unix binaries next to the ssh executable. This seems to affect the build process of open-ssh as reported by @christophe-calmejane , and potentially other packages.

marcrambo · 2020-03-28T16:44:49Z

@dan-shaw @christophe-calmejane I had another look and it turns out the ssh path problem was fixed in upstream git-for-windows. There is no need to modify the path at all anymore, which is great! There is an interesting read on the subject here https://gitter.im/git-for-windows/git?at=5d2d8c4e01621760bcc2e649.

The funny coincidence is that the git version was bumped quite recently in vcpkg: https://github.com/microsoft/vcpkg/pull/10311/files just 2 days before I rebased my PR, so that's a pretty good timing i'd say ;)

@christophe-calmejane thanks again for reporting the issue. It was also quite good timing i'd say ;)
I have tested that openssl successfully builds locally but it would be great if you can confirm it. And also please check that ssh is still working for you.

I have tested extensively all variants of downloaded / system git, with / without GIT_SSH GIT_SSH_COMMAND ... and everything is working as expected.

christophe-calmejane · 2020-03-28T16:52:01Z

@marcrambo I rebased my own work yesterday, and was forced to revert your commits due to openssl compilation issue, and the funny thing I noticed is my ssh repositories were working just fine!
At the time I just suspected it was because there was some sort of cache somewhere or something like that, and couldn't afford to wipe everything to test from scratch.
But based on what you said, it looks like it's actually fixed, which is good news then! I won't be able to test again (from scratch) before Monday, but I'll drop a note here if it's actually good when using vcpkg master branch!
Thanks :)

marcrambo · 2020-03-29T08:25:14Z

@marcrambo I rebased my own work yesterday, and was forced to revert your commits due to openssl compilation issue, and the funny thing I noticed is my ssh repositories were working just fine!
At the time I just suspected it was because there was some sort of cache somewhere or something like that, and couldn't afford to wipe everything to test from scratch.

Yes vcpkg uses a download cache for each package. So even if you remove and reinstall a package, vcpkg will not fetch the source again. This explains why it was still working for you after switching back to master. At least the remaining parts of this PR are needed to unblock non-https urls, and to whitelist some environmental variables for the ssh-agent.

@dan-shaw from my side this PR is good to go. Please feel free to merge.

christophe-calmejane · 2020-03-29T09:03:30Z

I was talking about a cache for SSH, not vcpkg. When I do my tests I always wipe all generated folders (downloads, buildtrees, packages, installed).
I just checked my branch and it was indeed rebased after the change to git version so I guess it was the reason it worked for me once again. I’ll do a new test tomorrow but it looks pretty good to me, gj :)

christophe-calmejane · 2020-03-30T14:21:03Z

@marcrambo I confirm I can build openssl-windows once again, and I can clone using SSH as well.

JackBoosY · 2020-03-31T05:52:38Z

@christophe-calmejane So, does this PR work now?

christophe-calmejane · 2020-03-31T06:44:56Z

@JackBoosY yes it does

dan-shaw · 2020-04-01T14:36:42Z

@marcrambo Thanks for the PR and wait!

@christophe-calmejane thanks for testing!

* vcpkg_from_git: Add support for git over ssh * vcpkg_from_git: append ssh bin directory to path * vcpkg_from_git: fix function signature on non windows platforms * Revert "vcpkg_from_git: fix function signature on non windows platforms" This reverts commit 0d608ee. * Revert "vcpkg_from_git: append ssh bin directory to path" This reverts commit 377ce3f. * Partial Revert "vcpkg_from_git: Add support for git over ssh" This partially reverts commit 9b81b16. Co-authored-by: Marc Boucek <marc.boucek@native-instruments.de> Co-authored-by: Marc Boucek <marc.boucek@posteo.net>

Outurnate mentioned this pull request Dec 26, 2019

[osg] fix non-windows builds #9441

Closed

JackBoosY self-assigned this Dec 27, 2019

JackBoosY added the needs-feature-review label Dec 27, 2019

JackBoosY requested a review from dan-shaw January 9, 2020 09:04

cbezault approved these changes Jan 23, 2020

View reviewed changes

JackBoosY approved these changes Feb 1, 2020

View reviewed changes

marcrambo force-pushed the enable-git-over-ssh branch from 83c4a8d to 60b904a Compare March 7, 2020 13:31

JackBoosY added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Mar 10, 2020

strega-nil self-assigned this Mar 10, 2020

JackBoosY added waiting for response and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Mar 11, 2020

JackBoosY removed the waiting for response label Mar 12, 2020

BillyONeal approved these changes Mar 13, 2020

View reviewed changes

marcrambo force-pushed the enable-git-over-ssh branch from dfd0fc3 to 4da3abf Compare March 14, 2020 16:27

strega-nil assigned dan-shaw and unassigned strega-nil Mar 16, 2020

dan-shaw approved these changes Mar 28, 2020

View reviewed changes

dan-shaw added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed waiting for response labels Mar 28, 2020

Marc Boucek added 3 commits March 28, 2020 11:59

Revert "vcpkg_from_git: fix function signature on non windows platforms"

a878a10

This reverts commit 0d608ee.

Revert "vcpkg_from_git: append ssh bin directory to path"

55f9002

This reverts commit 377ce3f.

Partial Revert "vcpkg_from_git: Add support for git over ssh"

d876447

This partially reverts commit 9b81b16.

JackBoosY added Needs Testing and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Mar 30, 2020

JackBoosY added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed Needs Testing labels Apr 1, 2020

dan-shaw merged commit e1fc03c into microsoft:master Apr 1, 2020

This was referenced Apr 2, 2020

vcpkg_from_git does not work with SSH URL #6126

Closed

Allow more transports (e.g. ssh, http) for vcpkg_from_git() #9046

Closed

Conversation

marcrambo commented Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackBoosY commented Jan 14, 2020

Uh oh!

JackBoosY commented Feb 1, 2020

Uh oh!

marcrambo commented Mar 7, 2020

Uh oh!

strega-nil commented Mar 10, 2020

Uh oh!

marcrambo commented Mar 11, 2020

Uh oh!

marcrambo commented Mar 11, 2020

Uh oh!

strega-nil commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcrambo commented Mar 13, 2020

Uh oh!

BillyONeal Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

marcrambo Mar 14, 2020

Choose a reason for hiding this comment

Uh oh!

strega-nil Mar 15, 2020

Choose a reason for hiding this comment

Uh oh!

BillyONeal Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

strega-nil commented Mar 13, 2020

Uh oh!

marcrambo commented Mar 14, 2020

Uh oh!

JackBoosY commented Mar 16, 2020

Uh oh!

JackBoosY commented Mar 17, 2020

Uh oh!

ras0219-msft commented Mar 17, 2020

Uh oh!

ras0219-msft commented Mar 18, 2020

Uh oh!

christophe-calmejane commented Mar 27, 2020

Uh oh!

marcrambo commented Mar 27, 2020

Uh oh!

christophe-calmejane commented Mar 27, 2020

Uh oh!

marcrambo commented Mar 27, 2020

Uh oh!

MVoz commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dan-shaw commented Mar 28, 2020

Uh oh!

christophe-calmejane commented Mar 28, 2020

Uh oh!

marcrambo commented Mar 28, 2020

Uh oh!

marcrambo commented Mar 28, 2020

Uh oh!

christophe-calmejane commented Mar 28, 2020

Uh oh!

marcrambo commented Mar 29, 2020

Uh oh!

christophe-calmejane commented Mar 29, 2020

Uh oh!

christophe-calmejane commented Mar 30, 2020

Uh oh!

JackBoosY commented Mar 31, 2020

Uh oh!

christophe-calmejane commented Mar 31, 2020

Uh oh!

dan-shaw commented Apr 1, 2020

Uh oh!

Reviewers

Assignees

marcrambo commented Dec 26, 2019 •

edited

Loading

strega-nil commented Mar 11, 2020 •

edited

Loading

MVoz commented Mar 27, 2020 •

edited

Loading