-
Notifications
You must be signed in to change notification settings - Fork 39
enable GPU IPC tests on Windows #739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
6da36e1
to
da553d7
Compare
a77f891
to
78361c2
Compare
976028a
to
cfc6f31
Compare
cfc6f31
to
1140442
Compare
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be cleaned up on a successful return also, like in the consumer_connect()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good job in general, a few minor issues
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
LIBS cuda ${UMF_IPC_LIBS} ${UMF_UTILS_FOR_TEST}) | ||
add_umf_ipc_test(TEST ipc_cuda_prov SRC_DIR providers) | ||
endif() | ||
|
||
if(LINUX) | ||
if(UMF_POOL_SCALABLE_ENABLED) | ||
build_umf_test( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -512,7 +581,11 @@ int run_producer(int port, const umf_memory_pool_ops_t *pool_ops, | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you have perhaps a logs from a verbose run on these tests on Windows...?
d0aca41
to
becaa7d
Compare
1d41c75
to
152c072
Compare
enable GPU IPC tests on Windows