doc: document windows shell support #16104

yamalight · 2017-10-09T17:29:21Z

Checklist

make -j4 test (UNIX)
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Fixes: #14100

Adds list of supported Windows shells and explains that Git Bash and Cygwin require winpty to work correctly. Fixes: #14100

vsemozhetbyt · 2017-10-09T17:50:03Z

Some additions to consider:

Add an example like this one.
Note that winpty is already installed with git for Windows.

gibfahn · 2017-10-09T17:54:07Z

cc/ @nodejs/platform-windows

Add an example like this one.

Agreed, including the winpty node.exe script.js (and the note that says if you just do node script.js you get winpty by default) would be helpful.

Note that winpty is already installed with git for Windows.

Agreed

Add simple example of winpty usage and clarify that it gets installed along with Git for Windows automatically.

yamalight · 2017-10-09T17:56:35Z

@vsemozhetbyt @gibfahn added! please have a look and let me know if there are any other changes you want.

Fishrock123 · 2017-10-09T18:02:03Z

What about powershell? Is that officially supported or not?

(I would hope so, I need to tell almost everyone to at least use it on Windows rather than Cmd.exe. :/)

yamalight · 2017-10-09T18:06:54Z

@Fishrock123 just tried running example snippet from ticket in powershell - it works OK. Interactive CLI also works fine (doesn't work on e.g. WSL without winpty).
Unless there are other known issues - powershell seems to be working OK.

gibfahn · 2017-10-09T18:09:00Z

BUILDING.md

+note2 - Running Node.js on Windows is officially supported using Cmd. Running Node.js
+  through any other Windows shell (e.g. WSL, Git Bash, Cygwin, etc) is experimental.
+  Please note that running Node.js through Git Bash and Cygwin requires usage of [winpty](https://github.com/rprichard/winpty) (installed with Git for Windows) for Node.js to work
+  correctly (e.g. `winpty node.exe script.js`).


I'd add something like:

If you call node without the .exe extension, winpty is used automatically.

@gibfahn that'd only be within git for windows, right?

Yes.

BTW Git for Windows === Git Bash, so maybe just use the latter for clarity.

@gibfahn alright, got it. let me add that..

vsemozhetbyt · 2017-10-09T18:15:01Z

Has anybody tried to build and run tests in powershell?

Added info on Git Bash running winpty automatically when running node without .exe extension. Changed "Git for Windows" to "Git Bash" for clarity.

gibfahn · 2017-10-09T18:30:35Z

Has anybody tried to build and run tests in powershell?

I think @refack was rewriting vcbuild.bat in powershell, so he might have tried it.

doesn't work on e.g. WSL without winpty

Interesting, possibly something we should look at.

yamalight · 2017-10-09T18:33:42Z

@gibfahn should I open a ticket about it (interactive CLI not working on WSL)?

gibfahn · 2017-10-09T18:37:51Z

@gibfahn should I open a ticket about it (interactive CLI not working on WSL)?

I'd wait and see how this PR pans out. At the moment we don't officially support WSL.

refack · 2017-10-09T18:38:45Z

BUILDING.md

@@ -59,6 +59,13 @@ note1 - The gcc4.8-libs package needs to be installed, because node
  by Joyent. SmartOS images >= 16.4 are not supported because
  GCC 4.8 runtime libraries are not available in their pkgsrc repository

+note2 - Running Node.js on Windows is officially supported using Cmd. Running Node.js


I think I understand the place this is coming from, but it's a way to complicated scenario to try to boil down to a Note...
For instance there's also PowerShell which is supported, WSL is a complete different animal (and is it Ubuntu or OpenSuse). Also it's completely Ok to run node without a shell (directly from a Windows app or via ❖Win + r).
Then there's the whole Git for Windows === Git Bash === MSYS ~== MinGW naming kerfuffle...
And as for terminal emulators, AFAIK winpty is only needed when using mintty. I don't know what happens when running MSYS Bash in Console2 or ComEmu...

tl;dr IMHO this should go in a different section with a simpler explanation of why winpty is needed and and which scenarios, and omit the "Officially supported" sentence since that's hard to define, and the sentence above is probably not the most correct approximation (for instance we CI on headless MSYS Bash but via python that traps the stdio file handlers).

refack · 2017-10-09T18:41:39Z

Hello @yamalight, first of all welcome and thank you for the contribution 🥇

You've hit a complicated scenario, and I'm not sure the note in it's current form is the best way to approach this...
I'm sorry I don't have an outright better suggestion, but I promise to think about it.

yamalight · 2017-10-09T18:44:32Z

@refack well, that issue was marked "good first contribution" 😂 but yeah, that doesn't seem to be quite as simple as it was outlined within the issue.
just let me know what changes to make once you figure out what you want to see here - I can wait 😄

refack · 2017-10-09T18:45:04Z

Has anybody tried to build and run tests in powershell?

I think @refack was rewriting vcbuild.bat in powershell, so he might have tried it.

PowerShell builds and works fine. It's only the MSYS / MinGW terminal emulators that are tricky.
As I commented in my review, it's AFAIK it's not the MSYS Bash but rather mintty. I need to check how the stdios work when running "git bash" in ConEmu.

gibfahn · 2017-10-09T18:45:54Z

You've hit a complicated scenario, and I'm not sure the note in it's current form is the best way to approach this...

I think this is better than what we had before (nothing), so I'm +1 on landing it and iterating. Agreed that it's very complex.

refack · 2017-10-09T18:53:09Z

@refack well, that issue was marked "good first contribution" 😂 but yeah, that doesn't seem to be quite as simple as it was outlined within the issue.
just let me know what changes to make once you figure out what you want to see here - I can wait 😄

I'd go with a minimal affirmative statement:

*Note*: Running node in windows terminal emulators like `mintty` requires the usage of
  [winpty](https://github.com/rprichard/winpty) for node's tty channels to work correctly
  (e.g. `winpty node.exe script.js`). In "Git bash" if you call the node shell wrapper
  (`node` without the `.exe` extension), `winpty` is used automatically.

P.S. We now tend to use *Note*: for note delimiters.
P.P.S. My paragraph is not 80 char aligned.

Simplify the explanation of node usage through non-standard windows shells.

yamalight · 2017-10-09T19:22:55Z

@refack alright, changed that. Please check!

refack · 2017-10-09T19:30:53Z

BUILDING.md

@@ -59,6 +59,12 @@ note1 - The gcc4.8-libs package needs to be installed, because node
  by Joyent. SmartOS images >= 16.4 are not supported because
  GCC 4.8 runtime libraries are not available in their pkgsrc repository

+*Note*: Running node in windows terminal emulators like `mintty` requires the


Just to clarify that this is only in regards to Windows, I'd change it to:

*Note*: On Windows, when running node in terminal emulators like...

refack · 2017-10-09T19:32:28Z

Thanks for following up!

If you haven't already, it's recommended you take a look at CONTRIBUTING.md guide (especialy the part about "discuss and update"). Customarily PRs are kept open for at least 48 hour so that anyone interested gets a chance to comment or review.
Also documentation changes, while easy to do are more difficult to validate. With code we can write tests and the CI can validate the change. With documentation the only way to validate is with human eyes, so don't be dismayed if you get a lot of feedback.
Anyway thank you for the contribution, and good luck 🥇

P.S. If you have any question you can also feel free to contact me directly, here or on IRC

Simple clarification that note about mintty and winpty is only relevant when running Node.js on Windows

yamalight · 2017-10-09T19:35:38Z

@refack got it. thanks for the guidance :)

seishun · 2017-10-10T06:32:36Z

BUILDING.md

+  requires the usage of [winpty](https://github.com/rprichard/winpty) for
+  Node's tty channels to work correctly (e.g. `winpty node.exe script.js`).
+  In "Git bash" if you call the node shell wrapper (`node` without the `.exe`
+  extension), `winpty` is used automatically.


I'd avoid adding this note because this is an implementation detail of "Git Bash" that could change anytime.

But it's also a really odd thing that isn't documented anywhere else, and the difference between node and node.exe is very unobvious.

I think it's worth documenting that if you're on Git Bash, you should just use node without the .exe to get the required behaviour, so far I've only seen it documented in Github comments (including by you 😁 ) , e.g. #5620 (comment) and #3006 (comment).

this is an implementation detail of "Git Bash" that could change anytime.

🤷‍♂️ , if it changes we can change the docs, but I wouldn't call it an implementation detail, this is an alias Git Bash added to make life easier for Node users.

I'm ambivalent. I had both arguments in my head ("GfW quirk" vs. "Undocumented helper")...

so, what's the final decision here? any changes needed from my side?

I personally think that it'd be better to mention it now and change later if needed, for all it's worth.

refack · 2017-10-11T13:13:43Z

@seishun I'm considering your comment as non-blocking, ok?

Linter: https://ci.nodejs.org/job/node-test-linter/12504/ ✔️

explain that Git Bash and Cygwin require winpty to work correctly. Added info on Git Bash running winpty automatically when running node without .exe extension. PR-URL: #16104 Fixes: #14100 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

refack · 2017-10-12T14:47:41Z

Landed in 641ba5e

@yamalight congratulations on getting promoted from:

to

explain that Git Bash and Cygwin require winpty to work correctly. Added info on Git Bash running winpty automatically when running node without .exe extension. PR-URL: nodejs/node#16104 Fixes: nodejs/node#14100 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

explain that Git Bash and Cygwin require winpty to work correctly. Added info on Git Bash running winpty automatically when running node without .exe extension. PR-URL: #16104 Fixes: #14100 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

doc: document windows shell support

dcde949

Adds list of supported Windows shells and explains that Git Bash and Cygwin require winpty to work correctly. Fixes: #14100

nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Oct 9, 2017

vsemozhetbyt approved these changes Oct 9, 2017

View reviewed changes

vsemozhetbyt added the windows Issues and PRs related to the Windows platform. label Oct 9, 2017

gibfahn approved these changes Oct 9, 2017

View reviewed changes

doc: clarify winpty in windows support section

de56d13

Add simple example of winpty usage and clarify that it gets installed along with Git for Windows automatically.

gibfahn reviewed Oct 9, 2017

View reviewed changes

refack mentioned this pull request Oct 9, 2017

Ctrl + C Doesn't Kill Gracefully | Node v6.11.3 - Git Bash #16103

Closed

doc: clarify winpty usage from Git Bash

013732e

Added info on Git Bash running winpty automatically when running node without .exe extension. Changed "Git for Windows" to "Git Bash" for clarity.

refack suggested changes Oct 9, 2017

View reviewed changes

doc: simplify windows shell support explanation

d524f37

Simplify the explanation of node usage through non-standard windows shells.

refack approved these changes Oct 9, 2017

View reviewed changes

refack self-assigned this Oct 9, 2017

doc: clarify that mintty note is related to running on windows

e9ca96d

Simple clarification that note about mintty and winpty is only relevant when running Node.js on Windows

gireeshpunathil approved these changes Oct 10, 2017

View reviewed changes

seishun reviewed Oct 10, 2017

View reviewed changes

refack closed this Oct 12, 2017

MylesBorins added the land-on-v6.x label Nov 16, 2017

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

mintty mentioned this pull request Aug 13, 2018

error ELIFECYCLE errno 3221225786 on ctrl-c in node/express/npm w/ mintty 2.8.5 mintty/mintty#788

Closed

refack removed their assignment Oct 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: document windows shell support #16104

doc: document windows shell support #16104

yamalight commented Oct 9, 2017

vsemozhetbyt commented Oct 9, 2017

gibfahn commented Oct 9, 2017

yamalight commented Oct 9, 2017

Fishrock123 commented Oct 9, 2017

yamalight commented Oct 9, 2017

gibfahn Oct 9, 2017

yamalight Oct 9, 2017

gibfahn Oct 9, 2017

yamalight Oct 9, 2017

vsemozhetbyt commented Oct 9, 2017

gibfahn commented Oct 9, 2017

yamalight commented Oct 9, 2017

gibfahn commented Oct 9, 2017

refack Oct 9, 2017

refack commented Oct 9, 2017

yamalight commented Oct 9, 2017

refack commented Oct 9, 2017

gibfahn commented Oct 9, 2017

refack commented Oct 9, 2017

yamalight commented Oct 9, 2017

refack Oct 9, 2017

refack commented Oct 9, 2017

yamalight commented Oct 9, 2017

seishun Oct 10, 2017

gibfahn Oct 10, 2017

refack Oct 10, 2017

yamalight Oct 11, 2017

refack commented Oct 11, 2017 •

edited

Loading

refack commented Oct 12, 2017

doc: document windows shell support #16104

doc: document windows shell support #16104

Conversation

yamalight commented Oct 9, 2017

Checklist

Affected core subsystem(s)

vsemozhetbyt commented Oct 9, 2017

gibfahn commented Oct 9, 2017

yamalight commented Oct 9, 2017

Fishrock123 commented Oct 9, 2017

yamalight commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt commented Oct 9, 2017

gibfahn commented Oct 9, 2017

yamalight commented Oct 9, 2017

gibfahn commented Oct 9, 2017

Choose a reason for hiding this comment

refack commented Oct 9, 2017

yamalight commented Oct 9, 2017

refack commented Oct 9, 2017

gibfahn commented Oct 9, 2017

refack commented Oct 9, 2017

yamalight commented Oct 9, 2017

Choose a reason for hiding this comment

refack commented Oct 9, 2017

yamalight commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Oct 11, 2017 • edited Loading

refack commented Oct 12, 2017

refack commented Oct 11, 2017 •

edited

Loading