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

fix: patch default shell on win32 #458

Merged
merged 7 commits into from
Jun 27, 2022
Merged

fix: patch default shell on win32 #458

merged 7 commits into from
Jun 27, 2022

Conversation

thezzisu
Copy link
Contributor

Fixes #398

It's a good idea to open an issue first for discussion.

  • Tests pass
  • Appropriate changes to README are included in PR
  • Types updated

@google-cla
Copy link

google-cla bot commented Jun 27, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@thezzisu
Copy link
Contributor Author

When using zx on a windows machine with wsl installed, this code core.ts#L74 will make base.exe the default shell and execute commands in it, which cause a lot of problems. This PR fixed that problem by skip bash check on win32 platforms.

@antonmedv
Copy link
Collaborator

So what is preferable shell on win? cmd.exe? What happens in running script within WSL?

@thezzisu
Copy link
Contributor Author

On windows, the defaults.shell will be true, make node.js decide which shell to run. Normally it's cmd.exe.
See also https://nodejs.org/api/child_process.html#default-windows-shell.
If running in WSL then process.platform should be linux, and nothing other happens.
And, for a very edge case that running windows' node.exe inside WSL,it's shoud be same as running on windows normally.

@antonmedv
Copy link
Collaborator

It also will be cool to add test for this. Which only runs on win32 and add a GitHub workflow (on Win machine) to test this use case. Can you, please, add such test&workflow?

@thezzisu
Copy link
Contributor Author

Ok, I'll implement the tests later.

@thezzisu
Copy link
Contributor Author

Ok. Win32 specific test implemented and validated both locally & on gh actions.

@antonmedv antonmedv merged commit 1ced6e3 into google:main Jun 27, 2022
tiensonqin pushed a commit to logseq/logseq that referenced this pull request May 4, 2023
`yarn install` currently fails due to two issues:

 a) several build scripts use "cd some_dir && yarn ...", which is not
 valid in cmd.exe.  Replace with "yarn --cwd some_dir .."
 b) zx is somehow defaulting to bash, which finds WSL and runs the
 script in the WSL instance.  Supposedly this was fixed in
 google/zx#458, not sure why it's not working
 even if I upgrade zx.
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.

Runs scripts under WSL/WSL2 if installed and sets incorrect prefix
2 participants