-
Notifications
You must be signed in to change notification settings - Fork 94
[ci] Build hip-tests #1606
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?
[ci] Build hip-tests #1606
Conversation
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.
Nice, thanks for taking this on! Since this isn't marked as a draft PR, I'll leave some review comments.
Please fill in the PR description and tag #1354 .
core/CMakeLists.txt
Outdated
therock_cmake_subproject_declare(hip-tests | ||
USE_DIST_AMDGPU_TARGETS | ||
EXTERNAL_SOURCE_DIR "${THEROCK_ROCM_SYSTEMS_SOURCE_DIR}/projects/hip-tests/catch" |
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.
Does this work as-is? I think @geomin12 tried something similar and saw conflicts between the forked version of Catch2 in hip-tests (https://github.com/ROCm/rocm-systems/tree/develop/projects/hip-tests/catch/external/Catch2) and the version we bundle in TheRock:
TheRock/third-party/Catch2/CMakeLists.txt
Lines 1 to 14 in 5c520ac
therock_subproject_fetch(therock-catch2-sources | |
CMAKE_PROJECT | |
# Originally mirrored from: https://github.com/catchorg/Catch2/archive/refs/tags/v3.8.1.tar.gz | |
URL https://rocm-third-party-deps.s3.us-east-2.amazonaws.com/Catch2-3.8.1.tar.gz | |
URL_HASH SHA256=18b3f70ac80fccc340d8c6ff0f339b2ae64944782f8d2fca2bd705cf47cadb79 | |
) | |
therock_cmake_subproject_declare(therock-catch2 | |
BACKGROUND_BUILD | |
EXCLUDE_FROM_ALL | |
NO_MERGE_COMPILE_COMMANDS | |
OUTPUT_ON_FAILURE | |
EXTERNAL_SOURCE_DIR "${CMAKE_CURRENT_BINARY_DIR}/source" | |
) |
core/CMakeLists.txt
Outdated
therock_cmake_subproject_declare(hip-tests | ||
USE_DIST_AMDGPU_TARGETS | ||
EXTERNAL_SOURCE_DIR "${THEROCK_ROCM_SYSTEMS_SOURCE_DIR}/projects/hip-tests/catch" |
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 also wonder if we'd want to run other samples from hip-tests (https://github.com/ROCm/rocm-systems/tree/develop/projects/hip-tests/samples), or if the catch/
folder has enough representative tests. The samples did look more standalone and harder to integrate into this build system...
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.
The goal is to have sharding work with hip-tests and have it run through the pre-submit tests we see in the legacy CI systems.
[components.test."core/hip-tests/stage"] | ||
include = [ | ||
"share/hip/**", | ||
] |
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 there no binaries that should be included or are these located at share/hip
?
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.
From build logs in the Azure CI system that builds hip-tests, the produced files seem to only be in share/hip
: https://dev.azure.com/ROCm-CI/ROCm-CI/_build/results?buildId=56010&view=logs&j=82fb555c-110a-59d3-1c2a-ac6df3710f95&t=f7e7b015-7ae0-53a0-6899-73e18ee6a991
56df402
to
534bfc7
Compare
Motivation
Builds hip-tests on theRock (closes the first point from #1354)
Technical Details
add hip-tests subproject to the core CMakeLists
Test Plan
Ensure hip-tests builds correctly on CI
Test Result
Submission Checklist