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(windows): Detect when winpty is present and if the current terminal is a TTY #4577

Merged
merged 1 commit into from
Sep 29, 2017

Conversation

thetrompf
Copy link
Contributor

@thetrompf thetrompf commented Sep 28, 2017

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:

$ 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:

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.

$ ./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:

 $ ./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>)

@thetrompf
Copy link
Contributor Author

There are also mentions of this problem in #2591 and #2998.

@thetrompf
Copy link
Contributor Author

It appears that the build fails due to some callback was not invoked within the default timeout?

@buildsize
Copy link

buildsize bot commented Sep 29, 2017

This change will decrease the build size from 9.83 MB to 9.83 MB, a decrease of 3.9 KB (0%)

File name Previous Size New Size Change
yarn-[version].noarch.rpm 848.45 KB 848.03 KB -426 bytes (0%)
yarn-[version].js 3.74 MB 3.74 MB -1.18 KB (0%)
yarn-legacy-[version].js 3.79 MB 3.79 MB -1.34 KB (0%)
yarn-v[version].tar.gz 854.17 KB 853.69 KB -487 bytes (0%)
yarn_[version]all.deb 645.48 KB 645 KB -496 bytes (0%)

1 similar comment
@buildsize
Copy link

buildsize bot commented Sep 29, 2017

This change will decrease the build size from 9.83 MB to 9.83 MB, a decrease of 3.9 KB (0%)

File name Previous Size New Size Change
yarn-[version].noarch.rpm 848.45 KB 848.03 KB -426 bytes (0%)
yarn-[version].js 3.74 MB 3.74 MB -1.18 KB (0%)
yarn-legacy-[version].js 3.79 MB 3.79 MB -1.34 KB (0%)
yarn-v[version].tar.gz 854.17 KB 853.69 KB -487 bytes (0%)
yarn_[version]all.deb 645.48 KB 645 KB -496 bytes (0%)

@BYK BYK self-assigned this Sep 29, 2017
Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Thanks a lot! I'm not sure if this will really fix it but worth a try!

@BYK BYK changed the title Detect when winpty is present and if the current terminal is a TTY Fix: Detect when winpty is present and if the current terminal is a TTY Sep 29, 2017
@BYK BYK changed the title Fix: Detect when winpty is present and if the current terminal is a TTY fix(windows): Detect when winpty is present and if the current terminal is a TTY Sep 29, 2017
@BYK BYK merged commit 75bc450 into yarnpkg:master Sep 29, 2017
@thetrompf thetrompf deleted the detect-tty-and-winpty branch October 2, 2017 07:45
@inosik
Copy link

inosik commented Oct 12, 2017

This breaks Git Bash in ConsoleZ on Windows 7. Yarn 1.1.0 worked properly, since 1.2.0 the terminal doesn't output anything anymore after invoking Yarn.

@thetrompf
Copy link
Contributor Author

@inosik can try invoking yarn with YARN_FORCE_WINPTY=1 yarn in 1.1.0?
And what is the output of which winpty?

@inosik
Copy link

inosik commented Oct 13, 2017

invoking yarn with YARN_FORCE_WINPTY=1 yarn in 1.1.0?

I get the same behavior as it is with 1.2.1, the terminal doesn't print any output anymore.

which winpty gives me this:

$ which winpty
/usr/bin/winpty

@thetrompf
Copy link
Contributor Author

I just tried installing ConsoleZ and running Git Bash in it without any trouble.
untitled

What happens if you run npm init ?

@thetrompf
Copy link
Contributor Author

thetrompf commented Oct 16, 2017

Whoops - very old version of yarn, I'll try 1.21 sorry.. 2 sec

@thetrompf
Copy link
Contributor Author

Works fine with the 1.21 as well.
untitled

@inosik
Copy link

inosik commented Oct 16, 2017

What happens if you run npm init ?

I'm using the default NPM which comes with Node.js LTS, and I never had problems with it. But I can try again tomorrow, when I'm in the office again.

Is it possible that the encoding of the shell causes this? I'm using a German Windows 7 and the default encoding on almost every terminal emulator is IBM850. I can also try Hyper, which uses UTF8 on my PC.

