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

sys.stdout being patched but not reset when error raised #723

Closed
tonybaloney opened this Issue Jan 3, 2018 · 3 comments

Comments

Projects
4 participants
@tonybaloney
Contributor

tonybaloney commented Jan 3, 2018

This is a bug in master, under the following circumstances:

If you read the following PR explaining a change to venv.py and changing the sys.stdout to a stream writer (temporarily). This stream writer does NOT have the same behaviour as sys.stdout (which is a buffered FileIOWrapper, not a stream writer.
https://bitbucket.org/hpk42/tox/pull-requests/115/fixed-console-encoding-issue/diff

These lines in venv make the change https://github.com/tox-dev/tox/blob/master/tox/venv.py#L304-L308

However (and this is the bug), when there is an invocation error, in action.popen, for example when the command fails, the lines of code to clean up sys.stdout are never called because the error is raised up the stack and caught elsewhere. This invocation error is raised here https://github.com/tox-dev/tox/blob/master/tox/session.py#L152

Any libraries calling Tox as a package, not using the CLI will therefore have unicode encoding errors whenever they run a tox action and it fails to operate the command correctly.

The lines to set sys.stdout to a temporary value should be inside a try block and the cleanup within a finally. I can raise the PR but raising the bug for reference.

tonybaloney added a commit to tonybaloney/tox that referenced this issue Jan 4, 2018

tonybaloney added a commit to tonybaloney/tox that referenced this issue Jan 4, 2018

gaborbernat added a commit that referenced this issue Jan 5, 2018

add safety wrapper around sys.stdout assignment (#724)
* add safety wrapper around sys.stdout assignment see #723
@tonybaloney

This comment has been minimized.

Contributor

tonybaloney commented Jan 5, 2018

Issue resolved in #724

@obestwalter

This comment has been minimized.

Member

obestwalter commented Jan 23, 2018

Thanks @tonybaloney for tracking that one down.

@obestwalter

This comment has been minimized.

Member

obestwalter commented Jan 24, 2018

Just as some extra info: I changed the bug severity from critical to normal, because it definitely was not a showstopper. I would even argue that it was not a bug as such - at least not in that particular use case, because tox - just like pip - has no official programmatic API. Only command line use is officially supported. I don't say this cannot change, but this is the status quo.

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