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

Port pantsd/ to python3 #6136

Merged
merged 6 commits into from Jul 21, 2018

Conversation

Projects
None yet
4 participants
@ghthor
Copy link
Contributor

ghthor commented Jul 16, 2018

Part of #6062.

I'm worried about this directory. Futurize reported files needing changes, but then made zero changes. It first failed on a parsing error, which was fixed by the single commit in this branch.

@ghthor ghthor force-pushed the ghthor:python3-port-pantsd branch from d016ca3 to 3223cc5 Jul 16, 2018

@ghthor

This comment has been minimized.

Copy link
Contributor

ghthor commented Jul 16, 2018

I've updated after I figured out why I wasn't getting the expected automated changes coming out of futurize.

@ghthor ghthor changed the title Port pantsd Port pantsd/ to python3 Jul 16, 2018

@ghthor ghthor force-pushed the ghthor:python3-port-pantsd branch from 25e9081 to 172ec97 Jul 16, 2018

@ghthor

This comment has been minimized.

Copy link
Contributor

ghthor commented Jul 16, 2018

I have a bad feeling this is some inconsistency between linux/osx https://travis-ci.org/pantsbuild/pants/jobs/404537075#L925

@ghthor

This comment has been minimized.

Copy link
Contributor

ghthor commented Jul 16, 2018

This[1] should be fairly straightforward to fix. The bytes constructor requires an encoding to be passed when the string is a unicode string.

[1] https://travis-ci.org/pantsbuild/pants/jobs/404537077#L5784

standard_library.install_aliases()



This comment has been minimized.

@stuhood

stuhood Jul 16, 2018

Member

I expect that CI will fail on this.

Recommend either manually running build-support/bin/pre-commit.sh, or installing (and skipping when necessary) the commithooks, as described here: https://www.pantsbuild.org/howto_contribute.html#getting-pants-source-code

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 16, 2018

Contributor

You're right Stu. We don't want to ever use this idiom because isort changes the order where it no longer works as expected. standard_library.install_aliases() is supposed to happen before the relevant import, but isort always moves it below every import.

Instead, we have to use from future.moves import x. The tricky part is figuring out what standard_library.install_aliases() was overriding. I use autocomplete and try out every import to see where it's coming from, e.g. see if future.moves.contextlib exists. Sometimes I can't find anything, so I delete the import because I think it might be an over-optimization?

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 16, 2018

Contributor

I'm also not sure why this line was being added. None of the above libraries should need future.moves. I think you can delete this + from future import standard_library

This comment has been minimized.

@stuhood

stuhood Jul 16, 2018

Member

IIRC, you can also use # noqa comments with isort? Not certain though.

This comment has been minimized.

@stuhood

stuhood Jul 16, 2018

Member

But the reason I mentioned that this will fail is that there is a style check for 2 blank lines between top level definitions.

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 16, 2018

Contributor

Unfortunately # noqa does not work with isort :/ The first day of this project was me spending 4-5 hours trying to hack a solution to getting isort to stop moving the line, until finally discovering future.moves

standard_library.install_aliases()



This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 16, 2018

Contributor

You're right Stu. We don't want to ever use this idiom because isort changes the order where it no longer works as expected. standard_library.install_aliases() is supposed to happen before the relevant import, but isort always moves it below every import.

Instead, we have to use from future.moves import x. The tricky part is figuring out what standard_library.install_aliases() was overriding. I use autocomplete and try out every import to see where it's coming from, e.g. see if future.moves.contextlib exists. Sometimes I can't find anything, so I delete the import because I think it might be an over-optimization?

from pants.pantsd.service.pants_service import PantsService


standard_library.install_aliases()

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 16, 2018

Contributor

This should be replaced with from future.moves import x.

I don't see why this is even necessary though, logging, threading, and time are in Py2 and Py3 without major changes and I can't find a relevant library stored from future.moves

standard_library.install_aliases()



This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 16, 2018

Contributor

I'm also not sure why this line was being added. None of the above libraries should need future.moves. I think you can delete this + from future import standard_library

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 20, 2018

It looks like there is a meaningful failure in the 5th shard.

@ghthor ghthor force-pushed the ghthor:python3-port-pantsd branch from c710342 to 8ea913b Jul 20, 2018

@ghthor ghthor force-pushed the ghthor:python3-port-pantsd branch from 8ea913b to 6cb860c Jul 20, 2018

@jsirois jsirois merged commit 7ad0298 into pantsbuild:master Jul 21, 2018

1 check passed

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

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

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