Git bash cannot execute shell command on login shell

[shivapoudel]

Make sure the login shell is turned on to replicate this issue. To check use shopt login_shell and if it is turned off then use bash --login and re-check login shell status.

Not test the below install shell config accordingly.

- create:
    - ~/.wp-cli

- shell:
    - "bash -c 'curl -sL https://raw.githubusercontent.com/wp-cli/wp-cli/master/utils/wp-completion.bash -o ~/.wp-cli/wp-completion.bash'"

With a non-login shell, it's a success but with a login shell, it fails. Here is the logged error:

?[94mPath exists C:\Users\shiva/.wp-cli?[0m
?[92mAll paths have been set up?[0m
?[94mbash -c 'curl -sL https://raw.githubusercontent.com/wp-cli/wp-cli/master/utils/wp-completion.bash -o ~/.wp-cli/wp-completion.bash'?[0m
?[95mCommand [bash -c 'curl -sL https://raw.githubusercontent.com/wp-cli/wp-cli/master/utils/wp-completion.bash -o ~/.wp-cli/wp-completion.bash'] failed?[0m
?[91mSome commands were not successfully executed?[0m
?[91m
==> Some tasks were not executed successfully?[0m

[anishathalye]

I don't currently have access to a Windows machine, so it's going to be hard for me to replicate and debug.

What happens if you use the extended form for the command, and enable stdout/stderr -- do you see a more detailed error message?

- shell:
    - command: ...
      stdout: true
      stderr: true

[shivapoudel]

@anishathalye I receive this error details:

?[94mPath exists C:\Users\shiva/.wp-cli?[0m
?[92mAll paths have been set up?[0m
?[94mbash -c 'curl -sL https://raw.githubusercontent.com/wp-cli/wp-cli/master/utils/wp-completion.bash -o ~/.wp-cli/wp-completion.bash'?[0m
/c: /c: Is a directory
?[95mCommand [bash -c 'curl -sL https://raw.githubusercontent.com/wp-cli/wp-cli/master/utils/wp-completion.bash -o ~/.wp-cli/wp-completion.bash'] failed?[0m
?[91mSome commands were not successfully executed?[0m
?[91m
==> Some tasks were not executed successfully?[0m

[anishathalye]

Huh, not exactly sure why this is happening. "/c: /c: Is a directory" is a strange error message to see. Maybe it has something to do with how the "~" is evaluated?

Does this happen if you run the command manually in a login shell in bash, with the whole bash -c ...?

[shivapoudel]

Yes, it does work in the login shell with curl -L without any silent:

If we run without the ~/.wp-cli with curl -L without any silence. This is received:

[shivapoudel]

@anishathalye Can you look in this comment? The same discussion it talked there :)

If you need any help to debug through this then let me know. I will help you with the tests in the Windows environment.

[anishathalye]

Ok I downloaded a copy of Windows, installed Git Bash and Python, and I was able to reproduce the issue. The issue you linked is indeed related, thank you for that. I think merging #177 will fix the immediate problem, but I don't know if it'll cause collateral damage. I want to understand why the current code doesn't work.

The executable= argument was introduced in #100; the discussion there explains why we want it in certain situations. Do these situations come up when running on Windows? I am not sure. If the answer is no, then we can safely merge #177.

When run in Git Bash, Dotbot calls subprocess.call() with the executable= kwarg set to C:\Program Files\Git\usr\bin\bash.exe on my computer.

It seems like subprocess.call() with shell=True doesn't play nicely with the executable=... argument. Here's a minimal self-contained failing example:

>>> bash = 'C:\\Program Files\\Git\\usr\\bin\\bash.exe'
>>> os.path.exists(bash)
True
>>> subprocess.call('echo hi', shell=True)
hi
0
>>> subprocess.call('echo hi', shell=True, executable=bash)
/c: /c: Is a directory
126

Looking at the subprocess.py implementation and tracing it with a sys.addaudithook(), it looks like it gets to the _winapi.CreateProcess and breaks somewhere after that. I haven't had time yet to go deeper and trace execution into CreateProcess, but I wanted to write down what I've found so far. Seems like it might be a bug in Python, perhaps to do with string escaping or path parsing?

The subprocess documentation doesn't really describe the semantics of the executable= arg when combined with shell=True on Windows either. It says this for POSIX:

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

But it doesn't have a similar description for Windows.

[anishathalye]

The relevant difference between the login shell and the non-login shell is that the login shell exports $SHELL, while the non-login shell does not (even though it has it defined). Dotbot calls subprocess.call() with executable=os.environ.get('SHELL'), so when it's not exported, it ends up doing executable=None. When executable=None, CreateProcess is called with None as the application name and the following as the command line:

 C:\\Windows\\system32\\cmd.exe /c "bash -c "echo hi""

So it runs cmd.exe with the /c ... argument, which tells it to run bash, and interpret the stuff following the -c. This is already a workaround, having to write the bash -c ... in the Dotbot install.conf.yaml. We should have just been able to write the command directly.

When executable='C:\\Program Files\\Git\\usr\\bin\\bash.exe', CreateProcess is called with C:\\Program Files\\Git\\usr\\bin\\bash.exe as the application to run, but it still has the following as the command line:

 C:\\Windows\\system32\\cmd.exe /c "bash -c "echo hi""

So we run bash, but with its argv set to ['cmd.exe, '/c', ...]. If you try running bash /c ..., you'll see the error we were originally getting:

/c: /c: Is a directory

What we really want is for subprocess.call() to understand that our shell is a POSIX-compatible one, and to respect the executable= argument but also set the argv correctly. So if we run subprocess.call('ls | wc', shell=True, executable='C:\\Program Files\\Git\\usr\\bin\\bash.exe'), we would like it to do the following:

C:\\Program Files\\Git\\usr\\bin\\bash.exe -c "ls | wc"

But instead, it does

C:\\Windows\\system32\\cmd.exe /c "ls | wc"

Bash doesn't care about its argv[0] here, and we could "fix" that by setting the COMSPEC environment variable. What is tripping it up is the /c instead of the -c. Looks like this is an issue with subprocess.py itself, and I think it's unclear how it should be fixed (how would subprocess.py know if the shell that we're calling wants /c or -c?)

On our side, we can work around it by doing something like #177 (not setting executable= on Windows), and then requiring users to do the bash -c ... thing if they want POSIX shell behavior to parse/execute their command. I don't think this will break anything, because this behavior was broken on Windows anyways.

I'm not even sure how to reliably detect Windows; e.g. does platform.system() work for Cygwin (and does this broken behavior happen on Cygwin as well?)

Update: installed Cygwin and Cygwin's Python; seems like everything works just fine there, so no workaround needed with that. platform.system() returns "CYGWIN_NT-...", so we won't confuse it in a check like platform.system() == 'Windows'

[shivapoudel]

@anishathalye In windows, I tried with login shell and displays platform correctly

[anishathalye]

Moving discussion to #220

[anishathalye]

Thank you for reporting this and helping with debugging. Fixed in #220.