Work around subprocess.call() issue on Windows

[anishathalye]

On POSIX-like systems, calling subprocess.call() with both shell=True and executable='...' has the following behavior:

If shell=True, on POSIX the executable argument specifies a replacement shell for the default /bin/sh.

(via https://docs.python.org/3/library/subprocess.html?highlight=subprocess#popen-constructor)

This seems to have a similar behavior on Windows, but this is problematic when a POSIX shell is substituted for cmd.exe. This is because when shell=True, the shell is invoked with a '/c' argument, which is the correct argument for cmd.exe but not for Bash, which expects a '-c' argument instead. See here: https://github.com/python/cpython/blob/1def7754b7a41fe57efafaf5eff24cfa15353444/Lib/subprocess.py#L1407

This is problematic when combined with Dotbot's behavior, where the executable argument is set based on $SHELL. For example, when running in Git Bash, the $SHELL environment variable is set to Bash, so any commands run by Dotbot will fail (because it'll invoke Bash with a '/c' argument).

This behavior of setting the executable argument based on $SHELL was introduced in 7593d8c. This is the desired behavior. See discussion in #97 and #100.

Unfortunately, this doesn't work quite right on Windows. This patch works around the issue by avoiding setting the executable argument when the platform is Windows, which is tested using platform.system() == 'Windows'. This means that shell commands executed by Dotbot on this platform will always be run using cmd.exe. Invocations of single programs or simple commands will probably work just fine in cmd.exe. If Bash-like behavior is desired, the user will have to write their command as bash -c '...'.

This shouldn't have any implications for backwards-compatibility, because setting the executable argument on Windows didn't do the right thing anyways. Previous workarounds that users had should continue to work with the new code.

When using Python from CYGWIN, platform.system() returns something like 'CYGWIN_NT-...', so it won't be detected with the check, but this is the correct behavior, because CYGWIN Python's subprocess.call() has the POSIX-like behavior.

This patch also refactors the code to factor out the subprocess.call(), which was being called in both link.py and shell.py, so the workaround can be applied in a single place.

See the following issues/pull requests for a discussion of this bug:

• Shell - Some commands were not successfully executed #170
• don't set executable arg to subprocess.call on Windows #177
• Git bash cannot execute shell command on login shell #219

An issue has also been raised in Python's issue tracker:

• https://bugs.python.org/issue40467

Thanks to @shivapoudel for originally reporting the issue, @SuJiKiNen for debugging it and submitting a pull request, and @mohkale for suggesting factoring out the code so that other plugins could use it.

[anishathalye]

I think omitting the executable arg on Windows is a decent workaround to the problem. I'm not really sure if there's a better fix for this (maybe the Python developers will have some ideas in https://bugs.python.org/issue40467).

Some commands will probably work without an extra bash -c "....", and if Bash behavior is desired, that doesn't seem too bad to include in the install.conf.yaml.

[shivapoudel]

@anishathalye I got some time today and go through this PR for test and it patches the bug.

[anishathalye]

Thanks!

I will wait a bit longer in case anyone else has feedback, and then I'll merge.