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

Port bin #6126

Merged
merged 11 commits into from Jul 25, 2018

Conversation

Projects
None yet
3 participants
@Eric-Arellano
Copy link
Contributor

Eric-Arellano commented Jul 15, 2018

Part of #6062

@@ -222,7 +223,7 @@ def post_fork_child(self):

# Broadcast our process group ID (in PID form - i.e. negated) to the remote client so
# they can send signals (e.g. SIGINT) to all processes in the runners process group.
NailgunProtocol.send_pid(self._socket, bytes(os.getpgrp() * -1))
NailgunProtocol.send_pid(self._socket, bytes(str(os.getpgrp() * -1), 'ascii'))

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 15, 2018

Contributor

@kwlzn and I discussed this in the Python3 slack channel. We're first calling str() to cast the int to a str, then bytes() because Nailgun is expecting bytes with its IO

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 16, 2018

There are some actual failures in CI.

@@ -85,9 +85,18 @@ class TruncatedPayloadError(TruncatedRead):
"""Raised if there is a socket error while reading the payload bytes."""

@classmethod
def _decode_unicode_seq(cls, seq):
def _encode_unicode(cls, payload, encoding='utf-8'):

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 22, 2018

Contributor

Extracted helper function from construct_chunk(). Same logic as it had before

return payload

@classmethod
def _encode_unicode_seq(cls, seq):

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 22, 2018

Contributor

I don't think the original function was implemented as intended. It was calling decode(), which tries to convert bytes -> unicode. I think what we really want is unicode -> bytes, because everything with Nailgun and I/O is meant to use bytes.

We didn't catch this for two reasons:

  1. In Py2, calling u'test'.decode() is a no-op, whereas in Py3 it raises AttributeError. This is why the tests have been failing when we use the backported newstr.
  2. The integration tests pass even when removing this function entirely. I think this is because construct_chunk() is also converting from unicode to bytes. So, we could actually completely remove this helper function _encode_unicode_seq()? Thoughts? I think it would simplify things for future readers.

cc @kwlzn

if isinstance(item, bytes):
yield item.decode('utf-8')
else:
yield item

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 23, 2018

Contributor

This changes the semantics less than my earlier proposed change, and should still fix the AttributeError of calling newstr.decode().

I'm still not sure the logic behind going from List[bytes] or List[str] -> List[str] -> str -> bytes, i.e. why we can't pass the bytes directly. But the test test_pantsd_unicode_environment from pants_test/pantsd/test_pantsd_integration.py:553 was failing when I tried to change this with e3877fa.

@stuhood stuhood merged commit e5a5e5e into pantsbuild:master Jul 25, 2018

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:port-bin branch Jul 25, 2018

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

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