Invoke: Implementation of #67 broke run() on Windows

[frol]

Recently introduced support of using arbitrary shells (#67) results in that Invoke cannot execute any external command on Windows resulting in the following traceback:

  File "c:\Anaconda\Win64\lib\site-packages\invoke\context.py", line 53, in run
    return runner_class(context=self).run(command, **kwargs)
  File "c:\Anaconda\Win64\lib\site-packages\invoke\runners.py", line 271, in run
    self.start(command, shell, env)
  File "c:\Anaconda\Win64\lib\site-packages\invoke\runners.py", line 857, in start
    stdin=PIPE,
  File "c:\Anaconda\Win64\lib\subprocess.py", line 710, in __init__
    errread, errwrite)
  File "c:\Anaconda\Win64\lib\subprocess.py", line 959, in _execute_child
    startupinfo)
WindowsError: [Error 2] The system cannot find the file specified

This is just due to the implementation of Popen for Windows in Python:

if shell:
    startupinfo.dwFlags |= _winapi.STARTF_USESHOWWINDOW
    startupinfo.wShowWindow = _winapi.SW_HIDE
    comspec = os.environ.get("COMSPEC", "cmd.exe")
    args = '{} /c "{}"'.format (comspec, args)

    # Start the process
    try:
        hp, ht, pid, tid = _winapi.CreateProcess(executable, args,
                                         # no special security
                                         None, None,
                                         int(not close_fds),
                                         creationflags,
                                         env,
                                         cwd,
                                         startupinfo)
...

As you can see, executable is not taken into account and args is constructed with a hard-coded /c flag.

I suggest to avoid executable parameter to Popen and construct a list as a first argument. BTW, hard-coded -c flag in Invoke might be just the same problem for somebody as /c in subprocess.

[frol]

By the way, shell=True without replacement for cmd.exe as executable on Windows (and due to the everything I wrote above we cannot really change cmd.exe on anything useful) causes #271.

Thus, Invoke should not use either shell=True or executable=.

[bitprophet]

If there's a reliable cross-platform set of kwargs we can give Popen, that'd probably be ideal, but IIRC one of the reasons we give shell=True + unmodified command is to avoid having to do potentially buggy lexing of command in order to arrive at a list-type command value. (But see also: #2, #137, #341, all of which argue for at least optionally offering the list version of things.)

However, it might just be easier to have the Popen instantiation branch on platform, with distinct invocations for WINDOWS==True/False.

[wooyek]

For people that get here looking for solution with their FileNotFoundError: [WinError 3] error while running pyinvoke:

Creating a configuration file file pointing out to a windows cmd should help (eg. ~/.invoke.yaml):

run:
    shell: C:\Windows\System32\cmd.exe

[thebjorn]

A platform independent way of setting the shell (in case anyone needs it) would be

import sys, os
from invoke import task, collection

@task(autoprint=True)
def directories(ctx, folder='.'):
    if sys.platform == 'win32':
        res = ctx.run('dir /AD /B {}'.format(folder))
    else:
        res = ctx.run("find {} -type d | sed -e 's|./||g' | grep -v '\.'".format(folder))
    dirs = res.stdout.split()
    return dirs

ns = collection.Collection(directories)
ns.configure({
    'run': {
        'shell': os.environ.get('COMSPEC', os.environ.get('SHELL'))
    }
})

[blakehawkins]

I think I have the same error, but it may be different. Here it is for completeness:

C:\Users\Blake\workspace\z>inv clean
Traceback (most recent call last):
  File "C:\Python27\lib\runpy.py", line 162, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "C:\Python27\lib\runpy.py", line 72, in _run_code
    exec code in run_globals
  File "C:\Python27\Scripts\inv.exe\__main__.py", line 9, in <module>
  File "C:\Python27\lib\site-packages\invoke\program.py", line 270, in run
    self.execute()
  File "C:\Python27\lib\site-packages\invoke\program.py", line 381, in execute
    executor.execute(*self.tasks)
  File "C:\Python27\lib\site-packages\invoke\executor.py", line 113, in execute
    result = call.task(*args, **call.kwargs)
  File "C:\Python27\lib\site-packages\invoke\tasks.py", line 111, in __call__
    result = self.body(*args, **kwargs)
  File "C:\Users\Blake\workspace\bot-wurst\tasks.py", line 6, in clean
    ctx.run('echo "hello world"')
  File "C:\Python27\lib\site-packages\invoke\context.py", line 53, in run
    return runner_class(context=self).run(command, **kwargs)
  File "C:\Python27\lib\site-packages\invoke\runners.py", line 259, in run
    self.start(command, shell, env)
  File "C:\Python27\lib\site-packages\invoke\runners.py", line 966, in start
    stdin=PIPE,
  File "C:\Python27\lib\subprocess.py", line 679, in __init__
    errread, errwrite)
  File "C:\Python27\lib\subprocess.py", line 896, in _execute_child
    startupinfo)
