-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Spawn yarn processes in a cmd subshell on Windows #9628
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
I have read the CLA Document and I hereby sign the CLA |
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.
Thanks @arilotter !
Could you replace with with cross-spawn
instead, and add it as a devDependency
? As you suggested in the description, this would do the same thing. I believe it would be easier to maintain as well, as I don't entirely follow what is going on here on the isWindows
branches.
We already have cross-spawn
as a transitive dependency of a number of our other devDependencies
, so adding it would not increase the size of our dependencies.
On Windows, spawn fails if the exact filename of a binary isn't passed. e.g. `spawn('yarn')` fails because the binary is named `yarn.cmd`. Instead, we depend on `cross-spawn` which handles differences in `spawn` across platforms.
@Gudahtt I've updated my branch to use cross-spawn. |
Thanks @arilotter ! I've pushed one small change to deduplicate the |
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.
LGTM! Everything seems to work as-expected when running this on Linux.
I'll ping a few other team members to test this on other platforms.
LGTM, working on |
In Windows, the "command" passed to spawn must be
The exact filename of an executable.
e.g. .\yarn.cmd can't be spawned by
spawn("yarn")
.Instead, we launch cmd and use a subshell.
This allows
yarn start
to succeed on Windows.This fixes the previously closed unresolved issue #9127 on my machine on Windows 10.
This issue was first reported in 2011: nodejs/node-v0.x-archive#2318
There's precedent (100+ references in the above issue) in the nodejs ecosystem to work around this issue by either spawning a cmd subshell on Windows, or using
cross-spawn
(which does the same thing).