-
Notifications
You must be signed in to change notification settings - Fork 340
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve child process spawning #818
Conversation
Note: an alternative would be not to use |
Thanks for the tip @ehmicky. I attempted that yesterday but could make the addons tests work on Windows. Found out the problem today, we were trying to invoke a JS file without shell, which isn't supported. Updated now. Please take another look! |
stdio: settings.stdio || 'inherit', | ||
detached: true, | ||
shell: true, | ||
stdio: 'pipe', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for settings.command
to ask for interactive input from the user?
If so, stdio: 'pipe'
would not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we do process.stdin.pipe(ps.stdin)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented here: #823
- Summary
If the path to binary is not quoted on Windows and contains a space, it will get the path wrong and fail. This change makes sure that the path to app binary is quoted.
Fixes: #817
- Test plan
netlify dev
- Description for the changelog
Quote app binary path before spawning it.
- A picture of a cute animal (not mandatory but encouraged)
馃惉