New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for wrong shell in Windows #407

Merged
merged 9 commits into from Nov 29, 2017

Conversation

Projects
None yet
5 participants
@mkusz
Contributor

mkusz commented Nov 14, 2016

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

This comment has been minimized.

Contributor

mkusz commented Nov 14, 2016

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

This comment has been minimized.

Member

bitprophet commented Nov 14, 2016

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

This comment has been minimized.

Contributor

mkusz commented Nov 14, 2016

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

@thebjorn

This comment has been minimized.

thebjorn commented Sep 17, 2017

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 added some commits Sep 18, 2017

@mkusz

This comment has been minimized.

Contributor

mkusz commented Sep 19, 2017

@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

This comment has been minimized.

thebjorn commented Sep 19, 2017

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

@mkusz

This comment has been minimized.

Contributor

mkusz commented Sep 19, 2017

@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

This comment has been minimized.

Contributor

mkusz commented Sep 19, 2017

@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

This comment has been minimized.

NickVolynkin commented Sep 26, 2017

@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

This comment has been minimized.

thebjorn commented Sep 27, 2017

@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

This comment has been minimized.

Contributor

mkusz commented Sep 27, 2017

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

@thebjorn

This comment has been minimized.

thebjorn commented Sep 27, 2017

@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

This comment has been minimized.

Contributor

mkusz commented Sep 27, 2017

@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

This comment has been minimized.

thebjorn commented Sep 27, 2017

@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..?

thebjorn referenced this pull request in datakortet/dk-tasklib Oct 1, 2017

Integrating pytest per latest docs.
Updating pytest version (otherwise the setup.cfg tools:pytest section is not read on windows).

@mkusz mkusz closed this Nov 14, 2017

@gucciferXCIV

This comment has been minimized.

gucciferXCIV commented Nov 27, 2017

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?

This was referenced Nov 28, 2017

@mkusz

This comment has been minimized.

Contributor

mkusz commented Nov 28, 2017

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.

@mkusz mkusz reopened this Nov 28, 2017

@gucciferXCIV

This comment has been minimized.

gucciferXCIV commented Nov 28, 2017

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

This comment has been minimized.

Member

bitprophet commented Nov 29, 2017

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

This comment has been minimized.

Member

bitprophet commented Nov 29, 2017

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 bitprophet merged commit 2fb4245 into pyinvoke:master Nov 29, 2017

2 of 4 checks passed

codecov/patch 33.33% of diff hit (target 94.28%)
Details
codecov/project 94.2% (-0.08%) compared to 51a7159
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

bitprophet added a commit that referenced this pull request Nov 29, 2017

bitprophet added a commit that referenced this pull request Nov 29, 2017

bitprophet added a commit that referenced this pull request Nov 29, 2017

@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 29, 2017

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!

@gucciferXCIV

This comment has been minimized.

gucciferXCIV commented Nov 29, 2017

@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

@gucciferXCIV

This comment has been minimized.

gucciferXCIV commented Nov 29, 2017

PR #496 submitted to use the first solution.

bitprophet added a commit that referenced this pull request Nov 29, 2017

@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 29, 2017

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.)

@gucciferXCIV

This comment has been minimized.

gucciferXCIV commented Nov 29, 2017

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 added a commit that referenced this pull request Nov 29, 2017

@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 29, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment