Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#3964 defect closed fixed (fixed)

_dumbwin32proc breaks with unicode environmental variables

Reported by: ivank Owned by: PenguinOfDoom
Priority: highest Milestone:
Component: core Keywords:
Cc: Thijs Triemstra Branch: branches/unicode-environ-3964-2
branch-diff, diff-cov, branch-cov, buildbot
Author: pahan

Description

Windows supports unicode environmental variables. pywin32 supports spawning processes with str or unicode environmental variables, but not a mix of them:

...
  File "C:\Python26\lib\site-packages\twisted\internet\utils.py", line 25, in _callProtocolWithDeferred
    reactor.spawnProcess(p, executable, (executable,)+tuple(args), env, path)
  File "C:\Python26\lib\site-packages\twisted\internet\posixbase.py", line 234, in spawnProcess
    return Process(self, processProtocol, executable, args, env, path)
  File "C:\Python26\lib\site-packages\twisted\internet\_dumbwin32proc.py", line 176, in __init__
    doCreate()
  File "C:\Python26\lib\site-packages\twisted\internet\_dumbwin32proc.py", line 174, in doCreate
    command, cmdline, None, None, 1, 0, env, path, StartupInfo)
exceptions.TypeError: All dictionary items must be strings, or all must be unicode

This problem and "solution" is sort of documented here: https://realityforge.vrsource.org/viewvc/maestro/trunk/maestro/daemon/plugins/services/launch/process.py , near "win32process.CreateProcess requires that all environment keys and"

This bug causes a lot of test failures with twisted.web2 on Windows Vista or Windows 7. The default install of Python 2.6.2 on these operating systems introduces environmental variables with unicode values:

C:\>python -c "import Tkinter, os; exec 'for k, v in os.environ.iteritems():\n\tif isinstance(v, unicode): print k, v'" TK_LIBRARY C:\Python26\tcl\tk8.5 TIX_LIBRARY C:\Python26\tcl\tix8.4.3 TCL_LIBRARY C:\Python26\tcl\tcl8.5

Change History (18)

comment:1 Changed 8 years ago by ivank

Both keys and values are unicode, or they are both ANSI, because of how CreateProcess works:

http://msdn.microsoft.com/en-us/library/ms682425%28VS.85%29.aspx

" lpEnvironment

[...]

An environment block consists of a null-terminated block of null-terminated strings. Each string is in the following form:

name=value\0

Because the equal sign is used as a separator, it must not be used in the name of an environment variable.

An environment block can contain either Unicode or ANSI characters. If the environment block pointed to by lpEnvironment contains Unicode characters, be sure that dwCreationFlags includes CREATE_UNICODE_ENVIRONMENT.

[...] "

comment:2 Changed 8 years ago by ivank

Priority: normalhigh

setting this to high priority, given that it very often breaks spawnProcess on Vista/Win7

comment:3 Changed 8 years ago by Glyph

Owner: changed from Glyph to PenguinOfDoom

comment:4 Changed 8 years ago by pahan

Author: pahan
Branch: branches/unicode-environ-3964

(In [28748]) Branching to 'unicode-environ-3964'

comment:5 Changed 8 years ago by PenguinOfDoom

Keywords: review added
Owner: PenguinOfDoom deleted
Priority: highhighest

comment:6 Changed 8 years ago by jesstess

Cc: jesstess added
Keywords: review removed
Owner: set to PenguinOfDoom

There don't appear to be commits on this branch. Removing from review.

Also, the link in the ticket description sounds very valuable but 404s for me.

comment:7 Changed 8 years ago by PenguinOfDoom

Keywords: review added
Owner: PenguinOfDoom deleted

oops :((((

comment:8 Changed 8 years ago by jesstess

Owner: set to jesstess

comment:9 Changed 8 years ago by jesstess

Cc: jesstess removed
Keywords: review removed
Owner: changed from jesstess to PenguinOfDoom

Thanks for tackling this, PenguinOfDoom! Some feedback:

test_process.py

  • use the plural forms of asserts in test_encodableUnicodeEnvironment
  • use 2 blank lines between functions, as per the coding standard
  • Given the changes in _dumbwin23proc.py, I would have expected a test case that tries to set up both string and unicode environment variables and asserted that no errors were raised and they were all converted to unicode on process creation, which doesn't seem to be what's going on in test_encodableUnicodeEnvironment. I don't have a Windows machine on which I can play around with this test case easily, so I could be wrong about what's going on - can you clarify what's going on in the test's docstring if it is handling this case and/or add this scenario as a new test case?

_dumbwin23proc.py

  • needs a copyright bump
  • has an unused failure import
  • can you clarify this comment: "# in year 2000, win32process.CreateProcess cannot deal with mixed str/unicode environment, so we make it all Unicode"? Or maybe just take out the "in year 2000" part since it's a problem even now in 2010.
  • Process.init is doing an insane amount compared to your average init, so it really needs a docstring, in conjunction with epytext markup for the instance variables in the class docstring. doCreate could also use a docstring.
  • There's a TODO above doCreate. If it is still relevant, it needs a ticket.

comment:10 Changed 7 years ago by Jean-Paul Calderone

(In [29358]) Fix some simple coding standard issues and update to a more modern testing style

refs #3964

comment:11 Changed 7 years ago by Jean-Paul Calderone

(In [29359]) Remove a confusing joke from this comment; meager beginning to a docstring for Process.__init__

refs #3964

comment:12 Changed 7 years ago by Jean-Paul Calderone

(In [29360]) Point to some existing tickets

refs #3964

comment:13 Changed 7 years ago by pahan

Branch: branches/unicode-environ-3964branches/unicode-environ-3964-2

(In [30958]) Branching to 'unicode-environ-3964-2'

comment:14 Changed 7 years ago by PenguinOfDoom

Keywords: review added
Owner: PenguinOfDoom deleted

comment:15 Changed 7 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

comment:16 Changed 7 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to PenguinOfDoom
Status: assignednew

Build results look okay. Unfortunate about that string comparison in the exception handler... Perhaps we can file a bug against pywin32 to get a custom exception type to detect this case? Otherwise, fine. Add a news file describing the bug fix and please merge.

comment:17 Changed 7 years ago by pahan

Resolution: fixed
Status: newclosed

(In [31033]) Merge unicode-environ-3964-2: Support Unicode in system environment under Windows

Author: PenguinOfDoom Reviewer: exarkun Fixes: #3964

comment:18 in reply to:  17 Changed 7 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

Replying to pahan:

Reviewer: exarkun

aww jesstess :(

Note: See TracTickets for help on using tickets.