Perfect your code
With built-in code review tools, GitHub makes it easy to raise the quality bar before you ship. Join the 40 million developers who've merged over 200 million pull requests.
Sign up for free See pricing for teams and enterprisesFixed #28174 -- Fixed crash in runserver's autoreload with Python 2 on Windows with non-str environment variables. #8737
Conversation
timgraham
changed the title
|
Add a release note in docs/releases/1.11.4.txt. You can squash commits and use the commit message from the PR title. |
| @@ -285,6 +285,9 @@ def restart_with_reloader(): | |||
| while True: | |||
| args = [sys.executable] + ['-W%s' % o for o in sys.warnoptions] + sys.argv | |||
| new_environ = os.environ.copy() | |||
| if sys.platform == "win32": | |||
This comment has been minimized.
This comment has been minimized.
timgraham
Jul 12, 2017
Member
There's a _win module level constant that could be used. Also, a comment seems appropriate. I believe the patch is only needed for Python 2 so maybe also check six.PY2? (not sure if this could cause an issue on Python 3 or not)
This comment has been minimized.
This comment has been minimized.
mrogaski
Jul 12, 2017
Author
I can't think of a scenario in Python 3 where this would occur without effort to make it happen. I think it's worth keeping the scope narrow.
| def test_restart_with_reloader(self): | ||
| os.environ['SPAM'] = u'spam' | ||
| with mock.patch.object(sys, 'argv', ['-c', 'pass']): | ||
| try: |
This comment has been minimized.
This comment has been minimized.
timgraham
Jul 12, 2017
Member
The try/except can be omitted. If the test fails with an error, that's fine.
| @@ -0,0 +1,20 @@ | |||
| import os | |||
This comment has been minimized.
This comment has been minimized.
timgraham
Jul 12, 2017
Member
Please put this test in the existing test_autoreload.py file in the same directory.
| """ | ||
|
|
||
| def test_restart_with_reloader(self): | ||
| os.environ['SPAM'] = u'spam' |
This comment has been minimized.
This comment has been minimized.
timgraham
Jul 12, 2017
Member
We use from __future__ import unicode_literals rather than u'' prefix.
| @@ -285,6 +285,9 @@ def restart_with_reloader(): | |||
| while True: | |||
| args = [sys.executable] + ['-W%s' % o for o in sys.warnoptions] + sys.argv | |||
| new_environ = os.environ.copy() | |||
| if sys.platform == "win32": | |||
| for key in new_environ.keys(): | |||
| new_environ[key] = str(new_environ[key]) | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mrogaski
Jul 12, 2017
Author
Yes, it should ... and turning on unicode_literals exposes the need. Thanks.
| autoreload is used. | ||
| """ | ||
|
|
||
| def test_restart_with_reloader(self): |
This comment has been minimized.
This comment has been minimized.
timgraham
Jul 12, 2017
Member
def test_environment(self):
""""
With Python 2 on Windows, restart_with_reloader() coerces environment
variables to str to avoid "TypeError: environment can only contain
strings" in Python's subprocess.py.
"""
| from django.utils import autoreload | ||
|
|
||
|
|
||
| class TestEnvironmentGrooming(SimpleTestCase): |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
All recommended changes have been applied. |
| # we coerce both to the str type. | ||
| for key in new_environ.keys(): | ||
| str_key = str(key) | ||
| str_value = str(new_environ[key]) |
This comment has been minimized.
This comment has been minimized.
charettes
Jul 12, 2017
Member
I guess this is going to crash if the value contains non-ASCII chars? Maybe we should be using force_bytes(s, encoding=SYSTEM_ENCODING) instead of a bare str() call? I'm not a Windows expert but I think we have similar handling of data the localization tools.
| strings" in Python's subprocess.py. | ||
| """ | ||
| def test_environment(self): | ||
| os.environ['SPAM'] = 'spam' |
This comment has been minimized.
This comment has been minimized.
charettes
Jul 12, 2017
Member
I know that this is only meant to land in 1.11.x wonder if using u'' prefixes/force_text() would make what we're testing more clear.
This comment has been minimized.
This comment has been minimized.
mrogaski
Jul 12, 2017
Author
I think it's a good example of how most people will encounter the problem if we leave it as is. Indeed, unicode_literals is how I hit it. If clarity is a concern, an extra comment should be enough.
This comment has been minimized.
This comment has been minimized.
|
Updated to use |
…ython 2 on Windows with non-str environment variables.
timgraham
merged commit fc6b90b
into
django:stable/1.11.x
This comment has been minimized.
This comment has been minimized.
|
Mark, did you get a Trac notification about a possible regression? |
This comment has been minimized.
This comment has been minimized.
|
Yep, I'll pick that issue up. It looks like an issue with the return value of |
mrogaski commentedJul 12, 2017
Ensure that all environment values are str objects to avoid a TypeError.
Ticket: https://code.djangoproject.com/ticket/28174