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

Spawn yarn processes in a cmd subshell on Windows #9628

Merged
merged 2 commits into from
Oct 20, 2020
Merged

Spawn yarn processes in a cmd subshell on Windows #9628

merged 2 commits into from
Oct 20, 2020

Conversation

arilotter
Copy link
Contributor

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

@arilotter arilotter requested review from kumavis and a team as code owners October 18, 2020 03:55
@github-actions
Copy link
Contributor

github-actions bot commented Oct 18, 2020

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.

@arilotter
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

Copy link
Member

@Gudahtt Gudahtt left a 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.

@Gudahtt Gudahtt changed the base branch from master to develop October 19, 2020 23:41
@Gudahtt Gudahtt dismissed a stale review October 19, 2020 23:41

The base branch was changed.

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.
@arilotter
Copy link
Contributor Author

@Gudahtt I've updated my branch to use cross-spawn.

@Gudahtt
Copy link
Member

Gudahtt commented Oct 20, 2020

Thanks @arilotter ! I've pushed one small change to deduplicate the cross-spawn version used by updating a transitive dependency.

Copy link
Member

@Gudahtt Gudahtt left a 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.

@tmashuang
Copy link
Contributor

LGTM, working on cmd and git-bash win10.

@Gudahtt Gudahtt merged commit c3fafe3 into MetaMask:develop Oct 20, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Oct 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants