Skip to content

Commit

Permalink
Fix incorrect use of bytes() when invoking the daemon in a tty (#6181)
Browse files Browse the repository at this point in the history
### Problem

`pkill -f "pantsd \[" pantsd-runner && ./pants --enable-pantsd help` fails on master when run within a tty because we incorrectly try to pass an `int` directly to a `bytes`. This was introduced during #6159.

### Solution

- Wrap the `bytes()` argument in a list in `NailgunProtocol.isatty_to_env()`.
- Add unit testing which mocks out the tty querying to ensure the environment we return is valid.

### Result

No more crashing when trying to create a pantsd in a tty.

#### Thoughts

This may be another argument for something like #6157. There may also be room for an integration test which opens up a "real" pty to catch issues like these in the future, but it's not clear to me how to do that right now.
  • Loading branch information
cosmicexplorer authored Jul 19, 2018
1 parent 4aec12c commit 7b04ad7
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 2 deletions.
5 changes: 3 additions & 2 deletions src/python/pants/java/nailgun_protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@

import os
import struct
from builtins import bytes, object, zip
from builtins import object, str, zip

import six
from future.utils import binary_type


STDIO_DESCRIPTORS = (0, 1, 2)
Expand Down Expand Up @@ -245,7 +246,7 @@ def isatty_to_env(cls, stdin, stdout, stderr):
def gen_env_vars():
for fd_id, fd in zip(STDIO_DESCRIPTORS, (stdin, stdout, stderr)):
is_atty = fd.isatty()
yield (cls.TTY_ENV_TMPL.format(fd_id), bytes(int(is_atty)))
yield (cls.TTY_ENV_TMPL.format(fd_id), binary_type(str(int(is_atty))))
if is_atty:
yield (cls.TTY_PATH_ENV.format(fd_id), os.ttyname(fd.fileno()) or '')
return dict(gen_env_vars())
Expand Down
39 changes: 39 additions & 0 deletions tests/python/pants_test/java/test_nailgun_protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,45 @@ def test_isatty_from_env_mixed(self):
(False, True, False)
)

def _make_mock_stream(self, isatty, fileno):
mock_stream = mock.Mock()
mock_stream.isatty.return_value = isatty
mock_stream.fileno.return_value = fileno
return mock_stream

_fake_ttyname = '/this/is/not/a/real/tty'

@mock.patch('os.ttyname', autospec=True, spec_set=True)
def test_isatty_to_env_with_mock_tty(self, mock_ttyname):
mock_ttyname.return_value = self._fake_ttyname
mock_stdin = self._make_mock_stream(True, 0)
mock_stdout = self._make_mock_stream(True, 1)
mock_stderr = self._make_mock_stream(True, 2)

self.assertEquals(
NailgunProtocol.isatty_to_env(mock_stdin, mock_stdout, mock_stderr),
{
'NAILGUN_TTY_0': b'1',
'NAILGUN_TTY_1': b'1',
'NAILGUN_TTY_2': b'1',
'NAILGUN_TTY_PATH_0': self._fake_ttyname,
'NAILGUN_TTY_PATH_1': self._fake_ttyname,
'NAILGUN_TTY_PATH_2': self._fake_ttyname,
})

def test_isatty_to_env_without_tty(self):
mock_stdin = self._make_mock_stream(False, 0)
mock_stdout = self._make_mock_stream(False, 1)
mock_stderr = self._make_mock_stream(False, 2)

self.assertEquals(
NailgunProtocol.isatty_to_env(mock_stdin, mock_stdout, mock_stderr),
{
'NAILGUN_TTY_0': b'0',
'NAILGUN_TTY_1': b'0',
'NAILGUN_TTY_2': b'0',
})

def test_construct_chunk(self):
with self.assertRaises(TypeError):
NailgunProtocol.construct_chunk(ChunkType.STDOUT, 1111)
Expand Down

0 comments on commit 7b04ad7

Please sign in to comment.