CI: Avoid recompiling hipFile in later stages of CI.#148
Conversation
riley-dixon
changed the title
riley-dixon
force-pushed
the
rildixon/ci-system-tests-use-artifacts
branch
5 times, most recently
from
6d4ca8d to
f109417
Compare
riley-dixon
requested review from
derobins,
gaoikawa,
jordan-turbofish and
kurtmcmillan
as code owners
riley-dixon
force-pushed
the
rildixon/ci-system-tests-use-artifacts
branch
from
f109417 to
84a0174
Compare
riley-dixon
changed the title
Self-hosted runners apparently do not automatically perform cleanup steps of the GITHUB_WORKSPACE at the end of the job.
The goal is to avoid redundant compile steps for executing the hipFile system tests. It is not trivial to configure CTest in CMake to run a GTest executable that has been moved outside of the build tree. The purpose of such a configuration is to have an installable test package to avoid needing to recompile hipFile for running system tests. For now, we will just reuse the build directory. This is for simplicity rather than just copying over a subset of the build directory. Notably: 1) Copying the CTestTestfiles.cmake in the root of the build directory. 2) The test subdirectory comprises the bulk of the build directory's overall size.
FIO will need the development package to build with hipFile support. The hipFile GTest executables are TBD.
This is to assist with debugging to ensure the right install paths are being used. dpkg does not like having -I and -c being provided at the same time.
riley-dixon
force-pushed
the
rildixon/ci-system-tests-use-artifacts
branch
from
84a0174 to
2d4dd72
Compare
jordan-turbofish
left a comment
jordan-turbofish
left a comment
There was a problem hiding this comment.
A couple comments.
| # GitHub Build Artifacts do not preserve file permissions, namely the executable bit. | ||
| - name: Fix file permissions on GTest executables | ||
| run: | | ||
| chmod a+x \ | ||
| ${GITHUB_WORKSPACE}/hipfile-build-dir/test/hipfile_system_tests \ | ||
| ${GITHUB_WORKSPACE}/hipfile-build-dir/test/amd_detail/batch_mt \ | ||
| ${GITHUB_WORKSPACE}/hipfile-build-dir/test/amd_detail/state_mt \ | ||
| ${GITHUB_WORKSPACE}/hipfile-build-dir/examples/aiscp/aiscp |
There was a problem hiding this comment.
I know that it would be an archive inside of an archive, but would it be worth creating a tar archive for the build directory, so you don't have this list to maintain?
There was a problem hiding this comment.
If this list changes frequently then yes I think that would be worthwhile until we can figure out how to produce a "test" package compatible with CTest. Not too worried about it at this time though.
| # Set hipFile Install Path to the ROCm directory | ||
| # Note: CMAKE_INSTALL_PREFIX is set to a default by project(). | ||
| if(CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT) | ||
| set(CMAKE_INSTALL_PREFIX "${ROCM_PATH}" CACHE PATH "The path where hipFile should be installed" FORCE) | ||
| endif() | ||
|
|
||
| # Fix library install directory to "lib" to be inline with the rest of ROCm | ||
| # Note: If testing is enabled, installing GTest calls GNUInstallDirs which will | ||
| # set this before ROCMInstallTargets can set it. | ||
| set(CMAKE_INSTALL_LIBDIR "lib" CACHE STRING "Directory name for installed ROCm libraries") | ||
|
|
There was a problem hiding this comment.
Would it be worth setting CMAKE_INSTALL_PREFIX on the configuration instead of changing the default? Also, when I test this, the ROCM_PATH is set to /opt/rocm, which is a symlink to the path of the versioned rocm directory. I think the package should be installing to the real path.
There was a problem hiding this comment.
In TheRock, ROCM_PATH (or more typically ROCM_DIR) is set by a driver build script that gets passed into CMake - which eventually hits a block like above. So, I think the thing that needs to change instead is how ROCM_PATH gets set. Right now, our CI isn't setup to be configurable on a variable ROCm version.
I think the package should be installing to the real path.
I agree but point back to the limitation that currently exists in our CI.
Depends on #148 Largely a refactoring PR, this breaks down our monolithic workflow. It also consolidates a few steps and a few variables that have notably been copy & pasted. AIROCFILE-97
3 participants
Replaces #141.
This PR removes redundantly compiling hipFile during later stages in the CI workflow, namely for system tests.
Where possible, the installed hipFile library from the DEB/RPM package should be used. This is already the case for FIO. However, for system tests executed via CTest we currently need to reuse the entire build directory. Removing this build-time dependency would be ideal, but is out of scope for this PR.
This PR also sets the hipFile package install path to the non-versioned ROCm directory (
/opt/rocm).AIROCFILE-97