@inosik
Copy link

inosik commented Oct 17, 2017

I tried NPM 3.10.10 and 5.5.1, both of them work as expected. I also tried Hyper and that didn't make any difference.

@thetrompf
Copy link
Contributor Author

I cannot reproduce this behavior, even with the node version you are posting. And since Console Z har nothing to do with terminal behavior, it is just a host for other terminal programs like Git Bash, cmd and PowerShell and is not at terminal itself, I really think it has something to do with your Git Bash version.

Can you please try installing the newest Git bash?

@inosik
Copy link

inosik commented Oct 19, 2017

After trying several things on the machine in question, I gave up and booted up my dusty Windows 8.1 on my Mac and tried Yarn there. It worked as expected, and I got the TTY error message when reverting this change.

It seems that this is either a problem with my setup (which wouldn't be the first time), or it's a bug in winpty, which is specific to Windows 7. I guess I'll use yarn.cmd in Bash for now.

Thank you, @thetrompf, I really appreciate your efforts!

@Zeroto
Copy link

Zeroto commented Oct 20, 2017

I don't think it is limited to that. I run windows 10 and have the same problem. I updated from an old version of yarn (0.24 I think) to 1.2.1 and now it fails in cmder+bash. If I comment out the check and winpty then it works correct for me.

@thetrompf
Copy link
Contributor Author

thetrompf commented Oct 23, 2017

@Zeroto Have you tried to use the newest version of Git bash? winpty is bundled with Git bash. What is the output of winpty --version?
I get:

$ winpty --version
winpty version 0.4.0-dev
commit none

@inosik
Copy link

inosik commented Oct 24, 2017

@thetrompf I get this:

winpty version 0.4.3
commit none

I got it with Git for Windows 2.14.3.windows.1, installed with Scoop. Do you have the latest Git for Windows?

@Zeroto
Copy link

Zeroto commented Oct 24, 2017

@thetrompf I get

λ winpty --version
winpty version 0.2.2-dev
commit none

@thetrompf
Copy link
Contributor Author

thetrompf commented Oct 24, 2017

Now I have tried the absolute newest versions of git bash, winpty and yarn, and it just works.

$ winpty --version
winpty version 0.4.3
commit none

$ bash --version
GNU bash, version 4.4.12(1)-release (x86_64-pc-msys)
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

$ yarn init
yarn init v1.2.1
question name (bkc):

I conclude if you run the somewhat newest versions it works, and it works if you run the absolute newest.
@Zeroto have you tried installing v2.14.3 from https://git-scm.com/download/win and see if it helps?

joaolucasl pushed a commit to joaolucasl/yarn that referenced this pull request Oct 27, 2017
…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>)
```
@lanoxx
Copy link

lanoxx commented Nov 6, 2017

I just installed yarn 1.3.2 on Windows and try to use it in git bash and I experience the above error. Is this supposed to be fixed? When I run yarn in the windows CMD it works but not in git bash.

@thetrompf
Copy link
Contributor Author

Are you running the newest git bash? What is the winpty version, OS version, and are you running git bash inside another terminal emulator / host program like ConsoleZ?

@lanoxx
Copy link

lanoxx commented Nov 6, 2017

From my memory: winpty version is 0.4.3.
git bash was updated today (2.15 IIRC). OS is Windows 7 64bit. I am runnig git bash in the mintty that comes with git bash. There is no other program / console inbetween.

@thetrompf
Copy link
Contributor Author

thetrompf commented Nov 6, 2017 via email

@lanoxx
Copy link

lanoxx commented Nov 6, 2017

Can someone either confirm that it works on Windows 7 or maybe provide a suggestion how I can investigate this further.

@Muhand
Copy link

Muhand commented Dec 6, 2017

Still not working on windows 10.

winpty version is 0.4.3
bash version 4.4.12
yarn version 1.3.2
node version 8.9.1

@thetrompf
Copy link
Contributor Author

It works fine on windows 10,as you can see in this thread I have run it a lot of different emulators besides from the "standard" git bash, and in a lot of different versions, including the ones just stated, so I don't think it is correct to say it does not work on windows 10, there must be more to your dev environment that makes it fail.

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.

6 participants