classification
Title: Mutualize win32 functions
Type: enhancement Stage: resolved
Components: Extension Modules, Windows Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, asksol, brian.curtin, gregory.p.smith, jnoller, kristjan.jonsson, pitrou, python-dev, santoso.wijaya, sbt, tim.golden
Priority: normal Keywords: needs review, patch

Created on 2011-04-03 18:50 by pitrou, last changed 2012-04-18 18:53 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
issue11750.diff brian.curtin, 2011-04-19 03:45 review
windows_module.patch sbt, 2012-04-14 19:30 review
windows_module.patch sbt, 2012-04-15 13:33 review
win32_module.patch sbt, 2012-04-15 17:52 review
winapi_module.patch sbt, 2012-04-16 16:02 review
winapi_module.patch sbt, 2012-04-17 13:20 review
Messages (27)
msg132868 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-04-03 18:50
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.
msg132882 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2011-04-03 20:58
+1
msg132884 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2011-04-03 21:04
Agreed; I'm not personally the windows expert that should handle that consolidation though.
msg132885 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2011-04-03 21:05
Big +1. I'll work up a patch.
msg134011 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2011-04-19 03:45
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.
msg134012 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-04-19 06:31
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.
msg134048 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2011-04-19 13:06
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.
msg134338 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-04-24 18:38
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
msg134339 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-04-24 18:39
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".
msg158206 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-04-13 11:05
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.)
msg158276 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-04-14 19:30
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.
msg158277 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2012-04-14 19:37
I don't think we need the vcproj file, unless I missed something.
msg158278 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-04-14 19:57
> 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?
msg158283 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-04-14 21:17
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.
msg158318 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-04-15 11:21
(fixed wsock32.lib in revision ab0aff639cfb)
msg158327 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-04-15 13:33
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(-)
msg158329 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-15 14:14
> 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.
msg158345 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-04-15 17:52
> 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().
msg158349 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2012-04-15 18:26
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.
msg158358 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-04-15 19:33
>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?
msg158463 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-04-16 15:13
> How about _windowsapi or _winapi then, to ensure there are no clashes?

I don't have any strong feelings, but I would prefer _winapi.
msg158465 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-16 15:26
> > 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.
msg158477 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-04-16 16:02
s/_win32/_winapi/g
msg158539 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-17 10:41
> 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).
msg158549 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-04-17 13:20
> Overlapped's naming is still lagging behind :-)

Argh.  And a string in winapi_module too.

Yet another patch.
msg158648 - (view) Author: Roundup Robot (python-dev) Date: 2012-04-18 18:52
New changeset f3a27d11101a by Antoine Pitrou in branch 'default':
Issue #11750: The Windows API functions scattered in the _subprocess and
http://hg.python.org/cpython/rev/f3a27d11101a
msg158649 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-18 18:53
Thanks a lot for doing this! Patch now committed to 3.3 (after testing under Windows 7 64 bits).
History
Date User Action Args
2012-04-18 18:53:40pitrousetcomponents: + Windows
2012-04-18 18:53:09pitrousetstatus: open -> closed
messages: + msg158649

assignee: brian.curtin ->
components: - Windows
resolution: fixed
stage: patch review -> resolved
2012-04-18 18:52:23python-devsetnosy: + python-dev
messages: + msg158648
2012-04-17 13:20:04sbtsetfiles: + winapi_module.patch

messages: + msg158549
2012-04-17 10:41:08pitrousetmessages: + msg158539
2012-04-16 16:02:16sbtsetfiles: + winapi_module.patch

messages: + msg158477
2012-04-16 15:26:40pitrousetmessages: + msg158465
2012-04-16 15:13:18sbtsetmessages: + msg158463
2012-04-15 19:33:08kristjan.jonssonsetmessages: + msg158358
2012-04-15 18:26:37brian.curtinsetmessages: + msg158349
2012-04-15 17:52:23sbtsetfiles: + win32_module.patch

messages: + msg158345
2012-04-15 14:14:19pitrousetmessages: + msg158329
2012-04-15 13:33:59sbtsetfiles: + windows_module.patch

messages: + msg158327
2012-04-15 11:21:48kristjan.jonssonsetmessages: + msg158318
2012-04-14 21:17:08kristjan.jonssonsetmessages: + msg158283
2012-04-14 19:57:12sbtsetmessages: + msg158278
2012-04-14 19:37:45brian.curtinsetmessages: + msg158277
2012-04-14 19:30:07sbtsetfiles: + windows_module.patch

messages: + msg158276
2012-04-14 10:09:31kristjan.jonssonsetnosy: + kristjan.jonsson
2012-04-13 11:05:01sbtsetmessages: + msg158206
2012-04-12 20:54:10pitrousetnosy: + sbt
2011-04-24 18:39:19pitrousetmessages: + msg134339
2011-04-24 18:38:23pitrousetmessages: + msg134338
2011-04-19 13:06:04brian.curtinsetmessages: + msg134048
2011-04-19 06:31:14amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg134012
2011-04-19 05:17:03santoso.wijayasetnosy: + santoso.wijaya
2011-04-19 03:46:14brian.curtinsetkeywords: + needs review
stage: patch review
2011-04-19 03:45:44brian.curtinsetfiles: + issue11750.diff
keywords: + patch
messages: + msg134011
2011-04-03 21:05:37brian.curtinsetassignee: brian.curtin
messages: + msg132885
2011-04-03 21:04:06jnollersetmessages: + msg132884
2011-04-03 20:58:32tim.goldensetmessages: + msg132882
2011-04-03 18:50:29pitroucreate