Skip to content
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

Fix own cmake files - also create symlink from INCLUDE cmake #107

Merged
merged 2 commits into from
Nov 20, 2022

Conversation

yhmtsai
Copy link
Contributor

@yhmtsai yhmtsai commented Aug 4, 2022

from 5.2.0, hip and rocm will also add cmake file into <ROCM>/<PACKAGE>/lib/cmake (not just <ROCM>/lib/cmake/<PACAKGE>)
hiprand and rocrand reports include could not find load file: ...
The workaround is link the missing file from <ROCM>/lib/cmake/<PACAKGE> to <ROCM>/<PACKAGE>/lib/cmake or use -D<pacakge_lower_case>_DIR=<ROCM>/<PACKAGE>/lib/cmake in CMake

Issue:
When cmake find the file inside <ROCM>/<PACKAGE>/lib/cmake, it will create issue about cmakefile non-found.
https://github.com/RadeonOpenCompute/rocm-cmake/blob/03ec8374afec9d96afb9cc817fb9258feb84ed32/share/rocm/cmake/ROCMInstallTargets.cmake#L289-L293
only adds the INCLUDE cmake into CONFIG_PACKAGE_INSTALL_DIR, which is lib/cmake/<PACKAGE>
and looks for the file from current cmake folder. ( <ROCM>/<PACKAGE>/lib/cmake)
Thus, it can not find the file because symlink is not created.

also create the symlink for INCLUDE cmakefile.
another way is to change the path to look for the INCLUDE, but I think all cmake should be under the same folder from the rocm cmakefile structure

Copy link
Collaborator

@lawruble13 lawruble13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing out the issue and suggesting a fix!

If we want to go this route to fix the problem, the noted changes should be made.

I'm of the opinion that a better route may be to resolve CMAKE_CURRENT_LIST_DIR using get_filename_component(... REALPATH) in the configuration file, although I'm not certain and will do some experimenting with the alternatives.

share/rocm/cmake/ROCMInstallTargets.cmake Outdated Show resolved Hide resolved
share/rocm/cmake/ROCMInstallTargets.cmake Outdated Show resolved Hide resolved
Co-authored-by: Liam Wrubleski <Liam.Wrubleski@amd.com>
@yhmtsai
Copy link
Contributor Author

yhmtsai commented Aug 5, 2022

@lawruble13 Thanks for suggestion.
Another way might glob all *.cmake from the source dir.
I am sure whether it will lead the circular issue I posted before again.
Also, sorry for no further reply in posted issue. I missed the notification

@lawruble13 lawruble13 self-requested a review August 18, 2022 19:30
Copy link
Collaborator

@lawruble13 lawruble13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, having tested & considered I think that this solution is fine, especially as this segment of code is to support backwards compatibility and will be removed eventually.

Copy link
Collaborator

@cgmb cgmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is probably going to end up in ROCm 5.5, so it might only be a single release before it's made irrelevant by the removal of the compatibility layout. Nevertheless... pushing the fix forward by a few months is still nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants