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.

Sign up for GitHub

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

Jump to bottom

Change use_winpty logic in bin/yarn #3245

Merged
merged 1 commit into from
Apr 26, 2017
Merged

Change use_winpty logic in bin/yarn #3245

merged 1 commit into from
Apr 26, 2017

Conversation

mikew
Copy link
Contributor

@mikew mikew commented Apr 24, 2017

Summary

There used to be issues with Yarn on Windows that required the use of winpty:

That fix caused issues down the line when some part of the Windows/Git/Bash toolchain was updated:

This change removes the troublesome auto detection of whether or not winpty is needed and relegates it to the environment variable YARN_FORCE_WINPTY

Test plan

Simple change of already existing logic. I'm not sure if there are any tests for it, but:

@bestander bestander merged commit a45a677 into yarnpkg:master Apr 26, 2017
@bestander bestander mentioned this pull request Apr 26, 2017
Merged
This was referenced May 4, 2017
Closed
Closed
@thetrompf thetrompf mentioned this pull request Sep 28, 2017
Merged
BYK pushed a commit that referenced this pull request Sep 29, 2017
@thetrompf @BYK
fix(windows): Detect when winpty is present and if the current termin…
75bc450 
…al is a TTY (#4577)

**Summary**

This addresses some of the windows issues regarding running yarn in gitbash and friends envrionment.
with this fix I keep the behavior introduced 5 months ago in #3245, but try to do a better job detecting when to use winpty out of the box, in order make `piping` of output work with yarn.

Before this fix:
```shell
$ yarn --version
1.1.0
$ yarn --version | cat
1.1.0
$ yarn init
yarn init v1.1.0
error An unexpected error occurred: "Can't answer a question unless a user TTY".
info If you think this is a bug, please open a bug report with the information provided in "D:\\workspace\\yarn\\yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/init for documentation about this command.
```

Piping works for simple commands, but interactive commands only work with an environment set to something. This prohibits scripts/tools around yarn that uses pipe, which is quite common to do in an unix like environment, but theses tools cannot work in windows' unix like environment.

WinPTY seems to be the savior here, but we need to only run yarn through winpty when a tty actually needs to be allocated. 

Previous attempts to solve this problem like: 
- #2230
- #2243

Did not address the use cases of piping, so they essentially broke that behavior.
Then #3245 fixed that, but now you have to use `YARN_FORCE_WINPTY=1` environment variable in order for `yarn init` and `yarn upgrade-interactive` to work and that's alright, but if you export that variable then piping is broken yet again because the variable will also be set in the piped command, and we haven't solved any problem.

I suggest we keep the environment variable behavior but open up for better detection when to use winpty out of the box.

This fix detects if the winpty binary is in path, and only use it if stdin is in fact a TTY: `test -t 1`. 

**Test plan**

The output of running:
- `yarn init`
- `yarn upgrade-interactive`
- `yarn --version | cat`

Without having the `YARN_FORCE_WINPTY=1` environment variable set.

```shell
$ ./bin/yarn init
yarn init v1.1.0
question name (yarn):

$ ./bin/yarn upgrade-interactive
yarn upgrade-interactive v1.1.0
info Color legend :
 "<red>"    : Major Update backward-incompatible updates
 "<yellow>" : Minor Update backward-compatible features
 "<green>"  : Patch Update backward-compatible bug fixes
? Choose which packages to update. (Press <space> to select, <a> to toggle all, <i> to inverse selection)
 devDependencies
   name                         range                      from               to             url
>( ) babel-core                   ^6.24.1                    6.24.1          ❯  6.26.0         https://babeljs.io/
 ( ) babylon                      ^6.5.0                     6.17.1          ❯  6.18.0         https://babeljs.io/
 ( ) eslint                       ^4.3.0                     4.3.0           ❯  4.7.2          http://eslint.org
 ( ) eslint-config-fb-strict      ^20.1.0-delta.3            20.1.0-delta.3  ❯  20.1.0-echo.1  https://github.com/facebook/jest#readme
 ( ) eslint-plugin-babel          ^4.0.0                     4.1.1           ❯  4.1.2          https://github.com/babel/eslint-plugin-babel#readme
 ( ) eslint-plugin-flowtype       ^2.35.0                    2.35.0          ❯  2.36.0         https://github.com/gajus/eslint-plugin-flowtype#readme
 ( ) eslint-plugin-jasmine        ^2.6.2                     2.6.2           ❯  2.8.4          https://github.com/tlvince/eslint-plugin-jasmine
 ( ) eslint-plugin-prettier       ^2.1.2                     2.1.2           ❯  2.3.1          https://github.com/prettier/eslint-plugin-prettier#readme
 ( ) eslint-plugin-react          ^7.1.0                     7.1.0           ❯  7.4.0          https://github.com/yannickcr/eslint-plugin-react
 ( ) eslint-plugin-yarn-internal  file:scripts/eslint-rules  0.0.0           ❯  exotic         file:scripts/eslint-rules
 ( ) gulp-sourcemaps              ^2.2.0                     2.6.0           ❯  2.6.1          http://github.com/gulp-sourcemaps/gulp-sourcemaps
 ( ) prettier                     ^1.5.2                     1.5.2           ❯  1.7.2          https://prettier.io
 ( ) webpack                      ^2.1.0-beta.25             2.6.0           ❯  2.7.0          https://github.com/webpack/webpack

 dependencies
   name                         range                      from               to             url
 ( ) babel-runtime                ^6.0.0                     6.23.0          ❯  6.26.0         https://github.com/babel/babel/tree/master/packages/babel-runtime
 ( ) commander                    ^2.9.0                     2.9.0           ❯  2.11.0         https://github.com/tj/commander.js#readme
 ( ) debug                        ^2.2.0                     2.6.8           ❯  2.6.9          https://github.com/visionmedia/debug#readme
 ( ) gunzip-maybe                 ^1.4.0                     1.4.0           ❯  1.4.1          https://github.com/mafintosh/gunzip-maybe
 ( ) inquirer                     ^3.0.1                     3.0.6           ❯  3.3.0          https://github.com/SBoudrias/Inquirer.js#readme
 ( ) node-emoji                   ^1.6.1                     1.6.1           ❯  1.8.1          https://github.com/omnidan/node-emoji#readme
 ( ) request                      ^2.81.0                    2.81.0          ❯  2.83.0         https://github.com/request/request#readme
 ( ) rimraf                       ^2.5.0                     2.6.1           ❯  2.6.2          https://github.com/isaacs/rimraf#readme
 ( ) semver                       ^5.1.0                     5.3.0           ❯  5.4.1          https://github.com/npm/node-semver#readme
 ( ) tar-fs                       ^1.15.1                    1.15.2          ❯  1.15.3         https://github.com/mafintosh/tar-fs
 ( ) uuid                         ^3.0.1                     3.0.1           ❯  3.1.0          https://github.com/kelektiv/node-uuid#readme

$ ./bin/yarn --version | cat
1.1.0
$
```

And importantly when running the interactive commands through a pipe, it will correctly fail by saying you not are running the interactive commands in a TTY:

```shell
 $ ./bin/yarn init | cat
yarn init v1.1.0
error An unexpected error occurred: "Can't answer a question unless a user TTY".
info If you think this is a bug, please open a bug report with the information provided in "D:\\workspace\\yarn\\yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/init for documentation about this command.

 $ ./bin/yarn upgrade-interactive | cat
yarn upgrade-interactive v1.1.0
info Color legend :
 "<red>"    : Major Update backward-incompatible updates
 "<yellow>" : Minor Update backward-compatible features
 "<green>"  : Patch Update backward-compatible bug fixes
Done in 1.43s.
Error: Can't answer a question unless a user TTY
    at D:\workspace\yarn\lib\reporters\console\console-reporter.js:487:31
    at Generator.next (<anonymous>)
    at step (D:\workspace\yarn\node_modules\babel-runtime\helpers\asyncToGenerator.js:17:30)
    at D:\workspace\yarn\node_modules\babel-runtime\helpers\asyncToGenerator.js:35:14
    at Promise (<anonymous>)
    at F (D:\workspace\yarn\node_modules\core-js\library\modules\_export.js:35:28)
    at D:\workspace\yarn\node_modules\babel-runtime\helpers\asyncToGenerator.js:14:12
    at ConsoleReporter.prompt (D:\workspace\yarn\lib\reporters\console\console-reporter.js:518:7)
    at Object.<anonymous> (D:\workspace\yarn\lib\cli\commands\upgrade-interactive.js:116:38)
    at Generator.next (<anonymous>)
```
@GulajavaMinistudio GulajavaMinistudio mentioned this pull request Sep 30, 2017
Merged
joaolucasl pushed a commit to joaolucasl/yarn that referenced this pull request Oct 27, 2017
@thetrompf @joaolucasl
fix(windows): Detect when winpty is present and if the current termin…
add0aed 
…al is a TTY (yarnpkg#4577)

**Summary**

This addresses some of the windows issues regarding running yarn in gitbash and friends envrionment.
with this fix I keep the behavior introduced 5 months ago in yarnpkg#3245, but try to do a better job detecting when to use winpty out of the box, in order make `piping` of output work with yarn.

Before this fix:
```shell
$ yarn --version
1.1.0
$ yarn --version | cat
1.1.0
$ yarn init
yarn init v1.1.0
error An unexpected error occurred: "Can't answer a question unless a user TTY".
info If you think this is a bug, please open a bug report with the information provided in "D:\\workspace\\yarn\\yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/init for documentation about this command.
```

Piping works for simple commands, but interactive commands only work with an environment set to something. This prohibits scripts/tools around yarn that uses pipe, which is quite common to do in an unix like environment, but theses tools cannot work in windows' unix like environment.

WinPTY seems to be the savior here, but we need to only run yarn through winpty when a tty actually needs to be allocated. 

Previous attempts to solve this problem like: 
- yarnpkg#2230
- yarnpkg#2243

Did not address the use cases of piping, so they essentially broke that behavior.
Then yarnpkg#3245 fixed that, but now you have to use `YARN_FORCE_WINPTY=1` environment variable in order for `yarn init` and `yarn upgrade-interactive` to work and that's alright, but if you export that variable then piping is broken yet again because the variable will also be set in the piped command, and we haven't solved any problem.

I suggest we keep the environment variable behavior but open up for better detection when to use winpty out of the box.

This fix detects if the winpty binary is in path, and only use it if stdin is in fact a TTY: `test -t 1`. 

**Test plan**

The output of running:
- `yarn init`
- `yarn upgrade-interactive`
- `yarn --version | cat`

Without having the `YARN_FORCE_WINPTY=1` environment variable set.

```shell
$ ./bin/yarn init
yarn init v1.1.0
question name (yarn):

$ ./bin/yarn upgrade-interactive
yarn upgrade-interactive v1.1.0
info Color legend :
 "<red>"    : Major Update backward-incompatible updates
 "<yellow>" : Minor Update backward-compatible features
 "<green>"  : Patch Update backward-compatible bug fixes
? Choose which packages to update. (Press <space> to select, <a> to toggle all, <i> to inverse selection)
 devDependencies
   name                         range                      from               to             url
>( ) babel-core                   ^6.24.1                    6.24.1          ❯  6.26.0         https://babeljs.io/
 ( ) babylon                      ^6.5.0                     6.17.1          ❯  6.18.0         https://babeljs.io/
 ( ) eslint                       ^4.3.0                     4.3.0           ❯  4.7.2          http://eslint.org
 ( ) eslint-config-fb-strict      ^20.1.0-delta.3            20.1.0-delta.3  ❯  20.1.0-echo.1  https://github.com/facebook/jest#readme
 ( ) eslint-plugin-babel          ^4.0.0                     4.1.1           ❯  4.1.2          https://github.com/babel/eslint-plugin-babel#readme
 ( ) eslint-plugin-flowtype       ^2.35.0                    2.35.0          ❯  2.36.0         https://github.com/gajus/eslint-plugin-flowtype#readme
 ( ) eslint-plugin-jasmine        ^2.6.2                     2.6.2           ❯  2.8.4          https://github.com/tlvince/eslint-plugin-jasmine
 ( ) eslint-plugin-prettier       ^2.1.2                     2.1.2           ❯  2.3.1          https://github.com/prettier/eslint-plugin-prettier#readme
 ( ) eslint-plugin-react          ^7.1.0                     7.1.0           ❯  7.4.0          https://github.com/yannickcr/eslint-plugin-react
 ( ) eslint-plugin-yarn-internal  file:scripts/eslint-rules  0.0.0           ❯  exotic         file:scripts/eslint-rules
 ( ) gulp-sourcemaps              ^2.2.0                     2.6.0           ❯  2.6.1          http://github.com/gulp-sourcemaps/gulp-sourcemaps
 ( ) prettier                     ^1.5.2                     1.5.2           ❯  1.7.2          https://prettier.io
 ( ) webpack                      ^2.1.0-beta.25             2.6.0           ❯  2.7.0          https://github.com/webpack/webpack

 dependencies
   name                         range                      from               to             url
 ( ) babel-runtime                ^6.0.0                     6.23.0          ❯  6.26.0         https://github.com/babel/babel/tree/master/packages/babel-runtime
 ( ) commander                    ^2.9.0                     2.9.0           ❯  2.11.0         https://github.com/tj/commander.js#readme
 ( ) debug                        ^2.2.0                     2.6.8           ❯  2.6.9          https://github.com/visionmedia/debug#readme
 ( ) gunzip-maybe                 ^1.4.0                     1.4.0           ❯  1.4.1          https://github.com/mafintosh/gunzip-maybe
 ( ) inquirer                     ^3.0.1                     3.0.6           ❯  3.3.0          https://github.com/SBoudrias/Inquirer.js#readme
 ( ) node-emoji                   ^1.6.1                     1.6.1           ❯  1.8.1          https://github.com/omnidan/node-emoji#readme
 ( ) request                      ^2.81.0                    2.81.0          ❯  2.83.0         https://github.com/request/request#readme
 ( ) rimraf                       ^2.5.0                     2.6.1           ❯  2.6.2          https://github.com/isaacs/rimraf#readme
 ( ) semver                       ^5.1.0                     5.3.0           ❯  5.4.1          https://github.com/npm/node-semver#readme
 ( ) tar-fs                       ^1.15.1                    1.15.2          ❯  1.15.3         https://github.com/mafintosh/tar-fs
 ( ) uuid                         ^3.0.1                     3.0.1           ❯  3.1.0          https://github.com/kelektiv/node-uuid#readme

$ ./bin/yarn --version | cat
1.1.0
$
```

And importantly when running the interactive commands through a pipe, it will correctly fail by saying you not are running the interactive commands in a TTY:

```shell
 $ ./bin/yarn init | cat
yarn init v1.1.0
error An unexpected error occurred: "Can't answer a question unless a user TTY".
info If you think this is a bug, please open a bug report with the information provided in "D:\\workspace\\yarn\\yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/init for documentation about this command.

 $ ./bin/yarn upgrade-interactive | cat
yarn upgrade-interactive v1.1.0
info Color legend :
 "<red>"    : Major Update backward-incompatible updates
 "<yellow>" : Minor Update backward-compatible features
 "<green>"  : Patch Update backward-compatible bug fixes
Done in 1.43s.
Error: Can't answer a question unless a user TTY
    at D:\workspace\yarn\lib\reporters\console\console-reporter.js:487:31
    at Generator.next (<anonymous>)
    at step (D:\workspace\yarn\node_modules\babel-runtime\helpers\asyncToGenerator.js:17:30)
    at D:\workspace\yarn\node_modules\babel-runtime\helpers\asyncToGenerator.js:35:14
    at Promise (<anonymous>)
    at F (D:\workspace\yarn\node_modules\core-js\library\modules\_export.js:35:28)
    at D:\workspace\yarn\node_modules\babel-runtime\helpers\asyncToGenerator.js:14:12
    at ConsoleReporter.prompt (D:\workspace\yarn\lib\reporters\console\console-reporter.js:518:7)
    at Object.<anonymous> (D:\workspace\yarn\lib\cli\commands\upgrade-interactive.js:116:38)
    at Generator.next (<anonymous>)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mikew @bestander