-
Notifications
You must be signed in to change notification settings - Fork 798
[CI][SYCL][Test] Make check-sycl-unittests
also run unit tests with ABI breaking changes.
#19687
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
Conversation
…eaking changes enabled.
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.
cmake lgtm
|
||
if(WIN32) | ||
# Windows doesn't support LD_LIBRARY_PATH, so instead we copy the mock OpenCL binary next to the test and ensure | ||
# that the test itself links to OpenCL (rather than through ur_adapter_opencl.dll) | ||
set(mock_ocl ${CMAKE_CURRENT_BINARY_DIR}/OpenCL.dll) | ||
|
||
# An error is thrown if both the preview and non-preview versions try to |
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.
unrelated but do we really need to copy the opencl library to the test dir?
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.
I think so. Before my changes, there was a comment (https://github.com/intel/llvm/blob/sycl/sycl/cmake/modules/AddSYCLUnitTest.cmake#L79) saying:
# Windows doesn't support LD_LIBRARY_PATH, so instead we copy the mock OpenCL binary next to the test and ensure
# that the test itself links to OpenCL (rather than through ur_adapter_opencl.dll)
- name: check-sycl-unittests-preview | ||
if: always() && !cancelled() && contains(inputs.changes, 'sycl') | ||
run: | | ||
# TODO consider moving this to Dockerfile. | ||
export LD_LIBRARY_PATH=/usr/local/cuda/compat/:/usr/local/cuda/lib64:$LD_LIBRARY_PATH | ||
cmake --build $GITHUB_WORKSPACE/build --target check-sycl-unittests-preview |
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.
IMO, existing target should just check both.
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 mean that check-sycl-unittests
should check both preview and non-preview?
OR
Was you comment specific to the CI job, like a single job for both preview and non-preview unittest run?
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.
check-sycl-unittests
should check both preview and non-preview.
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.
Done in 22d66d0
check-sycl-unittests
also run unit tests with ABI breaking changes.
endif() | ||
endif() | ||
|
||
# This is done to ensure that preview tests are kept in a seperate |
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.
# This is done to ensure that preview tests are kept in a seperate | |
# This is done to ensure that preview tests are kept in a separate |
This PR makes
check-sycl-unittests
CMake target run both preview and non-preview version of unit tests.