Fix for wrong shell in Windows

[mkusz]

Fix for #371 which was closed because of workaround. It seems that fixing it is very simple. Take a look into this pull request code.

[mkusz]

Why /bin/bash is default shell when pty is False? Should it be considered as Windows environment instead and check for cmd.exe in executable path? I think that assumptions for tests are wrong. If not please give me an explanation why it's set like this.

[bitprophet]

You may have missed the issue link in #371, but #345 is the main ticket for this - it has some solutions similar to yours, including being a bit more careful and defaulting to cmd.exe if COMSPEC is not set :)

If you want to update this to include the concerns folks bring up in #345 that might be helpful though. Thanks!

[mkusz]

Thanks. I will take a look a try to update this pull request.

[thebjorn]

Any progress on this? It's 4-5 lines in a single file, and this is stopping invoke from running on Windows since 0.13(!) I got bit by this one again when trying to introduce invoke to a new project:

>>> from invoke import run
>>> run('python -V')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "c:\srv\venv\vw1\lib\site-packages\invoke\__init__.py", line 38, in run
    return Context().run(command, **kwargs)
  File "c:\srv\venv\vw1\lib\site-packages\invoke\context.py", line 82, in run
    return self._run(runner, command, **kwargs)
  File "c:\srv\venv\vw1\lib\site-packages\invoke\context.py", line 89, in _run
    return runner.run(command, **kwargs)
  File "c:\srv\venv\vw1\lib\site-packages\invoke\runners.py", line 262, in run
    return self._run_body(command, **kwargs)
  File "c:\srv\venv\vw1\lib\site-packages\invoke\runners.py", line 276, in _run_body
    self.start(command, shell, env)
  File "c:\srv\venv\vw1\lib\site-packages\invoke\runners.py", line 981, in start
    stdin=PIPE,
  File "c:\python27\Lib\subprocess.py", line 390, in __init__
    errread, errwrite)
  File "c:\python27\Lib\subprocess.py", line 640, in _execute_child
    startupinfo)
WindowsError: [Error 2] The system cannot find the file specified
>>>

I'm betting I'm not the only one trying to hunt down why invoke can't find python.. --did it need to be python.exe, does it know what the path is, etc., etc. -- before remembering that I need to import my monkey-patch-hack (https://github.com/datakortet/dk-tasklib/blob/master/dktasklib/wintask.py).

[mkusz]

@thebjorn Thanks for pushing me into fixing it ;)
You can do this before PR will be accepted in main tasks.py file (it should be documented somewhere so anybody will be able to set/switch shell used by Invoke):

from sys import platform
if platform == 'win32':
    ns.configure({'run': {'shell': environ['COMSPEC']}})

@bitprophet I need some help with all checks that are set for this project (to pass them or you will merge it without codecoverage fixes) and If you need some more documentation please let me know what and where should be added.

[thebjorn]

@mkusz thanks for your hard work on this. The configure fix is useful, thx.

[mkusz]

@thebjorn My solution was incomplete. Bellow is fixed one:

from os import environ
from sys import platform
if platform == 'win32':
    ns.configure({'run': {'shell': environ['COMSPEC']}})

[mkusz]

@bitprophet I do not understand why codecov/patch is not green. Please help with it (I have added you to this branch/fork so you can fix it there).

[NickVolynkin]

@mkusz looks like GitHub doesn't recognize the authorship of your commits, probably because they're made with a value of user.email config different than the email that you use for GH. If it is so, you can add that other email here: https://github.com/settings/emails

[thebjorn]

@NickVolynkin @mkusz @bitprophet I might be wrong, but the codecov/patch looks like it has sub-average (94.19%) coverage due to the coverage not being ran on a windows machine (thus not exercising the new code paths). This does not indicate a problem with the patch (if anything it is a bug in the coverage setup -- which should normally combine coverage from runs on all platforms).

I can't see any reason why this PR can't be merged now, as-is.

Please release a new version to PyPI when this PR has been merged..

[mkusz]

@thebjorn Travis run all tests on Windows and they are passing ;)

[thebjorn]

