Leverage CMake 4.2 introduction of UNWIND_INCLUDE for find_package#888
Leverage CMake 4.2 introduction of UNWIND_INCLUDE for find_package#888robertmaynard wants to merge 3 commits intorapidsai:mainfrom
find_package#888Conversation
This allows rapids-cmake starting in 4.2 to correctly generate CMake config modules that gracefully error out instead of silently returning and producing weird downstream errors. These weird errors are generally around missing targets.
robertmaynard
added
feature request
|
|
||
| find_dependency(@name@) | ||
| if(CMAKE_VERSION VERSION_GREATER_EQUAL 4.2.0) | ||
| __find_dependency_no_return(@name@) |
There was a problem hiding this comment.
I'm hesitant to sign off on using an undocumented command like this. How much assurance do we have that its interface won't change? Do we at least have a way to test that it's still working properly?
There was a problem hiding this comment.
It's now used by CMake's own EXPORT_PACKAGE_DEPENENCIES, so its no less stable than that.
But EXPORT_PACKAGE_DEPENDENCIES is still gated, so might change. I think it's reasonable for us to promote this to a non-underscore name within CMake.
There was a problem hiding this comment.
It might be reasonable to accept this while 4.2.0 is still in the oven (4.1.0 is still in rc status), so it can get some "real-world" testing, and re-visit prior to 4.2.0 full release.
As I'm not sure what __find_dependency_no_return's final form will be just yet.
There was a problem hiding this comment.
It's now used by CMake's own
EXPORT_PACKAGE_DEPENENCIES, so its no less stable than that.
I disagree. I worked on CMake long enough to know that even if the interface to EXPORT_PACKAGE_DEPENDENCIES is stable, the underlying implementation details (which include this function) are not. If we're going to use an undocumented function in this manner, I want assurance that we have some sort of unit test that will tell us when the functionality is broken because the interface to the internal API changed.
There was a problem hiding this comment.
It might be reasonable to accept this while 4.2.0 is still in the oven (4.1.0 is still in rc status), so it can get some "real-world" testing, and re-visit prior to 4.2.0 full release.
Unfortunately, this won't work. The master branch of CMake has version number 4.1.20250804, and won't roll over to 4.2 until we get to 4.2.0-rc1. If we want real-world testing before 4.2, we need to change the version number to 4.1.20250804 (or whatever nightly build introduced the internal function that we're using.)
There was a problem hiding this comment.
Our other option is to use find_package only in our generated code. We would use ${CMAKE_FIND_PACKAGE_NAME}_FIND_REQUIRED and ${CMAKE_FIND_PACKAGE_NAME}_FIND_QUIETLY to match the behavior of find_dependency but wouldn't tie us to this undocumented function
There was a problem hiding this comment.
That sounds reasonable to me.
There was a problem hiding this comment.
I disagree. I worked on CMake long enough to know that even if the interface to
EXPORT_PACKAGE_DEPENDENCIESis stable, the underlying implementation details (which include this function) are not.
Because this function is exposed in generated export files that get shipped in downstream packages, we have an obligation to maintain whatever interface gets shipped once EXPORT_PACKAGE_DEPENDENCIES gets out of experimental gate. If that happened today it would be __cmake_find_dependencies_no_return. I'm just not sure what it will be when the gate is actually removed.
3 participants
Description
This allows rapids-cmake starting in 4.2 to correctly generate CMake config modules that gracefully error out instead of silently returning and producing weird downstream errors. These weird errors are generally around missing targets.
Checklist