Issue 11750: Mutualize win32 functions (original) (raw)

Issue11750

process

Created on 2011-04-03 18:50 by pitrou, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Messages (27)
msg132868 - (view)	Author: Antoine Pitrou (pitrou) *	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) *	Date: 2011-04-03 20:58
+1
msg132884 - (view)	Author: Jesse Noller (jnoller) *	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) *	Date: 2011-04-03 21:05
Big +1. I'll work up a patch.
msg134011 - (view)	Author: Brian Curtin (brian.curtin) *	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) *	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) *	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) *	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) *	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) *	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) *	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) *	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) *	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) *	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) *	Date: 2012-04-15 11:21
(fixed wsock32.lib in revision ab0aff639cfb)
msg158327 - (view)	Author: Richard Oudkerk (sbt) *	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) *	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) *	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) *	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) *	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) *	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) *	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) *	Date: 2012-04-16 16:02
s/_win32/_winapi/g
msg158539 - (view)	Author: Antoine Pitrou (pitrou) *	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) *	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) *	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).