From 7b04ad7a243bff87837654495077f56dfbef664c Mon Sep 17 00:00:00 2001
From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com>
Date: Wed, 18 Jul 2018 21:18:06 -0700
Subject: [PATCH] Fix incorrect use of bytes() when invoking the daemon in a
 tty (#6181)

### 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.
---
 src/python/pants/java/nailgun_protocol.py     |  5 ++-
 .../pants_test/java/test_nailgun_protocol.py  | 39 +++++++++++++++++++
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/src/python/pants/java/nailgun_protocol.py b/src/python/pants/java/nailgun_protocol.py
index 26252b6f8e6..2220e82c145 100644
--- a/src/python/pants/java/nailgun_protocol.py
+++ b/src/python/pants/java/nailgun_protocol.py
@@ -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)
@@ -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())
diff --git a/tests/python/pants_test/java/test_nailgun_protocol.py b/tests/python/pants_test/java/test_nailgun_protocol.py
index ef8af7484bb..1994f08205a 100644
--- a/tests/python/pants_test/java/test_nailgun_protocol.py
+++ b/tests/python/pants_test/java/test_nailgun_protocol.py
@@ -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)