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

LWPDCGSS-2256 - Improve library detection #62

Closed
wants to merge 1 commit into from

Conversation

paubauer
Copy link

Add HINTS to find_dependency calls to ensure find_package(HIP) works without having to set ROCM_PATH or CMAKE_PREFIX_PATH externally.

With this change, having find_package(HIP CONFIG PATHS $ENV{ROCM_PATH} "/opt/rocm") will properly detect the additional libaries. Without it, CMAKE_PREFIX_PATH has to be set to /opt/rocm to be able to configure

Add HINTS to find_dependency calls to ensure find_package(HIP)
works without having to set ROCM_PATH or CMAKE_PREFIX_PATH
@@ -63,7 +63,7 @@ if(NOT HIP_CXX_COMPILER)
endif()

if(NOT WIN32)
find_dependency(AMDDeviceLibs)
find_dependency(AMDDeviceLibs HINTS ${ROCM_PATH})
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be PATHS instead of HINTS?
According to https://cmake.org/cmake/help/latest/command/find_package.html

Copy link
Author

Choose a reason for hiding this comment

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

not sure about this myself, I have discussed this some time with others as well and it wasn't quite clear if it should be PATHS ${ROCM_PATH}, HINTS ${ROCM_PATH} or PATHS ${ROCM_PATH} HINTS /opt/rocm

I'll defer here to reviewer choice :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that (according to cmake doc): HINTS should be paths computed by system introspection, such as a hint provided by the location of another item already found. Hard-coded guesses should be specified with the PATHS option.
I think the correct option here is HINTS ${ROCM_PATH} PATHS "/opt/rocm"

Also a note that with the proposed changes it is not necessary for user to set the ROCM_PATH environment variable. i.e. just provide the path to the rocm installation would also work i.e.

find_package(HIP CONFIG PATHS "/opt/rocm")

I have transferred the changes to an internal PR and it should appear in this repo shortly after it is merged. Will then close this PR.

Copy link
Author

Choose a reason for hiding this comment

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

great, thank you!

@iassiour
Copy link
Contributor

Thank you @paubauer for raising this. The changes have now landed on develop branch with this commit:
eab2038
I am closing this PR.

@iassiour iassiour closed this Jun 21, 2024
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.

2 participants