Check for CUDA headers rather than compiler#28
Check for CUDA headers rather than compiler#28sethrj wants to merge 8 commits intoroot-project:masterfrom
Conversation
|
@amadio I see the following message: is there something I need to do? If it's because of the move to @root-project, then perhaps a CONTRIBUTING.md describing PR requirements would help. Thanks! 
|
This is just a message about commits requiring a GPG signature, I don't think you need to do anything. I will handle the merge when things are ready. The |
| # Print a pretty message if the caller doesn't have a FindVecGeom.cmake | ||
| include(FindPackageHandleStandardArgs) | ||
| set(${CMAKE_FIND_PACKAGE_NAME}_CONFIG "${CMAKE_CURRENT_LIST_FILE}") | ||
| find_package_handle_standard_args(@PROJECT_NAME@ CONFIG_MODE HANDLE_COMPONENTS) |
There was a problem hiding this comment.
It's probably better to hard-code VecCore in these lines, to ensure things still work when VecCore is used inside another project with add_subdirectory(). However, with ROOT deprecating VecCore, if this is fine for VecGeom/Celeritas, it can stay like this.
There was a problem hiding this comment.
PROJECT_NAME is the "local" project name, not the top-level CMAKE_PROJECT_NAME, so it will work as expected. But I can change it for clarity.
| set(VecCore_CUDA_FOUND True) | ||
| set(VecCore_CUDA_DEFINITIONS -DVECCORE_ENABLE_CUDA) | ||
| set(VecCore_CUDA_INCLUDE_DIR ${CMAKE_CUDA_TOOLKIT_INCLUDE_DIRECTORIES}) | ||
| set(VecCore_CUDA_INCLUDE_DIR ${CUDAToolkit_INCLUDE_DIRS}) |
There was a problem hiding this comment.
Could you please explain the motivation for this change? The current way of doing things follows advice from CMake policy CMP0146, which suggests projects should use CMake's first class support for cuda via check_language(CUDA)/enable_language(CUDA). The addition of -DVECCORE_ENABLE_CUDA does require that the CUDA language be enabled if you are compiling CUDA code, because that will enable host/device macros, etc. If you want to handle that yourself, you can always find_package(VecCore) without specifying the CUDA component, then add_definitions(VECCORE_ENABLE_CUDA) yourself, without having to change this logic.
There was a problem hiding this comment.
For the record, this was last modified in this commit 743566f.
There was a problem hiding this comment.
The article you link is about CUDA compilation, and CUDAToolkit is about locating the libraries. Since VecCore doesn't build any CUDA files at all, it doesn't need to enable the full language: linking against CUDA::cudart (or including CUDAToolkit_INCLUDE_DIRS) lets cuda_runtime.h and the associated macros to be found by C++ compilers.
|
@sethrj Do you still plan to work on this? Or shall I close? |
|
Apologies @amadio , this fell off my radar. Let me address your comments. Thanks! |
|
Hi @amadio , I think I've addressed all your feedback. Could you take another look? Thanks! |
|
Please submit fixes as amends to the original commits, we don't want to have broken intermediate history in the git log. Or, I can squash the commits when merging. |
|
The CI seems to have become broken, it's not been changed in a long time. I will check this locally and fix the CI when I have the time... |
|
Squashing would be perfect, thanks! |
2 participants
Since VecCore is header-only, it needs only to check the CUDA toolkit location. This also avoids some odd side effects that occur with
check_language(which can interfere with variables inenable_language). Additionally, use standard reporting for the library being found when loaded viafind_package, and update the cmake to avoid warnings with newer versions.