Shell commands fail on Windows due to popen shell=False in Action._popen

Issue #257 on hold
Stefano created an issue

Hello,

I am using tox 2.0.2 and Python 2.7 on Windows 7.

As part of my tox tests, I need to run gulp and other npm-related commands.

To do so, I first run npm install and then e.g. run gulp that has been installed in the node_modules directory.

The relevant part of my tox.ini looks like

[testenv]
whitelist_externals = npm
commands =
                     npm install
                     {toxinidir}/node_modules/.bin/gulp

When I run tox, I get the following error when gulp is run:

py27-win32 runtests: commands[0] | npm install
py27-win32 runtests: commands[1] | F:\my_project\my_project/node_modules/.bin/gulp
ERROR: invocation failed (errno 8), args: ['F:\\my_project\\my_project/node_modules/.bin/gulp'], cwd: F:\my_project\my_project
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\tox.exe\__main__.py", line 9, in <module>
  File "C:\Python27\lib\site-packages\tox\session.py", line 39, in main
    retcode = Session(config).runcommand()
  File "C:\Python27\lib\site-packages\tox\session.py", line 367, in runcommand
    return self.subcommand_test()
  File "C:\Python27\lib\site-packages\tox\session.py", line 540, in subcommand_test
    self.runtestenv(venv)
  File "C:\Python27\lib\site-packages\tox\session.py", line 548, in runtestenv
    venv.test(redirect=redirect)
  File "C:\Python27\lib\site-packages\tox\venv.py", line 360, in test
    ignore_ret=ignore_ret)
  File "C:\Python27\lib\site-packages\tox\venv.py", line 384, in _pcall
    return action.popen(args, cwd=cwd, env=env, redirect=redirect, ignore_ret=ignore_ret)
  File "C:\Python27\lib\site-packages\tox\session.py", line 130, in popen
    stdout=stdout, stderr=STDOUT)
  File "C:\Python27\lib\site-packages\tox\session.py", line 218, in _popen
    stdout=stdout, stderr=stderr, env=env)
  File "C:\Python27\lib\subprocess.py", line 710, in __init__
    errread, errwrite)
  File "C:\Python27\lib\subprocess.py", line 958, in _execute_child
    startupinfo)
WindowsError: [Error 193] %1 is not a valid Win32 application

The same configuration works OK on Linux.

I think that the problem lies in Action._popen in the session.py module, where self.session.popen is called (line 216).

popen is run with the shell keyword argument set to False, and causes the command to fall over on Windows.

Since I had already noticed that problem (on Windows only) in other places where I use subprocess, I tried to change the value to True by doing

    return self.session.popen(args, shell=(sys.platform == 'win32'), cwd=str(cwd),
                                             universal_newlines=True,
                                             stdout=stdout, stderr=stderr, env=env)

and that solved the issue (while keeping the default value to False on non-Windows platforms).

Comments (19)

  1. Stefano reporter

    I have created pull request #165 that attempts to address the issue.

    This is my first PR, so please excuse me if I have done something wrong with the various procedures etc.

    Thanks.

  2. Stefano reporter

    @The-Compiler,

    npm install installs a number of node modules, some of which can be executed, e.g. gulp is a sort of make for JavaScript stuff. The way that works is that npm creates a .bin directory inside its node_modules one with scripts that execute node on the actual JavaScript file. npm creates a batch script called <NAME>.cmd and a shell simply script called <NAME>.

    So, for Windows, .bin/gulp will actually call .bin/gulp.cmd. The contents of the script are presently:

    @IF EXIST "%~dp0\node.exe" (
      "%~dp0\node.exe"  "%~dp0\..\gulp\bin\gulp.js" %*
    ) ELSE (
      @SETLOCAL
      @SET PATHEXT=%PATHEXT:;.JS;=;%
      node  "%~dp0\..\gulp\bin\gulp.js" %*
    )
    

    Honestly, I don't know why subprocess fails on Windows if shell=False though. But that's what I've been observing quite consistently.

  3. Holger Krekel repo owner

    i also don't understand why shell is needed for gulp. At this point i don't think we want to change the invocation semantics without very clear reasons and probably even then only by a per-testenv switch or so.

  4. Florian Bruhin

    I guess cmd.exe prefers gulp.cmd to the shellscript if it exists, and subprocess (i.e. the Windows APIs) don't. These things are always... funny on Windows.

    Have you tried actually invoking gulp.cmd instead of gulp in your tox.ini?

  5. Stefano reporter

    Hi,

    I've just tried to run the cmd version in my tox.ini and this makes Windows happy.

    Oh well, I guess that we can discard my pull request and I will modify my tox.ini so that it'll run the .cmd version if on Windows.

    Would it be worth adding something to the documentation about this?

    Thank you very much for your super-fast feedback!

  6. Stefano reporter

    Not an issue, see discussion for details.

    Maybe some documentation can be added to avoid confusion to Windows users.

  7. Holger Krekel repo owner

    Wait a second -- tox does have machinery to find the correct .cmd/.bat etc. files if a command is on PATH (so e.g. "npm" could actually be "npm.exe" or "npm.cmd" and your tox.ini would work). However, the {toxinidir}/node_modules/.bin/gulp bit is not using this logic because it's an absolute path. I wonder if we should enhance tox to care for the proper extension on windows also for absolute paths.

  8. Stefano reporter

    Ahh! I did not know that!

    In fact, I was thinking about why npm install was working on the way home!

    In that case, it might be great if tox could be enhanced to do that too.

    Still, I feel that the specific issue that I opened is still "invalid". Should we open a new one with a clearer title and goal? Or would it be OK to reopen this one instead?

    Thanks.

  9. Stefano reporter

    A quick update.

    This is an excerpt from my tox.ini that allows me to work around the problem and keep the GNU/Linux and Windows environments separated (karma is basically a JS test runner):

    [jscommands:linux]
    gulp = {toxinidir}/node_modules/.bin/gulp
    karma = {toxinidir}/node_modules/.bin/karma
    
    [jscommands:win32]
    gulp = {[javascript:linux]gulp}.cmd
    karma = {[javascript:linux]karma}.cmd
    
    [testenv]
    whitelist_externals = npm
    commands =
             npm install
             py27-linux: {[jscommands:linux]gulp}
             py27-linux: {[jscommands:linux]karma} start --single-run
             py27-win32: {[jscommands:win32]gulp}
             py27-win32: {[jscommands:win32]karma} start --single-run
    

    Of course, if tox could take care of the naming, it would be great.

  10. Stefano reporter

    @The-Compiler, I've verified that on Windows, if shell=False only commands with their extensions will work. I guess that shell=True runs the command through cmd.exe which, in turn expands the extension.

  11. Stefano reporter

    @hpk42 I might have some time to work on this, would you mind to give me pointers to the parts of tox's code I should look at and where I should put the tests?

    Thanks.

  12. Holger Krekel repo owner

    i guess you need to look at the venv.py and tests/test_venv.py in particular. First writing a test that fails is the hard bit, maybe.

  13. Log in to comment