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

Python 3 fixes - fix net/http issues with bytes vs unicode #6258

Merged
merged 3 commits into from Jul 29, 2018

Conversation

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

Eric-Arellano commented Jul 28, 2018

Almost all functions should be using bytes in this module, because it deals with I/O. We were using unicode in some places.

Tests pass locally on Py2 and Py3.

@@ -126,6 +128,7 @@ def __init__(self, digest=None):
self._checksum = None

def recv_chunk(self, data):
data = [ensure_binary(v) for v in data] if isinstance(data, list) else ensure_binary(data)

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 28, 2018

Contributor

Doing a check for list to avoid converting a string into individual letters, which was happening with one test case.

@@ -160,7 +163,7 @@ def __init__(self, width=None, chunk_size_bytes=None, stream=None):
if not isinstance(self._width, six.integer_types):
raise ValueError('The width must be an integer, given {}'.format(self._width))
self._chunk_size_bytes = chunk_size_bytes or 10 * 1024
self._stream = stream or sys.stderr
self._stream = stream or (sys.stderr.buffer if PY3 else sys.stderr)

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 28, 2018

Contributor

Stream should be byte stream, so calling sys.stderr.buffer in Py3.

@@ -176,24 +179,26 @@ def status(self, code, content_length=None):
self.read = 0

def recv_chunk(self, data):
data = [str(num).encode('utf-8') for num in data] if isinstance(data, list) else str(data).encode('utf-8')

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 28, 2018

Contributor

Numbers must first be cast to strings so that encode can work. Not calling bytes() directly, because its behavior changes in Py2 vs Py3, i.e. b'1' vs b'\x00'

FetcherRedirectTest._URL2_ACCESSED = True
elif self.path.endswith('url1'):
self.send_response(200)
self.wfile.write('\nreturned from redirect')
self.end_headers()

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 28, 2018

Contributor

Without this line, HTTPServer fails to parse the status code in Py3. This line was the cause of a lot of pain 😑

@@ -378,4 +379,4 @@ def test_download_redirect(self):
self.assertTrue(self._URL1_ACCESSED)

with open(path) as fp:
self.assertEqual('returned from redirect\r\n', fp.read())
self.assertIn(fp.read(), ['returned from redirect\n', 'returned from redirect\r\n'])

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 28, 2018

Contributor

In Py2, HTTP Server adds \r\n and in Py3 just \n. I'm concerned this may be a platform specific implementation, which is why I changed this test to assertIn, rather than using an if PY3 check

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@stuhood stuhood merged commit e94727a into pantsbuild:master Jul 29, 2018

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:py3-fixes_net branch Jul 30, 2018

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

Python 3 fixes - fix net/http issues with bytes vs unicode (pantsbuil…
…d#6258)

Almost all functions should be using bytes in this module, because it deals with I/O. We were using unicode in some places. 

Tests pass locally on Py2 and Py3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment