Mutualize win32 functions

[pitrou]

BPO	11750
Nosy	@gpshead, @amauryfa, @pitrou, @kristjanvalur, @tjguk, @briancurtin
Files	issue11750.diff windows_module.patch windows_module.patch win32_module.patch winapi_module.patch winapi_module.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2012-04-18.18:53:09.962>
created_at = <Date 2011-04-03.18:50:29.052>
labels = ['extension-modules', 'type-feature', 'OS-windows']
title = 'Mutualize win32 functions'
updated_at = <Date 2012-04-18.18:53:40.835>
user = 'https://github.com/pitrou'

bugs.python.org fields:

activity = <Date 2012-04-18.18:53:40.835>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2012-04-18.18:53:09.962>
closer = 'pitrou'
components = ['Extension Modules', 'Windows']
creation = <Date 2011-04-03.18:50:29.052>
creator = 'pitrou'
dependencies = []
files = ['21710', '25217', '25223', '25224', '25241', '25252']
hgrepos = []
issue_num = 11750
keywords = ['patch', 'needs review']
message_count = 27.0
messages = ['132868', '132882', '132884', '132885', '134011', '134012', '134048', '134338', '134339', '158206', '158276', '158277', '158278', '158283', '158318', '158327', '158329', '158345', '158349', '158358', '158463', '158465', '158477', '158539', '158549', '158648', '158649']
nosy_count = 11.0
nosy_names = ['gregory.p.smith', 'amaury.forgeotdarc', 'pitrou', 'kristjan.jonsson', 'tim.golden', 'jnoller', 'brian.curtin', 'asksol', 'santoso.wijaya', 'python-dev', 'sbt']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue11750'
versions = ['Python 3.3']

[pitrou]

subprocess and multiprocessing both have their own private modules for wrappers of win32 functions: Modules/_multiprocessing/win32_functions.c and PC/_subprocess.c.

It would be nice to group them in a common module (_win32?) that could be used throughout the stdlib.

[tjguk]

+1

[jnoller]

Agreed; I'm not personally the windows expert that should handle that consolidation though.

[briancurtin]

Big +1. I'll work up a patch.

[briancurtin]

Here's a patch replacing Modules/_multiprocessing/win32_functions.c and PC/_subprocess.c with a common PC/_windows.c. There's not much to the patch despite its size -- it just shuffles around the C code and does a few renames in the appropriate Python modules. All tests pass.

I left the copyright notice from PC/_subprocess.c at the top. No idea if that needs to stay or not.

[amauryfa]

Two high-level remarks about the patch:

• IMO there is no reason to put _windows.c in the PC directory. After all, there is no such distinction for posix-specific modules.
• wxPython already has a submodule named _windows.py. I wonder if this will cause problems.

[briancurtin]

For the first point, I just put it there since other Windows-only modules already exist there. _subprocess did, msvcrt and winreg currently do, and there's a few other Windows-only things in there. It's not a big deal, so I can move it into Modules if we want -- winreg and msvcrt should probably get moved as well (in another issue).

As for the name clash, I could shorten it to _win, but I'd rather not name it _win32. Microsoft got away from calling it the "Win32 API" and instead say "Windows API" now since it also covers 64-bit. It's just an internal name so I won't fight too hard on this.

[pitrou]

I agree with Amaury that it would be better in Modules. In my experience, code that is in PC/ is a pain to discover.
A couple of nits about the patch:

• the functions in the PyMethodDef array could be sorted alphabetically
• the defint() macro isn't necessary, PyModule_AddIntMacro() should do the trick
• the "_win_handle_object" thing seems misguided, I would vote to remove it, and call CloseHandle() from Python code instead

[pitrou]

PS: I don't think there's a problem with the "_windows" name, as long as wxPython doesn't depend on a *toplevel* module named "_windows".

[sbt]

I think there are some issues with the treatment of the DWORD type. (DWORD is a typedef for unsigned long.)

_subprocess always treats them as signed, whereas _multiprocessing treats them (correctly) as unsigned. _windows does a mixture: functions from _subprocess parse DWORD arguments as signed ("i"), functions from _multiprocessing parse DWORD arguments as unsigned ("k"), and the constants are signed.

So in _windows the constants GENERIC_READ, NMPWAIT_WAIT_FOREVER and INFINITE will be negative. I think this will potentially cause errors from PyArg_ParseTuple() when used as arguments to functions from _multiprocessing.

I think it is also rather confusing that some functions (eg CreatePipe()) return handles using a wrapper type which closes on garbage collection, while others (eg CreateNamedPipe()) return handles as plain integers.

(The code also needs updating because quite a few functions have since been added to _multiprocessing.win32.)

[sbt]

Attached is an up to date patch.

• code has been moved to Modules/_windows.c
• DWORD is uniformly treated as unsigned
• _subprocess's handle wrapper type has been removed (although
  subprocess.py still uses a Python implemented handle wrapper type)

I'm not familiar with Visual Studio. I ended up copying _socket.vcproj
to _windows.vcproj and editing it by hand. I also edited
_multiprocessing.vcproj and pythoncore.vcproj by hand.

[briancurtin]

I don't think we need the vcproj file, unless I missed something.

[sbt]

I don't think we need the vcproj file, unless I missed something.

_multiprocessing.win32 currently wraps closesocket(), send() and recv() so it needs to link against ws2_32.lib.

I don't know how to make _windows link against ws2_32.lib without adding a vcproj file for _windows unless we make pythoncore depend on ws2_32.lib. I presume this is why _socket and _select have their own vcproj files.

Maybe the socket functions could be moved directly to the top level of _multiprocessing instead since they are not really win32 functions. (And I suppose if that does not happen then _multiprocessing should also stop linking against ws2_32.lib.)

BTW why does _select link against wsock32.lib instead of ws2_32.lib?

[kristjanvalur]

It shouldn't. I noticed this and fixed this at CCP a while back but I wasn't in Python Committer mode at the time. _select needs fixing.

[kristjanvalur]

(fixed wsock32.lib in revision ab0aff639cfb)

[sbt]

New patch. Compared to the previous one:

• socket functions have been moved from _windows to _multiprocessing
• _windows.vcpoj has been removed (so _windows is part of pythoncore.vcproj)
• no changes to pcbuild.sln needed
• removed reference to 'win32_functions.c' in setup.py

(I am not sure whether/how setup.py is used on Windows.)

Lib/multiprocessing/connection.py | 124 +-
Lib/multiprocessing/forking.py | 31 +-
Lib/multiprocessing/heap.py | 6 +-
Lib/multiprocessing/reduction.py | 6 +-
Lib/subprocess.py | 104 +-
Lib/test/test_multiprocessing.py | 2 +-
Modules/_multiprocessing/multiprocessing.c | 83 +-
Modules/_multiprocessing/win32_functions.c | 823 ----------------
Modules/_windows.c | 1337 +++++++++++++++++++++++++++
PC/_subprocess.c | 697 --------------
PC/config.c | 6 +-
PCbuild/_multiprocessing.vcproj | 4 -
PCbuild/pythoncore.vcproj | 8 +-
setup.py | 1 -
14 files changed, 1568 insertions(+), 1664 deletions(-)

[pitrou]

New patch. Compared to the previous one:

• socket functions have been moved from _windows to _multiprocessing
• _windows.vcpoj has been removed (so _windows is part of pythoncore.vcproj)
• no changes to pcbuild.sln needed
• removed reference to 'win32_functions.c' in setup.py

I think the module would be better named _win32, since that's the name
of the API (like POSIX under Unix).

Also, it seems there are a couple of naming inconsistencies renaming
(e.g. the overlapped wrapper is named
"_multiprocessing.win32.Overlapped")

Otherwise, I guess it's ok.

(I am not sure whether/how setup.py is used on Windows.)

Neither do I. It may be used under mingw or cygwin, but we don't
officially support these.

[sbt]

I think the module would be better named _win32, since that's the name
of the API (like POSIX under Unix).

Changed in new patch.

Also, it seems there are a couple of naming inconsistencies renaming
(e.g. the overlapped wrapper is named "_multiprocessing.win32.Overlapped")

I've fixed that one (and changed the initial comment at the beginning of _win32.c), but I can't see any other.

I also removed a duplicate of getulong().

[briancurtin]

pythoncore.vcproj)

> * no changes to pcbuild.sln needed
> * removed reference to 'win32_functions.c' in setup.py

I think the module would be better named _win32, since that's the name
of the API (like POSIX under Unix).

While there are many references to it being called Win32 API around the
web, at some point it became the Windows API.

[kristjanvalur]

at some point it became the Windows API.
You are right: http://en.wikipedia.org/wiki/Windows_API
How about _windowsapi or _winapi then, to ensure there are no clashes?

[sbt]

How about _windowsapi or _winapi then, to ensure there are no clashes?

I don't have any strong feelings, but I would prefer _winapi.

[pitrou]

> How about _windowsapi or _winapi then, to ensure there are no clashes?

I don't have any strong feelings, but I would prefer _winapi.

Ditto here.

[sbt]

s/_win32/_winapi/g

[pitrou]

sbt <shibturn@gmail.com> added the comment:

s/_win32/_winapi/g

Overlapped's naming is still lagging behind :-)

Other than that, a comment:

+ def Close(self):
+ if not self.closed:
+ self.closed = True
+ _winapi.CloseHandle(self)

Since Close() can be called at shutdown (through __del__), it should
probably cache its references to globals (because of the unpredictable
order of module cleanup), like this:

+ def Close(self, CloseHandle=_winapi.CloseHandle):
+ if not self.closed:
+ self.closed = True
+ CloseHandle(self)

Otherwise, looks good (I haven't tested).

[sbt]

Overlapped's naming is still lagging behind :-)

Argh. And a string in winapi_module too.

Yet another patch.

[python-dev]

New changeset f3a27d11101a by Antoine Pitrou in branch 'default':
Issue bpo-11750: The Windows API functions scattered in the _subprocess and
http://hg.python.org/cpython/rev/f3a27d11101a

[pitrou]

Thanks a lot for doing this! Patch now committed to 3.3 (after testing under Windows 7 64 bits).