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 contrib folders problems #6272

Merged
merged 17 commits into from Jul 31, 2018

Conversation

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

Eric-Arellano commented Jul 30, 2018

Most folders are still failing due to the Rust ImportError, but this fixes a lot of their issues.

I'll be submitting a separate PR for porting contrib/python, because it involves major changes resulting from changes in the AST grammar that are better reviewed independently from this.

Questions

  1. There are no tests for confluence. Is there a way to test for Py3 compatibility?
  2. Node has several functions to interface directly with subprocess, e.g. node/subsystems/command.py. I didn't change any of the source code for node, but fixed the unit tests to deal with bytes vs unicode. Should I be changing instead the underlying implementation? There is already logic in some places around bytes vs unicode, so I figured it's safer to leave as intended. This means uses of node may break in Py3, and the responsibility will be on the library user to fix their use.
@Eric-Arellano
Copy link
Contributor

Eric-Arellano left a comment

You'll notice the vast majority of changes are made to test code, not source code.

Most of the meaningful source code changes are at the bottom, and are made within the src/python/pants folder, not contrib.

Most of the test changes were innocuous, i.e. don't suggest to me that we need to fix the underlying implementation. node is the only one where we might want to.

@@ -219,7 +219,7 @@ def make_target(self,
if make_missing_sources and 'sources' in kwargs:
for source in kwargs['sources']:
if '*' not in source:
self.create_file(os.path.join(address.spec_path, source), mode='a')
self.create_file(os.path.join(address.spec_path, source), mode='a', contents='')

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 30, 2018

Contributor

Defaults tocontents='b', so this fixes bytes vs unicode.

@@ -429,13 +429,14 @@ def file_renamed(self, prefix, test_name, real_name):
os.rename(real_path, test_path)

@contextmanager
def temporary_file_content(self, path, content):
def temporary_file_content(self, path, content, binary_mode=True):

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 30, 2018

Contributor

Keeping default as binary_mode, because the author originally had it set to wb.

@@ -64,7 +65,7 @@ class Registrar(BuildFileTargetFactory):
def __init__(self, parse_context, type_alias, object_type):
self._parse_context = parse_context
self._type_alias = type_alias
self._object_type = object_type
self._object_type = object_type if inspect.isclass(object_type) else type(object_type)

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 30, 2018

Contributor

Sometimes Registrar was being passed an instance, rather than a type. To see a use case of this, add a debugger line

if inspect.isclass(object_type):
  import pdb; pdb.set_trace()

In that case, this will grab the underlying type.

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 30, 2018

Contributor

Hm, this is the result of all the test failures. So the above is not the correct solution.

I'll play around with this tomorrow.

In particular, I'm curious why in Python 2 the error about issubclass() arg 1 having to a class is not triggering, whereas it does in Python 3. When using the repl, issubclass(1, int) fails in both Py2 and Py3, so I would think it would fail here too. Yet the error would only raise in Python 3 here.

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 31, 2018

As to the questions: don't worry about confluence / node.js. The former is lightly used, and the latter is about to see a bunch of refactoring.

@stuhood stuhood merged commit 9acbac7 into pantsbuild:master Jul 31, 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_contrib branch Jul 31, 2018

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

Python 3 fixes - fix contrib folders problems (pantsbuild#6272)
Most folders are still failing due to the Rust ImportError, but this fixes a lot of their issues.

I'll be submitting a separate PR for porting contrib/python, because it involves major changes resulting from changes in the AST grammar that are better reviewed independently from this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment