Skip to content
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 privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(git-spawn): Add custom Batch wrapper when using plink.exe (#4729) #4805

Closed
wants to merge 1 commit into from

Conversation

neonowy
Copy link
Contributor

@neonowy neonowy commented Oct 30, 2017

Summary

Fixes #4729.

The main issue is that we use GIT_SSH_COMMAND instead of GIT_SSH, as we have to pass -batch argument in order to use ssh/plink in the non-interactive mode. Unfortunately, Git converts -p to -P for plink.exe only when using GIT_SSH.

The only way I could find to get out of this tricky issue is to create a wrapper script responsible for the -p/-P conversion and use it when plink.exe is provided via GIT_SSH.

Similar issue was also described here: https://jira.atlassian.com/browse/SRCTREEWIN-5600?page=com.atlassian.jira.plugin.system.issuetabpanels%3Aall-tabpanel

I'm not quite sure if the bin/ directory is the best place for the wrapper script yarn-plink.cmd, would scripts/ be a better choice?

Test plan

Before:

virtualbox_msedge_-_win10_30_10_2017_16_35_24

After:

virtualbox_msedge_-_win10_30_10_2017_16_31_42

I've also updated the test suite for git-spawn.js accordingly, but couldn't figure out how to test the yarn-plink.cmd wrapper script.

@buildsize
Copy link

buildsize bot commented Oct 30, 2017

This change will increase the build size from 9.94 MB to 9.94 MB, an increase of 1.71 KB (0%)

File name Previous Size New Size Change
yarn-[version].noarch.rpm 859.9 KB 860.36 KB 467 bytes (0%)
yarn-[version].js 3.78 MB 3.78 MB 264 bytes (0%)
yarn-legacy-[version].js 3.83 MB 3.83 MB 290 bytes (0%)
yarn-v[version].tar.gz 865.48 KB 865.79 KB 316 bytes (0%)
yarn_[version]all.deb 654.43 KB 654.84 KB 418 bytes (0%)

1 similar comment
@buildsize
Copy link

buildsize bot commented Oct 30, 2017

This change will increase the build size from 9.94 MB to 9.94 MB, an increase of 1.71 KB (0%)

File name Previous Size New Size Change
yarn-[version].noarch.rpm 859.9 KB 860.36 KB 467 bytes (0%)
yarn-[version].js 3.78 MB 3.78 MB 264 bytes (0%)
yarn-legacy-[version].js 3.83 MB 3.83 MB 290 bytes (0%)
yarn-v[version].tar.gz 865.48 KB 865.79 KB 316 bytes (0%)
yarn_[version]all.deb 654.43 KB 654.84 KB 418 bytes (0%)

@arcanis
Copy link
Member

arcanis commented Oct 30, 2017

Unfortunately that won't work with the single-file releases, so I don't think we can merge it :(

@arcanis arcanis self-assigned this Oct 30, 2017
@neonowy
Copy link
Contributor Author

neonowy commented Oct 30, 2017

@arcanis Oh, I didn't think of that. 😢

But actually, I think I found another (even better) way of doing this! Apparently GIT_SSH_VARIANT variable was added to Git, so we can just set it properly and it should work.

@BYK
Copy link
Member

BYK commented Oct 30, 2017

@neonowy are you sure it is not this line forcing everything to be lower case:

const sshExecutable = path.basename(sshCommand.toLowerCase(), '.exe');

@neonowy
Copy link
Contributor Author

neonowy commented Oct 30, 2017

@BYK I've tested it and it doesn't make any difference. If I understand this code right, we're using sshCommand directly when setting env.GIT_SSH_COMMAND and sshExecutable is only used as a normalized key for BATCH_MODE_ARGS here.

@neonowy
Copy link
Contributor Author

neonowy commented Oct 30, 2017

Closing in favor of #4806.

@neonowy neonowy closed this Oct 30, 2017
BYK pushed a commit that referenced this pull request Oct 31, 2017
**Summary**

Fixes #4729.
Previous version in #4805.

Manually specify `GIT_SSH_VARIANT` in order to get package download via `git+ssh` with a non-standard port when using `plink.exe` working.

Without `GIT_SSH_VARIANT` set properly, Git won't convert `-p` into `-P` and `plink.exe` will throw an error about unknown `-p` parameter.

**Test plan**

*Before:*
![virtualbox_msedge_-_win10_30_10_2017_16_35_24](https://user-images.githubusercontent.com/5042328/32179804-9a87c676-bd90-11e7-86d0-09380d61eadf.png)

*After:*
![virtualbox_msedge_-_win10_30_10_2017_19_07_15](https://user-images.githubusercontent.com/5042328/32187512-9bcb980e-bda5-11e7-96ea-27a513837d6e.png)

Also got `git-spawn.js` test suite updated for testing `GIT_SSH_VARIANT`.
calvinhuang pushed a commit to calvinhuang/yarn that referenced this pull request Nov 9, 2017
**Summary**

Fixes yarnpkg#4729.
Previous version in yarnpkg#4805.

Manually specify `GIT_SSH_VARIANT` in order to get package download via `git+ssh` with a non-standard port when using `plink.exe` working.

Without `GIT_SSH_VARIANT` set properly, Git won't convert `-p` into `-P` and `plink.exe` will throw an error about unknown `-p` parameter.

**Test plan**

*Before:*
![virtualbox_msedge_-_win10_30_10_2017_16_35_24](https://user-images.githubusercontent.com/5042328/32179804-9a87c676-bd90-11e7-86d0-09380d61eadf.png)

*After:*
![virtualbox_msedge_-_win10_30_10_2017_19_07_15](https://user-images.githubusercontent.com/5042328/32187512-9bcb980e-bda5-11e7-96ea-27a513837d6e.png)

Also got `git-spawn.js` test suite updated for testing `GIT_SSH_VARIANT`.
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.

3 participants