Properly render \n in exceptions with Py3 #7073
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When ran with Py3, stacktraces would not properly render new lines and would instead print them as literals. This is because the stacktrace was being printed as a byte string (with a b'' prefix). The issue is from using `print` with bytes. In a Py3 REPL, `print(b"test", file=sys.stderr.buffer)` will raise a TypeError, even though it looks like it should work. Instead, we must use sys.stdout.buffer.write. The only meaningful difference is that print() adds a new line at the end, so we manually add that ourselves. Note certain exceptions are still printing the new line literal.. This only fixes one of the sources of the problem.
It was using sys.stdout, rather than sys.stdout.buffer, in Py3, which originally caused it to print newlines and then with the most recent commit caused it to fail.
…_runner I'm not sure what the results of these changes will be, but they should be using a bytes interface for consistency with Py2 and what we already decided in prior PRs the interfaces should look like.
3 tasks
…te_pants_runner" This reverts commit 290c9c3. While these changes will be necessary down the road, I don't think they're needed to fix this newline rendering issue that this PR is focused on. I'm very confused by how to fix the regressions introduced by this commit, and would like to punt on them for now. I'll rerun this PR against the pants3-in-ci PR to confirm the fix still holds without this commit.
2 tasks
cosmicexplorer
approved these changes
Jan 14, 2019
benjyw
reviewed
Jan 14, 2019
src/python/pants/base/exiter.py
Outdated
| msg = ensure_binary(msg) | ||
| try: | ||
| print(msg, file=out) | ||
| out.write(msg + b'\n') |
There was a problem hiding this comment.
I imagine msg is usually short, but I abhor unnecessary string concatenation. Maybe instead:
out.write(msg)
out.write(b'\n')
?
Good point Benjy. There's nothing to gain from string concatenation here, only a performance hit for code we run frequently.
benjyw
approved these changes
Jan 14, 2019
|
Good find! |
Eric-Arellano
added a commit
that referenced
this pull request
Jan 26, 2019
) ## Problem Any test with `@ensure_daemon` fails when ran with `./pants3`. This is due to unicode issues. For example, `./pants3 test tests/python/pants_test/base:exiter_integration` will fail with: ```python Exception caught: (builtins.TypeError) File "/home/eric/pants/src/python/pants/bin/pants_loader.py", line 89, in <module> main() File "/home/eric/pants/src/python/pants/bin/pants_loader.py", line 85, in main PantsLoader.run() File "/home/eric/pants/src/python/pants/bin/pants_loader.py", line 81, in run cls.load_and_execute(entrypoint) File "/home/eric/pants/src/python/pants/bin/pants_loader.py", line 74, in load_and_execute entrypoint_main() File "/home/eric/pants/src/python/pants/bin/pants_exe.py", line 39, in main PantsRunner(exiter, start_time=start_time).run() File "/home/eric/pants/src/python/pants/bin/pants_runner.py", line 48, in run return RemotePantsRunner(self._exiter, self._args, self._env, options_bootstrapper).run() File "/home/eric/pants/src/python/pants/bin/remote_pants_runner.py", line 190, in run self._run_pants_with_retry(pantsd_handle) File "/home/eric/pants/src/python/pants/bin/remote_pants_runner.py", line 114, in _run_pants_with_retry return self._connect_and_execute(pantsd_handle.port) File "/home/eric/pants/src/python/pants/bin/remote_pants_runner.py", line 155, in _connect_and_execute result = client.execute(self.PANTS_COMMAND, *self._args, **modified_env) File "/home/eric/pants/src/python/pants/java/nailgun_client.py", line 269, in execute return self._session.execute(cwd, main_class, *args, **environment) File "/home/eric/pants/src/python/pants/java/nailgun_client.py", line 109, in execute return self._process_session() File "/home/eric/pants/src/python/pants/java/nailgun_client.py", line 80, in _process_session self._write_flush(self._stdout, payload) File "/home/eric/pants/src/python/pants/java/nailgun_client.py", line 63, in _write_flush fd.write(payload) Exception message: write() argument must be str, not bytes ``` ## Solution Use `sys.std{out,err}.buffer` with Py3. We reaffirmed in #7073 a prior decision that the `Exiter` related code should be using a bytes interface. However, we did not fix the pantsd related code because it was causing regressions. We now fix these usages. Note that in `pants_daemon.py`, we override `sys.stdout` to our own custom `_LoggerStream` object. To ensure Python 3 support, we add a `buffer` property. ## Caveats ### Exiter integration still fails on macOS `tests/python/pants_test/base:exiter_integration` will fail on macOS still, due to an upstream non fork safe osx lib (see https://bugs.python.org/issue28342). But this bug also affects Python 2. ### Python 3.7 daemon never executes See #7160.
This was referenced Jan 27, 2019
Merged
Eric-Arellano
added a commit
to Eric-Arellano/pants
that referenced
this pull request
Jan 31, 2019
Print works just fine when using unicode. The only issue is when using bytes as in pantsbuild#7073. So, I should not have made the original change as print() is simpler and more appropriate to use here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
When ran with Py3, stacktraces would not properly render new lines and would instead print them as literals. This is because the stacktrace was being printed as a byte string (with a
b''prefix).See https://travis-ci.org/pantsbuild/pants/jobs/478884824#L2636, for example.
Solution
Don't use the
printfunction when trying to print bytes, as it does not work as expected when printing bytes. In a Py3 REPL,print(b"test", file=sys.stderr.buffer)will raise aTypeError, even though it looks like it should work.Also use
sys.stderr.bufferwith Py3 to use a bytes interface.Result
Stack traces now readable and printed as intended. See https://travis-ci.org/pantsbuild/pants/jobs/479174634#L2340, which compares to the above example. (Both were run using a Py3 pex).