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 - add open() backport to safe_open() #6304

Merged
merged 14 commits into from Aug 6, 2018

Conversation

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

Eric-Arellano commented Aug 4, 2018

Final stage of adding Python 3 open semantics. Builds off of #6291, #6295, and #6290.

@@ -58,7 +58,7 @@ def test_avro_java_gen(self):
)
'''))

self.create_file(relpath='avro-build/src/avro/schema.avsc', mode='w', contents=dedent('''
self.create_file(relpath='avro-build/src/avro/schema.avsc', contents=dedent('''

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 4, 2018

Contributor

Originally I had set the default mode for create_file to binary. After looking at our use cases, I don't think I've found even one where we try writing binary, so a far more sensible default is to open in text mode. It will be less surprising to users of the function than getting a bytes vs unicode issue.

Whenever we need bytes mode, it can be turned on with mode='wb'.

@@ -99,29 +98,27 @@ def test_noop_parse(self):

def test_invalid_unicode_in_build_file(self):
"""Demonstrate that unicode characters causing parse errors raise real parse errors."""
# TODO(python3port): remove ensure_binary once safe_open() uses backport. This test fails on Py3.
self.add_to_build_file('BUILD', ensure_binary(dedent(
self.add_to_build_file('BUILD', dedent(

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 4, 2018

Contributor

We now have consistent Py2 and Py3 behavior with this test! Originally we had to choose to support one or the other, but not both.

Eric-Arellano added some commits Aug 4, 2018

Fix Pantsd issues resulting from issues writing to safe_file_dump.
Pantsd was throwing this exception in integration tests:
Exception message: abruptly lost active connection to pantsd runner: NailgunError(u"Problem talking to nailgun server (address: 127.0.0.1:52211): error(54, 'Connection reset by peer')",)

It turns out this had nothing to do with Nailgun Server. Instead, the issue was `safe_file_dump` silently failing to write metadata for pantsd. The open backport should theoretically have thrown a TypeError with this, but it didn't for some reason. I believe Py3 would have thrown an error, although cannot test due to the Rust ImportError.
@@ -332,7 +332,7 @@ def _run_services(self, services):

# Once all services are started, write our pid.
self.write_pid()
self.write_metadata_by_name('pantsd', self.FINGERPRINT_KEY, self.options_fingerprint)
self.write_metadata_by_name('pantsd', self.FINGERPRINT_KEY, self.options_fingerprint.decode('utf-8'))

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 6, 2018

Contributor

This was wild to debug.

Pantsd was throwing this exception in integration tests:
Exception message: abruptly lost active connection to pantsd runner: NailgunError(u"Problem talking to nailgun server (address: 127.0.0.1:52211): error(54, 'Connection reset by peer')",)

It turns out this had nothing to do with Nailgun Server. Instead, the issue was safe_file_dump silently failing to write metadata for pantsd. The open backport should theoretically have thrown a TypeError with this, but it didn't for some reason. I believe Py3 would have thrown an error, although cannot test due to the Rust ImportError.
ecda20b

In the process, I realized hasher.hexdigest() has inconsistent behavior between Py2 and Py3. In Py2, it returns bytes. In Py3, it returns unicode. As a separate PR, I'm going to change our usage of sha1() so that we always return unicode. I think it will catch some issues for us, similar to this open backport project.

@stuhood stuhood merged commit 792d703 into pantsbuild:master Aug 6, 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_safe-open branch Aug 6, 2018

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

Eric-Arellano added a commit that referenced this pull request Dec 20, 2018

Use stdout with bytes for task context (#6968)
We decided in #6304 and #6360 that `console_task.py` should be writing bytes to its `self._outstream`.

However, in `context.py`, when using Python 3, we had set the stdout stream to use unicode, leading to an issue when running Pants with Python 3.

We now consistently use byte streams.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment