New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect use of bytes() when invoking the daemon in a tty #6181

Merged
merged 6 commits into from Jul 19, 2018

Conversation

Projects
None yet
4 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Jul 18, 2018

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.

cosmicexplorer added some commits Jul 18, 2018

@cosmicexplorer cosmicexplorer requested review from stuhood and kwlzn Jul 18, 2018

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Jul 18, 2018

Thanks Danny for catching this and patching it!

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

I don't think #6157 would have caught this... but having a tty-centric integration test at some point would be awesome. Maybe @kwlzn will have time to add something like that as part of his effort to integrate the new UI experiment.

@@ -245,7 +245,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), bytes([int(is_atty)]))

This comment has been minimized.

@kwlzn

kwlzn Jul 18, 2018

Member

I think this should be: bytes(str(int(True)), 'ascii') instead.. because what we want here is to cast a bool value to e.g. b'0'/b'1' to set as an env var:

>>> bytes([int(True)])
b'\x01'
>>> bytes(str(int(True)), 'ascii')
b'1'

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 18, 2018

Contributor

Should we be using the python 2 str() here? It seems so -- when I add from builtins import str I get TypeError: unicode string argument without an encoding.

This comment has been minimized.

@kwlzn

kwlzn Jul 18, 2018

Member

looks like the second arg to bytes might be py3 specific.. so for now this is probably the best:

import six
...
six.binary_type(str(int(is_atty)))

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 18, 2018

Contributor

(confirmed that using bytes(str(int(is_atty))) fixes the crash (as expected))

This comment has been minimized.

@kwlzn

kwlzn Jul 18, 2018

Member

bytes(str(int(is_atty))) works too.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 18, 2018

Contributor

Ok, thanks!

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 18, 2018

Contributor

72b0f7e uses six.binary_type -- thanks.


@mock.patch('os.ttyname', autospec=True, spec_set=True)
def test_isatty_to_env_with_mock_tty(self, mock_ttyname):
mock_ttyname.return_value = '/dev/ttys000'

This comment has been minimized.

@kwlzn

kwlzn Jul 18, 2018

Member

this should probably point to a non-existent path to avoid any inadvertent use of that actual static char device.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 18, 2018

Contributor

Oops, should have thought a little harder about that. Fixed in 9d221e6.

{
'NAILGUN_TTY_0': b'\x01',
'NAILGUN_TTY_1': b'\x01',
'NAILGUN_TTY_2': b'\x01',

This comment has been minimized.

@kwlzn

kwlzn Jul 18, 2018

Member

the expected values should all be b'1' in this test case.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 18, 2018

Contributor

Fixed in 9d221e6.

{
'NAILGUN_TTY_0': b'\x00',
'NAILGUN_TTY_1': b'\x00',
'NAILGUN_TTY_2': b'\x00',

This comment has been minimized.

@kwlzn

kwlzn Jul 18, 2018

Member

the expected values should all be b'0' in this test case.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 18, 2018

Contributor

Fixed in 9d221e6.

cosmicexplorer added some commits Jul 18, 2018

@kwlzn

kwlzn approved these changes Jul 18, 2018

Copy link
Member

kwlzn left a comment

LGTM!

@@ -245,7 +245,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), six.binary_type(str(int(is_atty))))

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 19, 2018

Contributor

You can also use future.utils.binary_type(). They're equivalent in meaning, so this isn't worth re-pushing, but as an FYI.

This, however, is not Py3 compatible! The call to str() will have different behavior on Py2 and Py3, because it's missing the import from builtins import str. Right now, in Py2 this code is translating to bytes(bytes(int(is_atty))). In Py3, it translates to bytes(unicode(int(is_atty)))

To fix this, add from builtins import str and see if the tests pass.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 19, 2018

Contributor

Ah! I had tried from builtins import str, but not after switching bytes to six.binary_type -- see my comment above.

While this doesn't need to block getting this fix in, considering that the fix was because of a change we made for python 3 compat, I would strongly prefer to have the fix you described so we don't have knowingly broken code out, so I just pushed dfd1be1 and will wait for CI to pass.

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 19, 2018

Contributor

Hopefully it passes 😶 if it does, it theoretically will work identically on Python 3!

This is another reason the linter you describe would be helpful. This PR would have broken the assumption that the folder has been ported by introducing a Py2 only regression. It’s a lot of overhead for people to have to remember to add from builtins also.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 19, 2018

Contributor

may have to take a look at that linter soon then........

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 19, 2018

Contributor

I only have 2.5 weeks left at Foursquare so won’t be able to get to the linter anytime soon, unfortunately.

I’d definitely be interested in helping after I finish the internship though! I’m planning to stay engaged with this Python 3 port the next year - can’t wait to replace every str.format() with f strings once we drop Python 2 :)

@cosmicexplorer cosmicexplorer merged commit 7b04ad7 into pantsbuild:master Jul 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cosmicexplorer cosmicexplorer deleted the cosmicexplorer:fix-bytes-tty-nailgun-env-daemon branch Jul 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment