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

Python 3 issues - os.environ bytes vs unicode #6222

Merged
merged 1 commit into from Jul 26, 2018

Conversation

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

Eric-Arellano commented Jul 23, 2018

Problem

In Py2, os.environ expects either bytes or ASCII strings. So, to pass it unicode, you must encode into bytes and pass the bytes, which is why we have several helper functions.

In Py3, it expects unicode and will crash upon receiving bytes.

We didn't catch this because there is no backport for the os module, and this is the first time running contextutil with Py3.

Solution

If Py2, keep existing conversion from unicode to bytes. If Py3, don't modify the passed in unicode.

Testing

CI is only going to prove this doesn't break Py2. It has no guarantees Py3 will work.. I got the unit tests to work, but there may still be other issues I don't know how to detect because we can't yet run CI in Py3 (too many other issues that have to be resolved)

@stuhood
Copy link
Member

stuhood left a comment

Thanks.


One facility that it would be great to look into with regard to getting the first support for Python3 integration tests in place would be to do a mashup of https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/pants_run_integration_test.py#L115-L121 and https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/pants_run_integration_test.py#L62-L69 that could be used like:

@ensure_23
def test_some_integration_test(self):
  ..

Getting that working would involve a bit more fiddling in that file to ensure that PANTS_SCRIPT_NAME is invoked with the appropriate options. What those would look like would depend on whether we were running a pex or a venv script, but.



@contextmanager
def hermetic_environment_as(**kwargs):
"""Set the environment to the supplied values from an empty state."""
old_environment = _copy_and_decode_env(os.environ)
old_environment = os.environ.copy() if PY3 else _copy_and_decode_env(os.environ)

This comment has been minimized.

@stuhood

stuhood Jul 23, 2018

Member

It seems like it would be cleaner to do the PY3 check in _copy_and_decode_env and _os_encode rather than before calling those methods?

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 23, 2018

Contributor

I thought about that. But if we want the function names to stay true to their implementation, we would have to rename _copy_and_decode_env to something like _copy_env and _os_encode to _os_encode_if_py2, which doesn't seem ideal either.

@Eric-Arellano
Copy link
Contributor

Eric-Arellano left a comment

Thanks for the suggestion on Py3 integration test compatibility! I'll look into it as a separate issue.

We would likely still run into the issue of Py3 tests failing for reasons unrelated to changes to the file, though, unfortunately. Until every test is green with Py3, it will be ambiguous what is causing failures.

For now, we'll have to make do with CI only checking for Py2 regressions.



@contextmanager
def hermetic_environment_as(**kwargs):
"""Set the environment to the supplied values from an empty state."""
old_environment = _copy_and_decode_env(os.environ)
old_environment = os.environ.copy() if PY3 else _copy_and_decode_env(os.environ)

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 23, 2018

Contributor

I thought about that. But if we want the function names to stay true to their implementation, we would have to rename _copy_and_decode_env to something like _copy_env and _os_encode to _os_encode_if_py2, which doesn't seem ideal either.

@stuhood stuhood merged commit 2887d3c into pantsbuild:master Jul 26, 2018

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:py3-fixes_contextutil-os-environ branch Jul 26, 2018

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

Fix issue of os.environ expecting bytes vs unicode in Py2 vs Py3 (pan…
…tsbuild#6222)

### Problem
In Py2, `os.environ` expects either bytes or ASCII strings. So, to pass it unicode, you must encode into bytes and pass the bytes, which is why we have several helper functions.

In Py3, it expects unicode and will crash upon receiving bytes.

We didn't catch this because there is no backport for the `os` module, and this is the first time running `contextutil` with Py3.

### Solution
If Py2, keep existing conversion from unicode to bytes. If Py3, don't modify the passed in unicode.

### Testing
CI is only going to prove this doesn't break Py2. It has no guarantees Py3 will work.. I got the unit tests to work, but there may still be other issues I don't know how to detect because we can't yet run CI in Py3 (too many other issues that have to be resolved)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment