Check for CUDA headers rather than compiler

[sethrj]

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 in enable_language). Additionally, use standard reporting for the library being found when loaded via find_package, and update the cmake to avoid warnings with newer versions.

[sethrj]

@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!

[amadio]

@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 master branch is protected, though, pushing to it directly is restricted. I need to investigate what happened that the CI failed, though. Please make sure your branch is rebased on top of GitHub's tip of master and force-push, as the failure was to check out the commit before your changes, which should not have failed.

[amadio]

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.

[amadio]

For the record, this was last modified in this commit 743566f.

[sethrj]

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.

[amadio]

I want to understand what concrete problem this change is solving on the VecGeom side. Why is enable_language(CUDA) causing an issue there? Why my suggestion above to not add CUDA to components doesn't work in case you want to handle it differently?

[sethrj]

The concrete problem is that check_language(CUDA) has weird side effects on CMAKE_CUDA_HOST_COMPILER and other cuda-related variables: doing check_language before enable_language results in a different cmake state than simply doing enable_language. The toolkit headers in contrast behave more like a normal find_package.

[amadio]

@sethrj Do you still plan to work on this? Or shall I close?

[sethrj]

Apologies @amadio , this fell off my radar. Let me address your comments. Thanks!

[sethrj]

Hi @amadio , I think I've addressed all your feedback. Could you take another look? Thanks!

[amadio]

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.

[amadio]

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...

[sethrj]

Squashing would be perfect, thanks!

[amadio]

@sethrj Hi, sorry for the long delay with VecCore stuff. It's hard to find time for it. However, I played with Claude a bit over last weekend, and after that I fixed the CI. Please rebase your changes on the latest master and push again here, the builds should work this time. Cheers,

[amadio]

There seems to be something wrong with your fork, please make sure master there is in sync with the current repository. I see "failed to checkout base reference" in the build failures above, but on my own fork it works fine. Please go over my review comments, I see there's still some typos, like using VecGeom instead of VecCore in some places. If you could explain what motivates the changes (i.e. what is failing in VecGeom with the current version), then I can try locally to reproduce and propose an alternative fix or verify what's in the pull request later, since CUDA is not tested in the CI, as it's a pain to setup in GitHub Actions (and there's no GPU to run the tests either).

[amadio]

Please rebase and fixup the commits, I got tricked by the intermediate commits, which mention VecGeom, but later commits fixed that up. I'd like the git commit history to be clean from these temporary things. Thanks!