Rename CMake package to ROCmCMakeBuildTools #128
Conversation
There was a problem hiding this comment.
Why did the build fail before the tests were changed? Our tests passing without requiring any changes would be proof that this is being done in a backwards-compatible manner. We don't have to release (or even merge) like that, but it would be nice to see it pass once without test changes, if possible.
|
The tests failed prior to switching the package name due to the |
There was a problem hiding this comment.
I can see in the description of this PR: "No description provided." which is not very inclusive for an open-source project. :-)
Any rationale for this PR?
At least having the information from the CHANGELOG.md could be a good start.
Feel free to put some comments in the CMake files themselves too to help future readers/users.
saadrahim
changed the title
lawruble13
changed the base branch from
release-staging/rocm-rel-5.7
to
develop
@lawruble13 can you address this request? |
In order to have as smooth a transition as possible, starting this as a notice, will make into a deprecation/warning in ~6.2, error sometime later, remove in 7.0.
|
I changed the |
share/rocm/cmake/ROCMConfig.cmake
Outdated
| @@ -2,7 +2,7 @@ | |||
| # Copyright (C) 2023 Advanced Micro Devices, Inc. | |||
| # ###################################################################################################################### | |||
|
|
|||
| message(DEPRECATION | |||
| message(NOTICE | |||
| "Use of find_package(ROCM) is deprecated, please switch to find_package(ROCmCMakeBuildTools)." | |||
There was a problem hiding this comment.
Then, you probably need to change the message to something like
"Use of find_package(ROCM) will be deprecated soon, please switch to find_package(ROCmCMakeBuildTools)."
There was a problem hiding this comment.
I changed the
DEPRECATIONto aNOTICEso that we can start letting people know about this before we start to throw warnings/errors at them depending on their settings.
The NOTICE category wasn't added until CMake 3.15. You could use STATUS, but I would suggest leaving it as a DEPRECATION. Users can set CMAKE_ERROR_DEPRECATED or CMAKE_WARN_DEPRECATED to choose how they want deprecations to be treated. If you don't want to deprecate it yet, another option would be to omit the message entirely.
The updated deprecation message is better than before, but I think you would benefit from reviewing deprecation messages from project like rust, libstdc++ or Qt so that you can match their style and tone.
lawruble13
changed the base branch from
develop
to
release-staging/rocm-rel-6.0
lawruble13
merged commit df4df6d
into
ROCm:release-staging/rocm-rel-6.0
Having the name of this CMake package be ROCM is consistently confusing to users who expect find_package(ROCM [...]) to add ROCm packages to their build, instead of tools that we use to build ROCm (see #49). Renaming the CMake project to ROCmCMakeBuildTools makes the purpose of this package clearer. In order to have as smooth a transition as possible, starting this as a notice, will make into a deprecation/warning in ~6.2, error sometime later, remove in 7.0.
For discussion, see: Rename CMake package to ROCmCMakeBuildTools (#128). Having the name of this CMake package be ROCM is consistently confusing to users who expect find_package(ROCM [...]) to add ROCm packages to their build, instead of tools that we use to build ROCm (see #49). Renaming the CMake project to ROCmCMakeBuildTools makes the purpose of this package clearer. In order to have as smooth a transition as possible, starting this as a notice, will make into a deprecation/warning in ~6.2, error sometime later, remove in 7.0.
For discussion, see: Rename CMake package to ROCmCMakeBuildTools (ROCm#128). Having the name of this CMake package be ROCM is consistently confusing to users who expect find_package(ROCM [...]) to add ROCm packages to their build, instead of tools that we use to build ROCm (see ROCm#49). Renaming the CMake project to ROCmCMakeBuildTools makes the purpose of this package clearer. In order to have as smooth a transition as possible, starting this as a notice, will make into a deprecation/warning in ~6.2, error sometime later, remove in 7.0.
For discussion, see: Rename CMake package to ROCmCMakeBuildTools (#128). Having the name of this CMake package be ROCM is consistently confusing to users who expect find_package(ROCM [...]) to add ROCm packages to their build, instead of tools that we use to build ROCm (see #49). Renaming the CMake project to ROCmCMakeBuildTools makes the purpose of this package clearer. In order to have as smooth a transition as possible, starting this as a notice, will make into a deprecation/warning in ~6.2, error sometime later, remove in 7.0.
Having the name of this CMake package be
ROCMis consistently confusing to users who expectfind_package(ROCM [...])to add ROCm packages to their build, instead of tools that we use to build ROCm (see #49). Renaming the CMake project to ROCmCMakeBuildTools makes the purpose of this package clearer.