Repository: tox-dev/tox
Pull request #794 (2018-04-24 02:43:34)

handle interpreter directives with long lengths
handle interpreter directives with long lengths

When preparing virtual environments in a file container which has large length, the system might not be able to invoke shebang scripts which define interpreters beyond system limits (i.e. Linux has a limit of 128; `BINPRM_BUF_SIZE` \[1\]). This wrapper can be used to check if the executable is a script containing a shebang line \[2\]. If so, read the first line for the interpreter and check if length exceeds "common" limit. If the interpreter exceeds the limit, adjust the argument list to directly call the interpreter.

\[1\]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/include/uapi/linux/binfmts.h?h=linux-4.16.y#n19
\[2\]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/fs/binfmt_script.c?h=linux-4.16.y#n24

----

Recently had an issue attempting to run tox in a Jenkins Multibranch Pipeline where I received the following error:

```
$ tox
GLOB sdist-make: /var/lib/jenkins/workspace/[...]-py_initial-devwork-RC5P2SZZC5BWPRXRA37BMA5XOH53QUYKJ5Z2625THIQZFTY2FL6Q/setup.py
py27 create: /var/lib/jenkins/workspace/[...]-py_initial-devwork-RC5P2SZZC5BWPRXRA37BMA5XOH53QUYKJ5Z2625THIQZFTY2FL6Q/.tox/py27
py27 installdeps: -r/var/lib/jenkins/workspace/[...]-py_initial-devwork-RC5P2SZZC5BWPRXRA37BMA5XOH53QUYKJ5Z2625THIQZFTY2FL6Q/requirements.txt
ERROR: invocation failed (errno 2), args: ['/var/lib/jenkins/workspace/[...]-py_initial-devwork-RC5P2SZZC5BWPRXRA37BMA5XOH53QUYKJ5Z2625THIQZFTY2FL6Q/.tox/py27/bin/pip', 'install', '-r/var/lib/jenkins/workspace/[...]-py_initial-devwork-RC5P2SZZC5BWPRXRA37BMA5XOH53QUYKJ5Z2625THIQZFTY2FL6Q/requirements.txt'], cwd: /var/lib/jenkins/workspace/[...]-py_initial-devwork-RC5P2SZZC5BWPRXRA37BMA5XOH53QUYKJ5Z2625THIQZFTY2FL6Q
Traceback (most recent call last):
  File "/bin/tox", line 11, in <module>
    sys.exit(run_main())
  File "/usr/lib/python2.7/site-packages/tox/session.py", line 40, in run_main
    main(args)
  File "/usr/lib/python2.7/site-packages/tox/session.py", line 46, in main
    retcode = Session(config).runcommand()
  File "/usr/lib/python2.7/site-packages/tox/session.py", line 415, in runcommand
    return self.subcommand_test()
  File "/usr/lib/python2.7/site-packages/tox/session.py", line 599, in subcommand_test
    if self.setupenv(venv):
  File "/usr/lib/python2.7/site-packages/tox/session.py", line 491, in setupenv
    status = venv.update(action=action)
  File "/usr/lib/python2.7/site-packages/tox/venv.py", line 171, in update
    self.hook.tox_testenv_install_deps(action=action, venv=self)
  File "/usr/lib/python2.7/site-packages/pluggy/__init__.py", line 617, in __call__
    return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
  File "/usr/lib/python2.7/site-packages/pluggy/__init__.py", line 222, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "/usr/lib/python2.7/site-packages/pluggy/__init__.py", line 216, in <lambda>
    firstresult=hook.spec_opts.get('firstresult'),
  File "/usr/lib/python2.7/site-packages/pluggy/callers.py", line 201, in _multicall
    return outcome.get_result()
  File "/usr/lib/python2.7/site-packages/pluggy/callers.py", line 77, in get_result
    _reraise(*ex)  # noqa
  File "/usr/lib/python2.7/site-packages/pluggy/callers.py", line 180, in _multicall
    res = hook_impl.function(*args)
  File "/usr/lib/python2.7/site-packages/tox/venv.py", line 452, in tox_testenv_install_deps
    venv._install(deps, action=action)
  File "/usr/lib/python2.7/site-packages/tox/venv.py", line 331, in _install
    action=action)
  File "/usr/lib/python2.7/site-packages/tox/venv.py", line 303, in run_install_command
    action=action, redirect=self.session.report.verbosity < 2)
  File "/usr/lib/python2.7/site-packages/tox/venv.py", line 409, in _pcall
    redirect=redirect, ignore_ret=ignore_ret)
  File "/usr/lib/python2.7/site-packages/tox/session.py", line 150, in popen
    stdout=stdout, stderr=subprocess.STDOUT)
  File "/usr/lib/python2.7/site-packages/tox/session.py", line 243, in _popen
    stdout=stdout, stderr=stderr, env=env)
  File "/usr/lib64/python2.7/subprocess.py", line 711, in __init__
    errread, errwrite)
  File "/usr/lib64/python2.7/subprocess.py", line 1327, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory
```

My tox version:

```
$ tox --version
3.0.0 
```

After some investigation, it appears to be an issue with the path length generated by Jenkins and the interpreter strings of scripts tox needs to invoke. This issue has been also discussed in #66 and #649 (and related pypa/pip#1773, pypa/virtualenv#596). This proposed change is an attempt to help provide a workaround for tox user's in long-path environments. After the above change, I was able to invoke a tox-support project under a long-path environment:

```
$ tox
GLOB sdist-make: /var/lib/jenkins/workspace/[...]-py_initial-devwork-RC5P2SZZC5BWPRXRA37BMA5XOH53QUYKJ5Z2625THIQZFTY2FL6Q/setup.py
py27 inst-nodeps: /var/lib/jenkins/workspace/[...]-py_initial-devwork-RC5P2SZZC5BWPRXRA37BMA5XOH53QUYKJ5Z2625THIQZFTY2FL6Q/.tox/dist/[...]-0.1.0.dev0.zip
py27 installed: enum34==1.1.6,[...]==0.1.0.dev0
py27 runtests: PYTHONHASHSEED='931962048'
...
```

(note: I don't know much about tox's internals, so this could be completely wrong; if workarounds like this are not desired, I have no problem in dropping this request)


codecov[bot]: # [Codecov](https://codecov.io/gh/tox-dev/tox/pull/794?src=pr&el=h1) Report
> Merging [#794](https://codecov.io/gh/tox-dev/tox/pull/794?src=pr&el=desc) into [master](https://codecov.io/gh/tox-dev/tox/commit/e603f78e8438eadfb5e4067868096e377d3556ca?src=pr&el=desc) will **decrease** coverage by `1.39%`.
> The diff coverage is `87.5%`.

[![Impacted file tree graph](https://codecov.io/gh/tox-dev/tox/pull/794/graphs/tree.svg?src=pr&width=650&token=DYodAwDCZ5&height=150)](https://codecov.io/gh/tox-dev/tox/pull/794?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##           master     #794     +/-   ##
=========================================
- Coverage   94.85%   93.46%   -1.4%     
=========================================
  Files          11       11             
  Lines        2408     2416      +8     
=========================================
- Hits         2284     2258     -26     
- Misses        124      158     +34
```


| [Impacted Files](https://codecov.io/gh/tox-dev/tox/pull/794?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [tox/venv.py](https://codecov.io/gh/tox-dev/tox/pull/794/diff?src=pr&el=tree#diff-dG94L3ZlbnYucHk=) | `95.73% <87.5%> (-0.84%)` | :arrow_down: |
| [tox/interpreters.py](https://codecov.io/gh/tox-dev/tox/pull/794/diff?src=pr&el=tree#diff-dG94L2ludGVycHJldGVycy5weQ==) | `72.22% <0%> (-20.64%)` | :arrow_down: |
| [tox/\_quickstart.py](https://codecov.io/gh/tox-dev/tox/pull/794/diff?src=pr&el=tree#diff-dG94L19xdWlja3N0YXJ0LnB5) | `81.37% <0%> (-1.38%)` | :arrow_down: |
| [tox/session.py](https://codecov.io/gh/tox-dev/tox/pull/794/diff?src=pr&el=tree#diff-dG94L3Nlc3Npb24ucHk=) | `94.42% <0%> (-0.35%)` | :arrow_down: |
| [tox/config.py](https://codecov.io/gh/tox-dev/tox/pull/794/diff?src=pr&el=tree#diff-dG94L2NvbmZpZy5weQ==) | `97.64% <0%> (-0.13%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/tox-dev/tox/pull/794?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/tox-dev/tox/pull/794?src=pr&el=footer). Last update [e603f78...ed7f0e3](https://codecov.io/gh/tox-dev/tox/pull/794?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

gst: I guess adding some test(s) on this and it would be merge ready ?
there could be an eventual opt-in or opt-out flag as it's more a workaround IMO but making the job, and if it can be quickly integrated that would be awesome :)
obestwalter: Hi @gst thanks for testing on your setup. Which system did you test on?

Regarding merging this: the minimum we need are some tests and a file in the changelog folder explaining the change. 

I am still not sure if we should go for the retry option instead, but we would have to be able to make sure that we only retry if we can be sure that the problem was due to the shebang length.

We could merge the and test it out in an rc hoping we catch any problems. It is very hard to say if a hack like that might cause other problems down the road, so it would be good if more eyeballs would be involved.
gst: Ubuntu 16.04(.3). Python 2.7. Under a Jenkins instance (but not required).

I might have a look at adding some test for this (even if I don't know tox internals at all for now).

I'd make this ~feature~ workaround enabled by default but with an opt-out flag on my side.
jdknight: The following is another pass based off comments made in the active pull request. Changes made and reason for the changes include:

- This workaround is disabled* by default and can be enabled on supported systems with the `TOX_LIMITED_SHEBANG` environment variable set (e.x. `export TOX_LIMITED_SHEBANG=1`).
- No longer rely on an assume interpreter maximum length on a system. If an interpreter target shell script is detected, this workaround will always grab the interpreter to use and invoke the script explicitly (i.e. no longer relying on platform's shebang implementation).
- Support the optional single argument supported by shebang interpreter definitions.
- Increase the size of the parsed line to prevent issues on platforms which support an even larger interpreter length (e.x. FreeBSD supports up to 2048).
- Adding unit testing for the prepend interpreter method (suggested by @obestwalter; which could probably be expanded on).

\* I've (reluctantly) adjusted this workaround to be disabled by default (counter to what @gst has suggested). The reason is primarily due to the existing unit tests which have a certain expectations on the `_pcall` implementation. Changes could be made to these unit tests to support the workaround enabled by default, but I didn't want to attempt something like that without a second opinion. I also noticed that with the workaround enabled, the use of prepended interpreters can trigger warning reports about non-whitelisted externals. I've only seen a subset of this implementation, so there may be other implications that I haven't considered yet.

obestwalter: I actually think that it is a very good idea to have this workaround disabled by default. We can definitely run with this, after we have the [spring cleaning](https://github.com/tox-dev/tox/pull/788) out of the way that is currently going on.
gst: finally agree it's safer to keep it disabled by default. :+1:  
onlymellb: This PR works for my case.
gaborbernat: @jdknight please take time when you have to resolve conflicts and I'll take the time to review and merge :smile: 
jdknight: @gaborbernat, sure; I'll append another commit with a changelog entry (~8-10PM EST).

As for your second comment, are you looking for something like this instead:
```
@pytest.mark.skipif("sys.platform == 'win32'")
def test_tox_testenv_interpret_shebang_empty_instance(tmpdir):
    testfile = tmpdir.join('test_shebang.py')
    base_args = [str(testfile), 'arg1', 'arg2', 'arg3']

    testfile.write('')
    args = prepend_shebang_interpreter(base_args)
    assert args == base_args

@pytest.mark.skipif("sys.platform == 'win32'")
def test_tox_testenv_interpret_shebang_empty_interp(tmpdir):
    testfile = tmpdir.join('test_shebang.py')
    base_args = [str(testfile), 'arg1', 'arg2', 'arg3']

    testfile.write('#!')
    args = prepend_shebang_interpreter(base_args)
    assert args == base_args

@pytest.mark.skipif("sys.platform == 'win32'")
def test_tox_testenv_interpret_shebang_empty_interpws(tmpdir):
    testfile = tmpdir.join('test_shebang.py')
    base_args = [str(testfile), 'arg1', 'arg2', 'arg3']

    testfile.write('#!    \n')
    args = prepend_shebang_interpreter(base_args)
    assert args == base_args
...
```
gaborbernat: Given this thing is optional can you amend the description how to enable it? Maybe even in the documentation beside the changelog 😀
jdknight: @gaborbernat, sure. I did not know what the expected/desired content in a changelog entry was. I just assumed, based on what I saw in existing file entries, is to just include a simple line description trailed by ownership. After now examining the content I see in `CHANGELOG.rst`, I'll tweak the changelog entry to be more descriptive.
gaborbernat: @jdknight really sorry to be a hassle about this, but just one final thing add a note about this to our config sections end https://github.com/tox-dev/tox/blob/master/doc/config.rst
jdknight: Before I try to submit something for review, any objections for me to add level-two title (a child of `tox configuration specification`) at the bottom (before `Other Rules and notes`) called "advanced settings"? Under this section I can make a subsection describing this workaround.
gaborbernat: Yeah I think that's a good approach.

Patch:
diff --git a/changelog/794.feature.rst b/changelog/794.feature.rst
new file mode 100644
index 00000000..085334b9
--- /dev/null
+++ b/changelog/794.feature.rst
@@ -0,0 +1,5 @@
+Add support to explicitly invoke interpreter directives for environments with
+long path lengths. In the event that ``tox`` cannot invoke scripts with a
+system-limited shebang (e.x. a Linux host running a Jenkins Pipeline), a user
+can set the environment variable ``TOX_LIMITED_SHEBANG`` to workaround the
+system's limitation (e.x. ``export TOX_LIMITED_SHEBANG=1``) - by @jdknight
diff --git a/doc/config.rst b/doc/config.rst
index cf1d9ec4..5a40420d 100644
--- a/doc/config.rst
+++ b/doc/config.rst
@@ -699,6 +699,26 @@ With the previous configuration, it will install:
 - ``flake8`` package for ``py36`` environment.
 - ``flake8`` and ``coverage`` packages for ``coverage`` environment.
 
+Advanced settings
+-----------------
+
+Handle interpreter directives with long lengths
++++++++++++++++++++++++++++++++++++++++++++++++
+
+For systems supporting executable text files (scripts with a shebang), the
+system will attempt to parse the interpreter directive to determine the program
+to execute on the target text file. When ``tox`` prepares a virtual environment
+in a file container which has a large length (e.x. using Jenkins Pipelines), the
+system might not be able to invoke shebang scripts which define interpreters
+beyond system limits (e.x. Linux as a limit of 128; ``BINPRM_BUF_SIZE``). To
+workaround an environment which suffers from an interpreter directive limit, a
+user can bypass the system's interpreter parser by defining the
+``TOX_LIMITED_SHEBANG`` environment variable before invoking ``tox``::
+
+    export TOX_LIMITED_SHEBANG=1
+
+When the workaround is enabled, all tox-invoked text file executables will have
+their interpreter directive parsed by and explicitly executed by ``tox``.
 
 Other Rules and notes
 =====================
diff --git a/tox/venv.py b/tox/venv.py
index 1d52e521..1f05c3a1 100755
--- a/tox/venv.py
+++ b/tox/venv.py
@@ -400,6 +400,8 @@ class VirtualEnv(object):
 
         cwd.ensure(dir=1)
         args[0] = self.getcommandpath(args[0], venv, cwd)
+        if sys.platform != 'win32' and 'TOX_LIMITED_SHEBANG' in os.environ:
+            args = prepend_shebang_interpreter(args)
         env = self._getenv(testcommand=testcommand)
         bindir = str(self.envconfig.envbindir)
         env['PATH'] = p = os.pathsep.join([bindir, os.environ["PATH"]])
@@ -415,6 +417,30 @@ def getdigest(path):
     return path.computehash()
 
 
+def prepend_shebang_interpreter(args):
+    # prepend interpreter directive (if any) to argument list
+    #
+    # When preparing virtual environments in a file container which has large
+    # length, the system might not be able to invoke shebang scripts which
+    # define interpreters beyond system limits (e.x. Linux as a limit of 128;
+    # BINPRM_BUF_SIZE). This method can be used to check if the executable is
+    # a script containing a shebang line. If so, extract the interpreter (and
+    # possible optional argument) and prepend the values to the provided
+    # argument list. tox will only attempt to read an interpreter directive of
+    # a maximum size of 2048 bytes to limit excessive reading and support UNIX
+    # systems which may support a longer interpret length.
+    try:
+        with open(args[0], 'rb') as f:
+            if f.read(1) == b'#' and f.read(1) == b'!':
+                MAXINTERP = 2048
+                interp = f.readline(MAXINTERP).rstrip()
+                interp_args = interp.split(None, 1)[:2]
+                return interp_args + args
+    except IOError:
+        pass
+    return args
+
+
 @tox.hookimpl
 def tox_testenv_create(venv, action):
     config_interpreter = venv.getsupportedinterpreter()


Test patch:
diff --git a/tests/test_venv.py b/tests/test_venv.py
index 5ff601b0..912f03f6 100644
--- a/tests/test_venv.py
+++ b/tests/test_venv.py
@@ -7,7 +7,8 @@ import pytest
 import tox
 from tox.interpreters import NoInterpreterInfo
 from tox.venv import (
-    CreationConfig, VirtualEnv, getdigest, tox_testenv_create, tox_testenv_install_deps)
+    CreationConfig, VirtualEnv, getdigest, prepend_shebang_interpreter,
+    tox_testenv_create, tox_testenv_install_deps)
 
 
 def test_getdigest(tmpdir):
@@ -761,3 +762,108 @@ def test_tox_testenv_pre_post(newmocksession):
     assert log == []
     mocksession.runtestenv(venv)
     assert log == ['started', 'finished']
+
+
+@pytest.mark.skipif("sys.platform == 'win32'")
+def test_tox_testenv_interpret_shebang_empty_instance(tmpdir):
+    testfile = tmpdir.join('check_shebang_empty_instance.py')
+    base_args = [str(testfile), 'arg1', 'arg2', 'arg3']
+
+    # empty instance
+    testfile.write('')
+    args = prepend_shebang_interpreter(base_args)
+    assert args == base_args
+
+
+@pytest.mark.skipif("sys.platform == 'win32'")
+def test_tox_testenv_interpret_shebang_empty_interpreter(tmpdir):
+    testfile = tmpdir.join('check_shebang_empty_interpreter.py')
+    base_args = [str(testfile), 'arg1', 'arg2', 'arg3']
+
+    # empty interpreter
+    testfile.write('#!')
+    args = prepend_shebang_interpreter(base_args)
+    assert args == base_args
+
+
+@pytest.mark.skipif("sys.platform == 'win32'")
+def test_tox_testenv_interpret_shebang_empty_interpreter_ws(tmpdir):
+    testfile = tmpdir.join('check_shebang_empty_interpreter_ws.py')
+    base_args = [str(testfile), 'arg1', 'arg2', 'arg3']
+
+    # empty interpreter (whitespaces)
+    testfile.write('#!    \n')
+    args = prepend_shebang_interpreter(base_args)
+    assert args == base_args
+
+
+@pytest.mark.skipif("sys.platform == 'win32'")
+def test_tox_testenv_interpret_shebang_interpreter_simple(tmpdir):
+    testfile = tmpdir.join('check_shebang_interpreter_simple.py')
+    base_args = [str(testfile), 'arg1', 'arg2', 'arg3']
+
+    # interpreter (simple)
+    testfile.write('#!interpreter')
+    args = prepend_shebang_interpreter(base_args)
+    assert args == [b'interpreter'] + base_args
+
+
+@pytest.mark.skipif("sys.platform == 'win32'")
+def test_tox_testenv_interpret_shebang_interpreter_ws(tmpdir):
+    testfile = tmpdir.join('check_shebang_interpreter_ws.py')
+    base_args = [str(testfile), 'arg1', 'arg2', 'arg3']
+
+    # interpreter (whitespaces)
+    testfile.write('#!  interpreter  \n\n')
+    args = prepend_shebang_interpreter(base_args)
+    assert args == [b'interpreter'] + base_args
+
+
+@pytest.mark.skipif("sys.platform == 'win32'")
+def test_tox_testenv_interpret_shebang_interpreter_arg(tmpdir):
+    testfile = tmpdir.join('check_shebang_interpreter_arg.py')
+    base_args = [str(testfile), 'arg1', 'arg2', 'arg3']
+
+    # interpreter with argument
+    testfile.write('#!interpreter argx\n')
+    args = prepend_shebang_interpreter(base_args)
+    assert args == [b'interpreter', b'argx'] + base_args
+
+
+@pytest.mark.skipif("sys.platform == 'win32'")
+def test_tox_testenv_interpret_shebang_interpreter_args(tmpdir):
+    testfile = tmpdir.join('check_shebang_interpreter_args.py')
+    base_args = [str(testfile), 'arg1', 'arg2', 'arg3']
+
+    # interpreter with argument (ensure single argument)
+    testfile.write('#!interpreter argx argx-part2\n')
+    args = prepend_shebang_interpreter(base_args)
+    assert args == [b'interpreter', b'argx argx-part2'] + base_args
+
+
+@pytest.mark.skipif("sys.platform == 'win32'")
+def test_tox_testenv_interpret_shebang_real(tmpdir):
+    testfile = tmpdir.join('check_shebang_real.py')
+    base_args = [str(testfile), 'arg1', 'arg2', 'arg3']
+
+    # interpreter (real example)
+    testfile.write('#!/usr/bin/env python\n')
+    args = prepend_shebang_interpreter(base_args)
+    assert args == [b'/usr/bin/env', b'python'] + base_args
+
+
+@pytest.mark.skipif("sys.platform == 'win32'")
+def test_tox_testenv_interpret_shebang_long_example(tmpdir):
+    testfile = tmpdir.join('check_shebang_long_example.py')
+    base_args = [str(testfile), 'arg1', 'arg2', 'arg3']
+
+    # interpreter (long example)
+    testfile.write(
+        '#!this-is-an-example-of-a-very-long-interpret-directive-what-should-'
+        'be-directly-invoked-when-tox-needs-to-invoked-the-provided-script-'
+        'name-in-the-argument-list')
+    args = prepend_shebang_interpreter(base_args)
+    assert args == [
+        b'this-is-an-example-of-a-very-long-interpret-directive-what-should-be-'
+        b'directly-invoked-when-tox-needs-to-invoked-the-provided-script-name-'
+        b'in-the-argument-list'] + base_args