WindowsError: [Error 3] The system cannot find the path specified

[AndreLouisCaron]

I also get this, it's rather annoying that it breaks by default. Also, I find it really confusing that Windows doesn't tell you _which_ path is invalid (I know, not your fault), so I spent some time trying to verify the path of the executable I was trying to run.

@wooyek Thanks for the configuration file tip, I like that much better than setting the shell path explicitly in my tasks.py :-)

[bitprophet]

@pfmoore ran into this as well, curious if he has opinions on how best to solve this (presumably one of the comments above in June/July is applicable - branch to tweak Popen args for Windows, or maybe what @thebjorn suggested, though that seems maybe more fragile to me offhand).

Re Paul's q about appveyor from the other ticket:

Appveyor seems to be at https://ci.appveyor.com/project/bitprophet/invoke/ tho it is building both PRs and the source master w/ no obvious way to filter the former out (all of the PR related options seem to be tweaking _how_ PRs get built). But a definitely on master build is https://ci.appveyor.com/project/bitprophet/invoke/build/1.0.284/job/aemekb5619lfp31g which shows an encoding fail but not the shell one.

[pfmoore]

I'm currently trying to set up a failing test that demonstrates the issue, before I develop a fix. That process is complicated by the fact that I don't know anything about the "spec" tool used to write Invoke's tests, and the use of ANSI codes to colour the output doesn't work on appveyor, resulting in nigh-on unreadable test output. (But maybe that's just sour grapes because my preferred test framework is py.test :-)).

I'd argue that unless the user explicitly specifies a shell, the default should be None and that implies not specifying the argument in Popen. But that does mean that the user's $SHELL will be respected on Unix, rather than the current behaviour of forcing /bin/bash - and I don't know if there is any specific reason behind that.

Anyway, I'll work on a solution, but I _really_ don't want to spend time on it without a test to ensure it doesn't get broken again (and I'm concerned that as you say the appveyor tests are failing - looks like there's already some bitrot crept into the Windows implementation).

[thebjorn]

I just tried upgrading to latest again, and immediately ran into this problem: Windows doesn't work out of the box and there is no way to write library code that fixes it for the end user -- the end user is required to know how to set the correct shell, in an OS independent manner. I see at least one other package has pinned invoke at 0.12.2... Any chance the patch from #407 will be included in a release in the near future?

ps: I tried to write a wrapper for @task that would monkey-patch ctx.run with a version that forced the shell=os.environ['COMSPEC'], but it got really ugly really fast.

[bitprophet]

@thebjorn If you're able to try a PR that builds on top of #407 to account for this comment so the final fix is slightly cleaner (i.e. honor COMSPEC, then fall back to cmd.exe or whatever's appropriate) that'd be awesome and I'd do my best to get it out soon.

[thebjorn]

I've tested #407 on top of master (the merge conflict is trivial - you want both branches).

Using %COMSPEC% is the only correct option. Falling back to any of cmd.exe/command.com/4NT.exe/... is wrong, since (a) the comspec env-var should always be available, and (b) if it isn't, then the correct solution is for the user to fix their system :-) I can't imagine why anyone would want to delete their comspec variable, on purpose, but if they did they could always use one of the solutions from this ticket to get invoke to work.

Please merge #407.

[bitprophet]

Thanks for the details! worksforme. let's roll this into #407 officially.

[thebjorn]

Can we get a new milestone date for #407 (it currently says 0.21, but that's already released without 407..?) Everything is done and it's just waiting for you to merge it...