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

Falls back to default shell to install plugin on win32 #1565

Merged
merged 1 commit into from
Feb 27, 2017
Merged

Falls back to default shell to install plugin on win32 #1565

merged 1 commit into from
Feb 27, 2017

Conversation

kwonoj
Copy link
Contributor

@kwonoj kwonoj commented Feb 21, 2017

related to #1480.

This PR try to fix plugin installation fails on windows machine, if user specifies custom shell other than cmd.exe. The crux of this issue is due to behavior of child_process::exec (https://nodejs.org/dist/latest-v6.x/docs/api/child_process.html#child_process_child_process_exec_command_options_callback), node.js requires any shell specified in windows should compliant to command line parsing with cmd.exe to understand /s /c (or /d /s /c on later version) and does not provide any way to override it.

There isn't seems stable manner of drop-in replacement to child process supports this , so in this PR simply let exec falls back to default shell instead of custom shell when install plugins.

@rauchg
Copy link
Member

rauchg commented Feb 25, 2017

Are you sure you meant to test for not windows? I don't understand the logic 100%

@kwonoj
Copy link
Contributor Author

kwonoj commented Feb 25, 2017

Changed logic is

  1. if platform is windows
  2. fall back to shell to cmd.exe, by ignoring any custom configuration in hyper.js

if platform is non-windows, it'll still honor config as-is. Main issue in here is node.js force inject /s /c command for windows, assumes all shell specified compliant to cmd.exe, in opposite for non-windows it always inject -c, assumes shell complaint to those switch.

In windows, it is possible to have non-cmd compliant shell. For non windows, afaik it is not possible to have those shell since most known shell even including fish which isn't POSIX-compliant honors -c switch, doesn't need to fall back to default system shell.

@kwonoj
Copy link
Contributor Author

kwonoj commented Feb 25, 2017

This is reason in code it test against if platform isn't win32, specify shell as config file.

@dcinzona
Copy link

For the sake of conversation, I was having this same issue on my install on Windows 10 with Bash -> zsh set up as my default shell. Including the changes that were part of this PR solved the issue for me.

@rauchg
Copy link
Member

rauchg commented Feb 27, 2017

Thanks @dcinzona

@rauchg rauchg merged commit 3df8274 into vercel:master Feb 27, 2017
@kwonoj kwonoj deleted the fix-plugin-win32 branch February 27, 2017 23:10
sugatmahanti added a commit to sugatmahanti/hyper that referenced this pull request Feb 28, 2017
Falls back to default shell to install plugin on win32 (vercel#1565)
@jmcbee
Copy link

jmcbee commented Feb 28, 2017

This worked for me. Thank you!

tylerong pushed a commit to tylerong/hyper that referenced this pull request Jul 3, 2017
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.

None yet

4 participants