@mkusz Surely not Travis, that's linux only as far as I know (view the configs from https://travis-ci.org/pyinvoke/invoke/builds/277335648, they all say "os": "linux"), maybe you meant Appveyor -- which runs the tests on windows but doesn't seem to report any coverage data (unless I'm missing something..?) See e.g. the if WINDOWS block starting on line 27 https://codecov.io/gh/pyinvoke/invoke/src/42d10a634eee5b04102f89d14ef5114d57603a51/invoke/platform.py#L27 which should have been covered if coverage had been ran on/collected from windows (it top-level code and the rest of the module has been imported).

[mkusz]

@thebjorn Yes you are right about Travis/Linux and AppVeyour/Windows. I misplaced them. As for coverage I do not understand what's happening and do not have enough time for it. If someone is interested to help, I can give assess to my fork for this PR.

[thebjorn]

@mkusz I don't believe there's anything you can do (short of adding enough no-op lines that will be hit on linux and thus bring the coverage up). The "bug" here is in the appveyor setup which doesn't report coverage to codecov.io.

The appveyor issue ought to be fixed, for sure, but shouldn't be a stopper for this PR, which, and I feel like I'm repeating myself here.., fixes a problem that prevents invoke from running on Windows.

Yes, there are workarounds -- mine being perhaps the ugliest, but also perhaps the easiest to work with (https://github.com/datakortet/dk-tasklib/blob/master/dktasklib/wintask.py) -- but none of the workarounds are mentioned in the README or the documentation, so for all intents and purposes invoke is broken on windows for new users since 0.13.

I would really appreciate if @bitprophet would just merge this PR now and release a new version to PyPI.. Pretty please? With sugar on top..?

[garrettXCIV]

So where are we at on getting this fixed? Has it been given up on? If so, why? I'd like to try to help get it fixed. Also, what's with all the extra stuff going on just to get the proper shell in Windows?

[mkusz]

It needs some attention from @bitprophet since, from my point of view it's ready, but for unknown reasons checks are not passing (possible error with code coverage testing on Windows). If you see what can be changed please let me know and I can give you write access to my fix branch.

[garrettXCIV]

Nope, yours looks good other than you should probably update the changelog. I'm gonna delete mine. Hopefully it gets approved soon because it opens up full support for Windows without having to do anything quirky, and at least to me, that sounds like a benefit worth approving quickly.

[bitprophet]

Sincerest apologies for letting this moulder on the vine, been trying not to get bogged down in bugfixes while sprinting on feature related work :( but that pendulum has definitely swung too far in that direction at this point. Will merge this as-is and hope its current state is sufficient for most Windows users; we can make new issues for anything else outstanding and/or the appveyor problem.

[bitprophet]

I lied, did a bunch of cleanup tweaks first. End behavioral result should be the same, however - COMSPEC if it contains cmd.exe (also: why wouldn't we trust a useful shell exe in COMSPEC even if it doesn't end with literal cmd.exe? Don't care enough for now, but...) else literal cmd.exe.

[bitprophet]

Appveyor seems happy at this point https://ci.appveyor.com/project/bitprophet/invoke/build/1.0.563 (as does Travis, unsurprisingly.)

I'll put this up on PyPI as 0.22.0 (which will include another bugfix and drop old Python versions) once 1-2 folks from this thread give me a +1 that latest master still works well for them. Thanks!

[garrettXCIV]

@bitprophet My man! And I agree, there's no reason to even check COMSPEC if we're just going to set all Windows users to cmd.exe in the end, so that leaves us to either:

1. Check COMSPEC and accept whatever it spits out or fall back to cmd.exe if it doesn't exist or is empty, or...
2. Throw out checking COMSPEC entirely and just force all Windows users to use cmd.exe.

I'm definitely leaning towards the former, because generally anyone with a non-default COMSPEC value very purposefully changed it, and we should trust that they knew what they were doing and give them the shell they want.

Tag for inclusion: @mkusz

[garrettXCIV]

PR #496 submitted to use the first solution.

[bitprophet]

Thanks for the feedback on that! I took a slightly different approach to the same end & also added you to the changelog (I knew I'd forgotten at least one person...). See below (or...above...depending on when Github gets all of this.)

[garrettXCIV]

Thanks, I appreciate it! Glad this all got taken care of and that invoke is now Win compatible by default, plus invoke got a nice little clean up from you as well.

[bitprophet]

0.22.0 is on PyPI.

Of minor note, realized afterwards that if any of y'all are (or plan to) using the Fabric 2 alpha for SSH stuff, this change will cause some issues for you until fabric/fabric#1686 is solved.