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 unicode handling in Exiters #6032

Merged
merged 3 commits into from Jun 27, 2018

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

stuhood commented Jun 26, 2018

Problem

Three unicode issues had snuck into the two Exiter implementations (with/without pantsd), which prevented a useful error from rendering in the local case, and caused pantsd to hang up in the remote case.

Solution

Add a unicode-containing compilation failure integration test that runs with/without pantsd, and fix the three issues.

Result

Unicode-containing exceptions should render correctly in more cases.

@stuhood stuhood requested review from benjyw and kwlzn Jun 26, 2018

task(name='scalafix', action=ScalaFixFix).install('fmt')
task(name='scalafmt', action=ScalaFmtFormat, serialize=False).install('fmt')

This comment has been minimized.

@kwlzn

kwlzn Jun 26, 2018

Member

expected in this review?

This comment has been minimized.

@stuhood

stuhood Jun 26, 2018

Member

Oh, derp. Bad rebase. Fixing.

@stuhood stuhood force-pushed the twitter:stuhood/unicode-in-exiters branch from 89de819 to 06d85a3 Jun 26, 2018

@kwlzn

kwlzn approved these changes Jun 26, 2018

Copy link
Member

kwlzn left a comment

lgtm!

@@ -71,7 +72,7 @@ def exit(self, result=0, msg=None, out=None):
:param out: The file descriptor to emit `msg` to. (Optional)
"""
if msg:
print(msg, file=out or sys.stderr)
print(msg.encode('UTF8'), file=out or sys.stderr)

This comment has been minimized.

@kwlzn

kwlzn Jun 26, 2018

Member

utf-8?

This comment has been minimized.

@stuhood

stuhood Jun 26, 2018

Member

Either works, apparently. Although funny that I managed to spell this two ways in one review.

@@ -106,7 +107,7 @@ def _log_exception(self, msg):
exception_log.write('timestamp: {}\n'.format(datetime.datetime.now().isoformat()))
exception_log.write('args: {}\n'.format(sys.argv))
exception_log.write('pid: {}\n'.format(os.getpid()))
exception_log.write(msg)
exception_log.write(msg.encode('utf8'))

This comment has been minimized.

@kwlzn

kwlzn Jun 26, 2018

Member

utf-8?

@stuhood stuhood added this to the 1.8.x milestone Jun 26, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jun 27, 2018

Spurious failure. Merging.

@stuhood stuhood merged commit 8fdee38 into pantsbuild:master Jun 27, 2018

1 check failed

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

@stuhood stuhood deleted the twitter:stuhood/unicode-in-exiters branch Jun 27, 2018

stuhood added a commit that referenced this pull request Jun 27, 2018

Fix unicode handling in Exiters (#6032)
### Problem

Three unicode issues had snuck into the two `Exiter` implementations (with/without `pantsd`), which prevented a useful error from rendering in the local case, and caused `pantsd` to hang up in the remote case.

### Solution

Add a unicode-containing compilation failure integration test that runs with/without `pantsd`, and fix the three issues.

### Result

Unicode-containing exceptions should render correctly in more cases.

stuhood added a commit that referenced this pull request Jun 27, 2018

Fix bad exclusion introduced during rushed change. (#6034)
In a hurry while landing #6032, I introduced a bug in the exclusions list for testprojects. I waited until Travis completed, but assumed that the failure I saw was spurious. Narrator: "The error was NOT spurious."

stuhood added a commit that referenced this pull request Jun 27, 2018

Fix bad exclusion introduced during rushed change. (#6034)
In a hurry while landing #6032, I introduced a bug in the exclusions list for testprojects. I waited until Travis completed, but assumed that the failure I saw was spurious. Narrator: "The error was NOT spurious."
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment