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 most of src unit tests green #6372

Merged
merged 42 commits into from Aug 29, 2018

Conversation

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

Eric-Arellano commented Aug 21, 2018

No description provided.

try:
reversed_tasktypes_by_name = reversed(self._tasktypes_by_name.items())
except TypeError:
reversed_tasktypes_by_name = reversed(list(self._tasktypes_by_name.items()))

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 21, 2018

Contributor

Very weird Py3.4 quirk. Using the try construct because future.utils doesn't have PY34 and I don't like using sys.info.

Show resolved Hide resolved tests/python/pants_test/engine/examples/parsers.py
{type_alias_json},
{friend_json},
{relative_json}
}}

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 21, 2018

Contributor

Yay dictionary order changing 😵

@stuhood
Copy link
Member

stuhood left a comment

Thanks Eric!

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Aug 22, 2018

Earlier I was planning on breaking this out into several smaller PRs, but now that I got the integration tests to pass, I'm hoping to keep this as one.

The remaining issue is getting releases to work. I can reproduce locally, but the results seem slightly non-deterministic? Originally I git bisected to 5347cad, but then after incrementelly rebuilding that commit it suddenly was passing. Another time I git bisected to eb4372b. So, while I'm able to get failures locally, I can't determine what the issue is or why it doesn't consistently fail.

Tomorrow is my busiest day of school week if anyone has time to look, otherwise I'll get to it tomorrow or Thursday. (Probably more helpful to address the other issues, though, especially #6382 because I can't repro locally.)

To reproduce:

  1. ./pants clean-all (I'd do this every invocation)
  2. ./build-support/bin/release.sh -n

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:py3-green_src branch from 18c923a to 75f3571 Aug 24, 2018

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Aug 27, 2018

@benjyw I'm still on flight and don't have WiFi on work laptop or access to Slack, but have internet on personal computer and was able to trigger a rebase from GitHub's web app.

Would be great to include this in release if it's still within the timeline. Thanks!

@illicitonion
Copy link
Contributor

illicitonion left a comment

One test failure... Maybe re-ignore that, and let's merge? :)

class ExecuteProcessResult(datatype(['stdout', 'stderr', 'output_directory_digest'])):
class ExecuteProcessResult(datatype([('stdout', binary_type),
('stderr', binary_type),
'output_directory_digest'])):

This comment has been minimized.

@illicitonion

illicitonion Aug 28, 2018

Contributor

This should be of type DirectoryDigest - might be nice to add type constraints to all three rather than leaving it weirdly partially-typed :)

(As with FallibleExecuteProcessResult)

class FallibleExecuteProcessResult(datatype(['stdout', 'stderr', 'exit_code', 'output_directory_digest'])):
class FallibleExecuteProcessResult(datatype([('stdout', binary_type),
('stderr', binary_type),
'exit_code',

This comment has been minimized.

@illicitonion

illicitonion Aug 28, 2018

Contributor

exit_code is a numeric type (specifically an int32, but I guess an int in python-world?)

# Neither treat __new__ as a method.
# Always treat __init__ and __new__ as methods, to drop self from their args.
if inspect.ismethod(func) or func.__name__ == '__new__' or func.__name__ == '__init__':
if 'self' in arg_names or 'cls' in arg_names:

This comment has been minimized.

@illicitonion

illicitonion Aug 28, 2018

Contributor

if arg_names[:1] == ['self'] or arg_names[:1] == ['cls']: or if arg_names and arg_names[0] in {'self', 'cls'}: or something similar?

if PY3 else
"[u'darwin', u'linux'].")
self.assertIn(expected_msg, the_raised_exception_message)
if PY3:

This comment has been minimized.

@illicitonion

illicitonion Aug 28, 2018

Contributor

I would rather just sort the keys when we produce the error message. We could even get rid of the icky dict_keys and u' prefixes :)

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Aug 28, 2018

Thanks for the close review!

Agreed let’s punt that one failing test - this is actually the first time I’ve seen the failure, and I think it’s due to Py3.4’s dictionary randomness (which I discovered we can turn on for python 2!)

Will fix your suggestions in a couple hours when I’m free.

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Aug 29, 2018

@Eric-Arellano I pushed a tiny commit to this branch to hopefully get travis green :)

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Aug 29, 2018

@illicitonion Looks great, thanks for that patch!

It's been far too long since I first started working on this locally 15 days ago 😱 Excited to see it merged soon.

@@ -69,13 +69,20 @@ def __new__(
)


class ExecuteProcessResult(datatype(['stdout', 'stderr', 'output_directory_digest'])):
class ExecuteProcessResult(datatype([('stdout', binary_type),

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 29, 2018

Contributor

I have typically been indenting these as follows when it's too long to fit on one line:

class ExecuteProcessResult(datatype([
  ('stdout', binary_type),
  ('stderr', binary_type),
  ('output_directory_digest', DirectoryDigest),
])):
  # ...

I have zero problem changing my style, just would be neat to settle on one (I hadn't considered the layout you're using here before, it seems perfectly legit).

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 29, 2018

Contributor

I agree actually! I like that style more and that is how we do things at Foursquare.

In the interest of landing this, can I fix in a follow up PR?

@stuhood
Copy link
Member

stuhood left a comment

Thanks a ton Eric!

@stuhood stuhood merged commit 0258a07 into pantsbuild:master Aug 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-green_src branch Aug 29, 2018

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