feat(cli): allow running scripts with custom shell (#4248)

[yunyu]

Summary

This fixes issue #4248, based off of what NPM does.

Test plan

I'm not actually sure how to write a test suite for this (as testing any alternate shell would involve additional installations), but it passes all of the current tests. Any feedback/advice would be appreciated!

Here's how I am manually confirming that the change works (see the echo $SHELL output):

The Windows terminal emulator (which I am using) doesn't support emojis, which causes the weird characters before the Done message. However, MinTTY should.

[buildsize]

This change will increase the build size from 10.39 MB to 10.4 MB, an increase of 1.43 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	900.21 KB	900.29 KB	73 bytes (0%)
yarn-[version].js	3.92 MB	3.92 MB	615 bytes (0%)
yarn-legacy-[version].js	4.06 MB	4.06 MB	613 bytes (0%)
yarn-v[version].tar.gz	905.33 KB	905.42 KB	94 bytes (0%)
yarn_[version]all.deb	669.13 KB	669.19 KB	66 bytes (0%)

[rally25rs]

I didn't know NPM could do this. It's, IMO, a dangerous idea to change shells. People's package install scripts are tested to work in sh, and things like globing patterns differ between shells, especially bash vs zsh, so install scripts might start failing if someone is using a different shell.

Take a look at #4951 for an example of a glob pattern that changes between shells.

---

I haven't looked into the NPM behavior, but I would say that if we only used this setting for yarn run commands, and not for package install scripts, then that would be OK. However the way this is implemented now, it would be used for everything.

[yunyu]

This is really only needed on Windows, where lots of scripts assume *sh (see the original issue report) and use features that aren't available in cmd.exe (example). As such, I don't expect people to use this on *nix (as you said, there's hardly a good reason to).

Edit: I checked the NPM docs and it says that it's only used for npm run: https://docs.npmjs.com/misc/config#script-shell

I'll modify the PR.

[yunyu]

@rally25rs I made the requested change and manually tested the behavior again. I'm wondering if __tests__\run.js would the appropriate place to add a test, now that I've modified the signature for execCommand? I don't think it's feasible to test the actual behavior of a custom shell, but testing the arguments for execCommand is doable.

[rally25rs]

I'm wondering if __tests__\run.js would the appropriate place to add a test

__tests__\commands\run.js contains all the current tests for the yarn run command, so that would be the correct place to add a test.

Thanks!

[yunyu]

After some wrangling, I managed to add the test. Just for reference, NPM only tests the passed arguments as well: https://github.com/npm/npm/blob/b2b03733f8cf8983892076d15b830b5913891adb/test/tap/run-script.js#L319

[rally25rs]

Nicely done. Thanks for the contribution! 👍

[aikar]

@rally25rs a main use for this is to override: https://github.com/yarnpkg/yarn/pull/5133/files#diff-939cc0d319704d4f0d98baf9167ba850R191

My team mandates windows users use bash, and even with cross-env, we still had issues with some scripts on windows.

this setting fixed the issues with npm config set script-shell bash (not even hard coding path, just use whatever is in ENV). but did not fix it for yarn due to this missing support.

glad to see this finally land, though too late now that my entire team went to linux heh.

[BYK]

We may need to revert this change since it essentially reverts #4031 which fixed an issue on Windows. We should have added tests for this but haven't back then so now it came back to bite us.

@yunyu what do you think?

[aikar]

@BYK why do you think it reverts it? Users have to opt into this, and never binaries still work under git bash just fine?

[BYK]

@aikar - this PR changes the execution command for all cases, regardless of them using this new option or not: https://github.com/yarnpkg/yarn/pull/5133/files#diff-939cc0d319704d4f0d98baf9167ba850R218

[aikar]

@BYK but if a shell isn't set, it would fall into the same windows specific check. The desire here is to explicitly opt out of that code. If shell isn't set, it looks like it behaves the same?

[yunyu]

@BYK I can't seem to reproduce that issue on my computer (Windows 10 x64, Node.js 8.9.3, script-shell not set):

PowerShell:

cmd:

I think there's a likely chance that fixCmdWinSlashes fixed it.

There actually is an issue replaced to sh backslash handling if script-shell is set, however (I didn't encounter this as I didn't know this was a feature):

This can be fixed by assuming a sh-compatible shell if script-shell is set (same as in execute-lifecycle-script): yunyu@691ee08

[aikar]

When I say opt out, I meant to opt out of the cmd.exe stuff. Shouldn't it be the user's problem if they set it to a shell that does not handle windows commands correctly?

[BYK]

@aikar - Although the code looked identical, using the shell:true option did something differently and fixed the issue. I don't think you and I are talking about the same thing. I'm not talking about anything on the user-side. There literally was a bug that was fixed with using shell:true and this patch reverts that fix. If you look at the previous code there, before the shell:true fix, it looks very much like the current code, sans the custom shell feature this PR introduced.

@yunyu

I think there's a likely chance that fixCmdWinSlashes fixed it.

I think this is the reason why it is not an issue right now since this is what changed in-between that may affect running commands.

There actually is an issue replaced to sh backslash handling if script-shell is set, however (I didn't encounter this as I didn't know this was a feature):

I don't think to assume a sh-compatible shell is a good idea on Windows. PowerShell is a common shell replacement for instance. All that said, this is why I think this feature is not a great idea. It adds complexity without covering important edge cases. It is a band-aid solution to this problem that will likely create more issues.

The docs for child_process.exect says the following about the shell option:

shell Shell to execute the command with. Default: '/bin/sh' on UNIX, process.env.ComSpec on Windows. See Shell Requirements and Default Windows Shell.

And the "Default Windows Shell" section states the following:

Although Microsoft specifies %COMSPEC% must contain the path to 'cmd.exe' in the root environment, child processes are not always subject to the same requirement. Thus, in child_process functions where a shell can be spawned, 'cmd.exe' is used as a fallback if process.env.ComSpec is unavailable.

So essentially, this whole feature can be replaced by simply defining $COMSPEC before running yarn scripts and I think that is a better way to fix this since I'm guessing this feature is not popular amongst macOS or Linux users. What do you think?

[aikar]

@BYK Well if somethings still off, seems rather trivial to simply special case 'shell: true' to the previous behavior.

From my understanding, script-shell should be overriding the shell used, and not use comspec if script-shell is set.

So this PR satisfies that situation correctly.

if script-shell isn't set, then shell shouldn't be changed?

https://github.com/yarnpkg/yarn/pull/5133/files#diff-fd065890a349a1c304804a9a9dafe02dR84

[yunyu]

@BYK the main problem with setting the shell value in opts or setting $COMSPEC is that Node assumes shells on Windows are CMD-compatible, and that shells on *nix are sh compatible: https://github.com/nodejs/node/blob/master/lib/child_process.js#L477

This results in Git Bash/WSL not working (as they don't recognize the /d /s /c flags), which are the primary use cases for this feature.

[aikar]

My team members on Windows used 'npm set script-shell bash', and work specifically out of Git Bash/WSL, so the intent was to stay within the bash ecosystem and not ever jump back through cmd.exe.

Based on @yunyu 's response, looks like everythings working correctly and this PR is fine?

[yunyu]

The only problem right now is described in #5133 (comment), but that doesn't break anything that was previously working without a shell set. For compatibility with NPM and targeting the most common use case,, I personally think that assuming a sh-compatible shell is the best approach, but I'm open to discussion. Should I file a new issue for that?