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 to get backend mostly green #6360

Merged
merged 25 commits into from Aug 20, 2018

Conversation

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

Eric-Arellano commented Aug 17, 2018

Changes to get the majority of unit and integration tests passing for backend/ folders.

This PR does not solve everything because some of the tests are hard to get passing, but this solves a substantial amount of issues.

@@ -539,8 +539,8 @@ def __eq__(self, other):
# TODO(python3port): Return NotImplemented if other does not have attributes
def __lt__(self, other):
# We can't just re-use __repr__ or __str_ because we want to order rev last
return ((self.org, self.name, self.classifier, self.ext, self.rev) <
(other.org, other.name, other.classifier, other.ext, other.rev))
return ((self.org, self.name, self.classifier or '', self.ext, self.rev) <

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 17, 2018

Contributor

self.classifier can be None, which can't be compared to strings in Py3.

@@ -300,7 +300,7 @@ def collect_reduced_dependencies(current):
class SetupPy(Task):
"""Generate setup.py-based Python projects."""

SOURCE_ROOT = b'src'
SOURCE_ROOT = 'src' if PY3 else b'src'

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 17, 2018

Contributor

This file is a little hacky to get to work on both versions.

It relies on pprint to generate the content written to file. In Py2, unicode is prefaced with u, and in Py3 bytes are prefaced with b. The goal is to have no prefix displayed, so to do this we have to use bytes in Py2 and unicode in Py3.

@@ -546,8 +551,8 @@ def convert(input):
return out
elif isinstance(input, list):
return [convert(element) for element in input]
elif isinstance(input, string):
return to_bytes(input)
elif PY2 and isinstance(input, str):

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 17, 2018

Contributor

If Py3, keep as unicode

This comment has been minimized.

@stuhood

stuhood Aug 17, 2018

Member

Hm. That would mean that one branch would result in bytes, while another results in a unicode/string?

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 17, 2018

Contributor

Exactly, due to this:

This file is a little hacky to get to work on both versions.

It relies on pprint to generate the content written to file. In Py2, unicode is prefaced with u, and in Py3 bytes are prefaced with b. The goal is to have no prefix displayed, so to do this we have to use bytes in Py2 and unicode in Py3.

I hate us having to have different implementations between Py2 and Py3, but I'm not sure how else to get this to work.

This comment has been minimized.

@stuhood

stuhood Aug 17, 2018

Member

Ok. That kind of comment should probably go in the code.

@@ -29,7 +29,7 @@ def register_options(cls, register):

def __init__(self, *args, **kwargs):
super(ConsoleTask, self).__init__(*args, **kwargs)
self._console_separator = self.get_options().sep.decode('unicode_escape')
self._console_separator = self.get_options().sep.encode('ascii').decode('unicode_escape')

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 17, 2018

Contributor

The order matters here. I originally had encode('unicode_escape').decode('ascii') and it resulted in Pants outputting \\\\n instead of \n.

This comment has been minimized.

@stuhood

stuhood Aug 17, 2018

Member

Should probably still encode this as unicode, in case someone wants to separate with an emoji =P

diff = self.minor - other.minor
if diff:
return self.patch < other.patch
return self.snapshot < other.snapshot

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 17, 2018

Contributor

snapshot is a boolean

Eric-Arellano added some commits Aug 17, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@@ -546,8 +551,8 @@ def convert(input):
return out
elif isinstance(input, list):
return [convert(element) for element in input]
elif isinstance(input, string):
return to_bytes(input)
elif PY2 and isinstance(input, str):

This comment has been minimized.

@stuhood

stuhood Aug 17, 2018

Member

Hm. That would mean that one branch would result in bytes, while another results in a unicode/string?

@@ -29,7 +29,7 @@ def register_options(cls, register):

def __init__(self, *args, **kwargs):
super(ConsoleTask, self).__init__(*args, **kwargs)
self._console_separator = self.get_options().sep.decode('unicode_escape')
self._console_separator = self.get_options().sep.encode('ascii').decode('unicode_escape')

This comment has been minimized.

@stuhood

stuhood Aug 17, 2018

Member

Should probably still encode this as unicode, in case someone wants to separate with an emoji =P

@@ -55,7 +55,7 @@ def execute(self):
targets = self.context.targets()
for value in self.console_output(targets) or tuple():
self._outstream.write(value.encode('utf-8'))
self._outstream.write(self._console_separator.encode('utf-8'))
self._outstream.write(self._console_separator.encode('ascii'))

This comment has been minimized.

@stuhood

stuhood Aug 17, 2018

Member

Ditto unicode.

@@ -76,7 +76,8 @@ def test_agent_dependency(self):
jar = "dist/manifest-with-agent.jar"
with open_zip(jar, mode='r') as j:
with j.open("META-INF/MANIFEST.MF") as jar_entry:
entries = {tuple(line.strip().split(": ", 2)) for line in jar_entry.readlines() if line.strip()}
normalized_lines = (line.strip().decode('utf-8') for line in jar_entry.readlines() if line.strip())

This comment has been minimized.

@stuhood

stuhood Aug 17, 2018

Member

Decode before strip? ... probably doesn't matter, but.


try:
self.assertIn("ModuleNotFoundError: No module named 'colors'", stderr_data)
except AssertionError:

This comment has been minimized.

@stuhood

stuhood Aug 17, 2018

Member

Better to just check the PY3 variable?

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 17, 2018

Contributor

I went with try construct here because this isn't Py2 vs Py3, it's Py3.6+ vs everything else, and future.utils doesn't have a PY36 value and using sys.info doesn't read very well.

@jsirois jsirois merged commit 7fb7c83 into pantsbuild:master Aug 20, 2018

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:py3-green_backend branch Aug 20, 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