handle interpreter directives with long lengths

[jdknight]

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]

Codecov Report

Merging #794 into master will decrease coverage by <1%.
The diff coverage is 77%.

@@          Coverage Diff           @@
##           master   #794    +/-   ##
======================================
- Coverage      95%    95%   -<1%     
======================================
  Files          11     11            
  Lines        2307   2320    +13     
======================================
+ Hits         2202   2212    +10     
- Misses        105    108     +3

[obestwalter]

Hi @jdknight, thanks for having a go at this. I like the idea to only try this, when it would otherwise fail anyway, so this might be a good direction to go.

[obestwalter]

Why is it not possible to take the actual value of BINPRM_BUF_SIZE into account here?

[jdknight]

Truthfully, it was a guess to what the actual total interpret length could be 😅. I suspected that BINPRM_BUF_SIZE's value (i.e. 128) couldn't be used directly since I didn't know (or explicitly check) to whether or not the prefix #! and other conditions would be part of the "real limit". But yes, I would agree just throwing it this magic number here isn't really helpful for maintenance reasons.

I re-checked Linux's binfmt_script.c and (I believe) confirmed that a value of 125 should be the value. The define BINPRM_BUF_SIZE (128) with an enforced null-terminating character gives us 127 characters. The prefix #! does contribute to the maximum length, so we have a maximum of 125 characters for our interpreter string (at least, on Linux; 1). Instead of just having a value 127, we could do something like (a note why MAXINTERP's value exist in the comment):

MAXINTERP = 125
if len(interp) > MAXINTERP:

---

On a related note, it may be ideal to also change args = [interp] + args to be args = [interp.lstrip()] + args. This should trim leading whitespace characters from the interpreter before execution (which I assume is a good thing).

[obestwalter]

The problem is that AFAIK different distros patch this length differently, so we would need to be able to know from within a running system. Another (ugly) approach would be to try it and react to that specific problem to try again with the adjusted approach ...

[jdknight]

Agreed that handling interpreter limits for various distributions is a problem. The lazy'er approach would be to just define the common maximum interpret length and allow the user to override the limit (through an environment variable?) to help force this workaround in a more limited-length environment.

The "ugly" approach could work. Execution the command as expected and if it fails with a ENOENT (and is !win32 and is a detected shebang script), try it again with the wrapped call (I'd worry if an executable actually returns a value ENOENT for some operating case; but what are the chances there...).

Another approach would be just to detect is the to be executed script is a shebang script and always wrap it.

---

The code interp = f.readline(1024).rstrip() probably needs more work as well:

• In the event that we don't care about the interpreter's length, we could drop rstrip.
• The size 1024 may be too small. In FreeBSD, MAXINTERP is set to PATH_MAX (2048).
• The directive should probably be split on whitespaces (to handle scenarios where something like #!/usr/bin/env python set).

[gst]

I confirm it solves the problem in my case

[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 then 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 out of the way that is currently going on.

[gst]

finally agree it's safer to keep it disabled by default. 👍

[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 😄

[gaborbernat]

@jdknight there's one more thing missing, please add a changelog entry as in https://github.com/tox-dev/tox/blob/master/changelog/798.feature.rst

Also consider either parametrizing the test case, or just moving into multiple independent tests.

[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]

@jdknight yes; tests which are independent should be I think separate, so we can see failures separately, and gives better options to parallel run it

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

[jdknight]

Thanks everyone.