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

Support MSYS{2} bash in bin/yarn #5859

Merged
merged 1 commit into from
May 24, 2018
Merged

Conversation

akrieger
Copy link
Contributor

@akrieger akrieger commented May 23, 2018

Summary
Add a case in bin/yarn to run cygpath on the yarn base dir if the environment is MSYS.

Automatic path translation happens differently, or perhaps conditionally, depending on whether msys detects the target binary is a native Win32 application or not. While bash from Git for Windows seems to treat winpty as a Win32 binary, the version of winpty which is compatible with MSYS2 does not trigger the path conversion.

Test plan
yarn executed from within bash within MSYS2 correct executes, instead of failing with a path
related error. yarn continues to execute correctly from within Git Bash.

// Changes were made to system install to test more easily.
$ which yarn
/c/Program Files (x86)/Yarn/bin/yarn
akrieger@AKRIEGER-701 /c/Users/akrieger
$ yarn
yarn install v1.6.0
info No lockfile found.
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...

The path translation happens differently, or perhaps conditionally dependdding on whether msys detects
the target binary is a native Win32 application or not. While Git Bash seems to treat its `winpty` as
a Win32 binary, the version of `winpty` which is compatible with MSYS2 does not trigger the path conversion.
So, add a case for it to first run `cygpath` on the base dir.

To test, `yarn` executed from within bash within MSYS2 correct executes, instead of failing with a path
related error. `yarn` continues to execute correctly from within Git Bash.
@BYK BYK requested a review from Daniel15 May 24, 2018 12:00
@BYK
Copy link
Member

BYK commented May 24, 2018

Looks and sounds good to me but calling our expert, @Daniel15, on this before merging.

Copy link
Member

@Daniel15 Daniel15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, seems fine to me

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