enable GPU IPC tests on Windows #739
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bratpiorka
force-pushed
the
rrudnick_ipc_win
branch
5 times, most recently
from
6da36e1 to
da553d7
Compare
bratpiorka
force-pushed
the
rrudnick_ipc_win
branch
3 times, most recently
from
a77f891 to
78361c2
Compare
bratpiorka
changed the title
bratpiorka
force-pushed
the
rrudnick_ipc_win
branch
6 times, most recently
from
976028a to
cfc6f31
Compare
bratpiorka
changed the title
bratpiorka
force-pushed
the
rrudnick_ipc_win
branch
from
cfc6f31 to
1140442
Compare
PatKamin
reviewed
Sep 17, 2025
|
|
||
| set UMF_LOG="level:debug;flush:debug;output:stderr;pid:yes" | ||
|
|
||
| echo "Starting test_ipc_cuda_prov CONSUMER on port %PORT% ..." |
There was a problem hiding this comment.
Could you make it a single script with a binary name parameterized?
|
|
||
| err_WSA_cleanup: | ||
| #ifdef _WIN32 | ||
| WSACleanup(); |
There was a problem hiding this comment.
Shouldn't this be cleaned up on a successful return also, like in the consumer_connect()?
test/CMakeLists.txt
Outdated
| if(WINDOWS) | ||
| message( | ||
| STATUS "DEBUG CMAKE_CURRENT_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}") | ||
| message(STATUS "DEBUG CMAKE_BUILD_TYPE ${CMAKE_BUILD_TYPE}") |
There was a problem hiding this comment.
are these debug messages supposed to be committed...?
test/CMakeLists.txt
Outdated
| message(STATUS "IPC tests are supported on Linux only - skipping") | ||
| message( | ||
| STATUS | ||
| "IPC file, devdax and proxy lib tests are supported on Linux only - skipping" |
There was a problem hiding this comment.
for clarity I'd say: IPC tests for OS, file, devdax providers and proxy lib are supported only on Linux - skipping (including missing "OS provider") - pls remove "OS" if it's working on Windows.
|
|
||
| if(LINUX) | ||
| if(UMF_POOL_SCALABLE_ENABLED) | ||
| build_umf_test( |
There was a problem hiding this comment.
os_prov should be working on Windows, right? if this is not working yet, pls add a TODO, otherwise move it out of if(LINUX) as well, pls
| "(the old one / 2 = %llu) to my shared memory: %llu\n", | ||
| (SHM_number_1 / 2), SHM_number_2); | ||
| } | ||
|
|
There was a problem hiding this comment.
do you have perhaps a logs from a verbose run on these tests on Windows...?
bratpiorka
force-pushed
the
rrudnick_ipc_win
branch
7 times, most recently
from
d0aca41 to
becaa7d
Compare
bratpiorka
force-pushed
the
rrudnick_ipc_win
branch
12 times, most recently
from
1d41c75 to
152c072
Compare
KFilipek
requested changes
Sep 23, 2025
| memcpy(&ze_ipc_handle, &fd_local, sizeof(fd_local)); | ||
| } | ||
|
|
||
| CloseHandle(source_process_handle); | ||
|
|
||
| return result ? UMF_RESULT_SUCCESS : UMF_RESULT_ERROR_UNKNOWN; | ||
| } |
There was a problem hiding this comment.
It will always return UMF_RESULT_SUCCESS since you check error at 79.
Otherwise:
Suggested change
| } | |
| release_current: | |
| CloseHandle(current_process_handle); | |
| release_source: | |
| CloseHandle(source_process_handle); | |
| return result; | |
| } |
Comment on lines
+79
to
+84
| if (!result) { | ||
| LOG_ERR("DuplicateHandle() failed for pid=%d fd_in=%d.", pid, fd_in); | ||
| CloseHandle(current_process_handle); | ||
| CloseHandle(source_process_handle); | ||
| return UMF_RESULT_ERROR_UNKNOWN; | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (!result) { | |
| LOG_ERR("DuplicateHandle() failed for pid=%d fd_in=%d.", pid, fd_in); | |
| CloseHandle(current_process_handle); | |
| CloseHandle(source_process_handle); | |
| return UMF_RESULT_ERROR_UNKNOWN; | |
| } | |
| if (!result) { | |
| LOG_ERR("DuplicateHandle() failed for pid=%d fd_in=%d.", pid, fd_in); | |
| result = UMF_RESULT_ERROR_UNKNOWN; | |
| goto release_current; | |
| } |
Comment on lines
+70
to
+74
| if (!source_process_handle) { | ||
| LOG_ERR("OpenProcess() failed for pid=%d.", pid); | ||
| CloseHandle(current_process_handle); | ||
| return UMF_RESULT_ERROR_UNKNOWN; | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (!source_process_handle) { | |
| LOG_ERR("OpenProcess() failed for pid=%d.", pid); | |
| CloseHandle(current_process_handle); | |
| return UMF_RESULT_ERROR_UNKNOWN; | |
| } | |
| BOOL result; | |
| if (!source_process_handle) { | |
| LOG_ERR("OpenProcess() failed for pid=%d.", pid); | |
| result = UMF_RESULT_ERROR_UNKNOWN; | |
| goto release_source; | |
| } |
|
|
||
| int producer_connect(int port) { | ||
| struct sockaddr_in consumer_addr; | ||
| int ret = -1; |
There was a problem hiding this comment.
Used only at the end of the function
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
enable GPU IPC tests on Windows