Skip to content
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

Fix `./pants server` not working with Python 3 #7458

Merged
merged 1 commit into from Mar 30, 2019

Conversation

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

commented Mar 29, 2019

Problem

Fixes #7443.

Due to various unicode issues, ./pants server would always crash when you clicked the link to the server.

Solution

Fix unicode issues.

Also add an assertion that the argument to self._send_content() is bytes so that Python 2 behaves the same as Python 3. Notably, we do not try to coerce the content in this method because we cannot assume which encoding it should use, e.g. assets do not use UTF-8.

Result

./pants server works consistently, including taking all actions like browsing the codebase.


def _send_content(self, content, content_type, code=200):
"""Send content to client."""
assert isinstance(content, bytes)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 29, 2019

Author Contributor

This assertion is added so that Python 2 behaves like Python 3 does.

Note we do not coerce the content here, because we don't know what the encoding should be, e.g. if it's an asset that doesn't use UTF-8. So, we require the caller to encode its content beforehand.

@Eric-Arellano
Copy link
Contributor Author

left a comment

cc @codealchemy

@Eric-Arellano Eric-Arellano added this to the 1.15.x milestone Mar 29, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Reviewers, we don't have very good integration testing for ./pants server it seems. Otherwise we would have caught this earlier.

I didn't add testing here because we need to land this fix into 1.15.0 and I don't have time (nor the knowledge) to write comprehensive tests.

Instead, I tested this out locally by running ./pants server, opening the link, and trying to stress test the UI, e.g. clicking every link. When reviewing this, would be helpful to also try to break ./pants server.

@stuhood
Copy link
Member

left a comment

Thanks!

@Eric-Arellano Eric-Arellano merged commit 59a2345 into pantsbuild:master Mar 30, 2019

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:fix-server branch Mar 30, 2019

stuhood added a commit that referenced this pull request Apr 1, 2019

Fix `./pants server` not working with Python 3 (#7458)
### Problem
Fixes #7443.

Due to various unicode issues, `./pants server` would always crash when you clicked the link to the server.

### Solution
Fix unicode issues.

Also add an assertion that the argument to `self._send_content()` is bytes so that Python 2 behaves the same as Python 3. Notably, we do not try to coerce the content in this method because we cannot assume which encoding it should use, e.g. assets do not use UTF-8.

### Result
`./pants server` works consistently, including taking all actions like browsing the codebase.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.