gh-77589: Add unix domain socket for Windows #137420
Conversation
PC/pyconfig.h
Outdated
| @@ -671,6 +671,9 @@ Py_NO_ENABLE_SHARED to find out. Also support MS_NO_COREDLL for b/w compat */ | |||
| /* Define if you have the <sys/un.h> header file. */ | |||
| /* #define HAVE_SYS_UN_H 1 */ | |||
|
|
|||
| /* Define if you have the <afunix.h> header file. */ | |||
| #define HAVE_AFUNIX_H 1 | |||
There was a problem hiding this comment.
There is a question for how to detect if the header file exists.
There was a problem hiding this comment.
Fixed by check #if defined(NTDDI_WIN10_RS4) && (NTDDI_VERSION >= NTDDI_WIN10_RS4).
There was a problem hiding this comment.
#if defined(NTDDI_WIN10_RS4) && (NTDDI_VERSION >= NTDDI_WIN10_RS4) did'nt work because NTDDI_VERSION is defined in PC\pyconfig.h with hard code value NTDDI_WIN10 which is lesser than NTDDI_WIN10_RS4.
There was a problem hiding this comment.
Instead of skipping some tests, would not be worth to test a Windows specific behavior?
Some tests can currently be skipped on Windows, but they may work after adding support of AF_UNIX. Please check all currently skipped tests for socket and multiprocessing.
| @unittest.skipIf(sys.platform == 'win32', | ||
| 'Windows allow bind on empty path') |
There was a problem hiding this comment.
Should not we test that it does not fail on Windows?
| @unittest.skipIf(sys.platform == 'win32', | ||
| 'Windows will raise Error if is not bound') |
There was a problem hiding this comment.
Should not we test that it raises Error if is not bound?
| @@ -2747,6 +2747,7 @@ def test_is_socket_false(self): | |||
| @unittest.skipIf( | |||
| is_wasi, "Cannot create socket on WASI." | |||
| ) | |||
| @unittest.skipIf(sys.platform=='win32', "didn't work on Windows") | |||
There was a problem hiding this comment.
What does not work? Should not we have a test for this?
| @@ -215,6 +215,7 @@ def test_devices(self): | |||
| break | |||
|
|
|||
| @socket_helper.skip_unless_bind_unix_socket | |||
| @unittest.skipIf(sys.platform=='win32', "didn't work on Windows") | |||
There was a problem hiding this comment.
Should not we add a test for this?
|
It seems that the previous attempt did have more changes in the |
The previous attempt modified the The change in |
Didn't add
asynciosupport in this PR to avoid too many code changes and keep the review process simpler.