psi-jack
/
dotbot
mirror of
1
0
Fork 0
Code Issues Releases Wiki Activity

Work around subprocess.call() issue on Windows 

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:
1def7754b7/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 7593d8c134. This is the
desired behavior. See discussion in
https://github.com/anishathalye/dotbot/issues/97 and
https://github.com/anishathalye/dotbot/pull/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:
- https://github.com/anishathalye/dotbot/issues/170
- https://github.com/anishathalye/dotbot/pull/177
- https://github.com/anishathalye/dotbot/issues/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.
This commit is contained in:
Anish Athalye 2020-05-01 11:52:51 -04:00
parent 7ffaa65482
commit f5e019105e
6 changed files with 79 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
dotbot/plugins/clean.py
View File

@ -1,4 +1,6 @@
import os, dotbot
import os
import dotbot
class Clean(dotbot.Plugin):
'''

3
dotbot/plugins/create.py
View File

@ -1,8 +1,5 @@
import os
import glob
import shutil
import dotbot
import subprocess
class Create(dotbot.Plugin):

10
dotbot/plugins/link.py
View File

@ -2,6 +2,7 @@ import os
import glob
import shutil
import dotbot
import dotbot.util
import subprocess
@ -105,14 +106,7 @@ class Link(dotbot.Plugin):
return success
def _test_success(self, command):
with open(os.devnull, 'w') as devnull:
ret = subprocess.call(
command,
shell=True,
stdout=devnull,
stderr=devnull,
executable=os.environ.get('SHELL'),
)
ret = dotbot.util.shell_command(command, cwd=self._context.base_directory())
if ret != 0:
self._log.debug('Test \'%s\' returned false' % command)
return ret == 0

82
dotbot/plugins/shell.py
View File

@ -1,4 +1,8 @@
import os, subprocess, dotbot
import os
import subprocess
import dotbot
import dotbot.util
class Shell(dotbot.Plugin):
'''
@ -19,48 +23,40 @@ class Shell(dotbot.Plugin):
def _process_commands(self, data):
success = True
defaults = self._context.defaults().get('shell', {})
with open(os.devnull, 'w') as devnull:
for item in data:
stdin = stdout = stderr = devnull
quiet = False
if defaults.get('stdin', False) == True:
stdin = None
if defaults.get('stdout', False) == True:
stdout = None
if defaults.get('stderr', False) == True:
stderr = None
if defaults.get('quiet', False) == True:
quiet = True
if isinstance(item, dict):
cmd = item['command']
msg = item.get('description', None)
if 'stdin' in item:
stdin = None if item['stdin'] == True else devnull
if 'stdout' in item:
stdout = None if item['stdout'] == True else devnull
if 'stderr' in item:
stderr = None if item['stderr'] == True else devnull
if 'quiet' in item:
quiet = True if item['quiet'] == True else False
elif isinstance(item, list):
cmd = item[0]
msg = item[1] if len(item) > 1 else None
else:
cmd = item
msg = None
if msg is None:
self._log.lowinfo(cmd)
elif quiet:
self._log.lowinfo('%s' % msg)
else:
self._log.lowinfo('%s [%s]' % (msg, cmd))
executable = os.environ.get('SHELL')
ret = subprocess.call(cmd, shell=True, stdin=stdin, stdout=stdout,
stderr=stderr, cwd=self._context.base_directory(),
executable=executable)
if ret != 0:
success = False
self._log.warning('Command [%s] failed' % cmd)
for item in data:
stdin = defaults.get('stdin', False)
stdout = defaults.get('stdout', False)
stderr = defaults.get('stderr', False)
quiet = defaults.get('quiet', False)
if isinstance(item, dict):
cmd = item['command']
msg = item.get('description', None)
stdin = item.get('stdin', stdin)
stdout = item.get('stdout', stdout)
stderr = item.get('stderr', stderr)
quiet = item.get('quiet', quiet)
elif isinstance(item, list):
cmd = item[0]
msg = item[1] if len(item) > 1 else None
else:
cmd = item
msg = None
if msg is None:
self._log.lowinfo(cmd)
elif quiet:
self._log.lowinfo('%s' % msg)
else:
self._log.lowinfo('%s [%s]' % (msg, cmd))
ret = dotbot.util.shell_command(
cmd,
cwd=self._context.base_directory(),
enable_stdin=stdin,
enable_stdout=stdout,
enable_stderr=stderr
)
if ret != 0:
success = False
self._log.warning('Command [%s] failed' % cmd)
if success:
self._log.info('All commands have been executed')
else:

1
dotbot/util/__init__.py
View File

@ -0,0 +1 @@
from .common import shell_command

34
dotbot/util/common.py Normal file
View File

@ -0,0 +1,34 @@
import os
import subprocess
import platform
def shell_command(command, cwd=None, enable_stdin=False, enable_stdout=False, enable_stderr=False):
with open(os.devnull, 'w') as devnull_w, open(os.devnull, 'r') as devnull_r:
stdin = None if enable_stdin else devnull_r
stdout = None if enable_stdout else devnull_w
stderr = None if enable_stderr else devnull_w
executable = os.environ.get('SHELL')
if platform.system() == 'Windows':
# We avoid setting the executable kwarg on Windows because it does
# not have the desired effect when combined with shell=True. It
# will result in the correct program being run (e.g. bash), but it
# will be invoked with a '/c' argument instead of a '-c' argument,
# which it won't understand.
#
# See https://github.com/anishathalye/dotbot/issues/219 and
# https://bugs.python.org/issue40467.
#
# This means that complex commands that require Bash's parsing
# won't work; a workaround for this is to write the command as
# `bash -c "..."`.
executable = None
return subprocess.call(
command,
shell=True,
executable=executable,
stdin=stdin,
stdout=stdout,
stderr=stderr,
cwd=cwd
)