-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Migrate to FindCUDAToolkit.cmake #3004
Conversation
The stuff like But yeah, this would be awesome to update to assuming we don't find it breaks anything. Updating cmake is often an odyssey :| |
I just tried this on an
errors out with this:
This is with cmake |
None of the github CI builds have cuda machines so you have to test this yourself :( |
Thanks for trying it out under Linux. I will try to spin up a VM and try to fix the CUDNN finder (I think the pkgconfig is missing).
Sure, those get also re-added. |
This code is much closer to the code in master. The cudnn finder is again the same as the master, so your error should not occur anymore (I wasn't able to set up a ubuntu 18.04 vm with cuda yet). It mostly replaced CUDA with CUDAToolkit and enables CUDA as a language. Sadly, currently only tested on windows. |
fd99641
to
9da5c52
Compare
Tried it on ubuntu and got this:
|
Thanks for retrying it. I think only the cuda runtime was missing on the cudnn test target. I installed cuda/cudnn on a non nvidia Linux machine, but it failed to enable CUDA in cmake, because it couldn't figure out the architecture version (because, no nvidia gpu, I assume?). Sadly, I couldn't trick cmake to compile anyway and reach the cudnn test part. I really hope it works this time, otherwise I have to figure out something else. I feel really bad that you have to try it so often. If it fails now, I will not work on it until next weekend, because I will be gone during the next week. |
Got it on Debian 12 with CUDA 12/cudnn 9 and cmake 3.25.1 via WSL Had to set Output works:
|
Did you build it? Like do I just tried that and the build failed due to a bunch of missing symbol errors. I.e. |
Sadly yes, I was able to link and run it successfully. Here a fresh build with only CUDAToolkit_ROOT defined
Did you also try out a fresh build/clear directory? Cleared CMake Cache? Removed |
Huh yeah, must have forgotten to blow away the old cmake build files. Looks like it's working. I'll look this over more in a bit :D |
list (APPEND dlib_needed_private_libraries CUDA::toolkit) | ||
list (APPEND dlib_needed_private_libraries CUDA::cublas) | ||
list (APPEND dlib_needed_private_libraries ${cudnn}) | ||
list (APPEND dlib_needed_private_libraries ${CUDA_curand_LIBRARY}) | ||
list (APPEND dlib_needed_private_libraries ${CUDA_cusolver_LIBRARY}) | ||
list (APPEND dlib_needed_private_libraries ${CUDA_CUDART_LIBRARY}) | ||
list (APPEND dlib_needed_private_libraries CUDA::curand) | ||
list (APPEND dlib_needed_private_libraries CUDA::cusolver) | ||
list (APPEND dlib_needed_private_libraries CUDA::cudart) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work when generating pkgconfig files.
If you look at my old PR #2912, which included using FindCUDAToolkit.cmake, I was unable to properly interoperate with pkgconfig due to cmake's use of "blah::blah" when linking libraries. This was my final hurdle but sadly couldn't overcome it. It included a whole bunch of other tidy-ups too... Sad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no, this sounds bad. I wasn't reading all of the PR, because it is way too long. But I assume you reference this comment [1] of yours.
You suggest using the "old" cmake way of using XXX_LIBRARIES and XXX_INCLUDE_DIRS.
Given the result variables from the new finder [2], the major problem seems to be the XXX_LIBRARIES variable, which is not set. I was also browsing the cmake source code of the finder and there isn't even a private one.
Did you try out to get all the link libraries via a target property? [3] Maybe creating the XXX_LIBRARIES manually by extracting all the libraries from the LINK_LIBRARIES [4] property is fine.
[1] #2912 (comment)
[2] https://cmake.org/cmake/help/latest/module/FindCUDAToolkit.html#result-variables
[3] get_target_property: https://cmake.org/cmake/help/latest/command/get_target_property.html
[4] LINK_LIBRARIES: https://cmake.org/cmake/help/latest/prop_tgt/LINK_LIBRARIES.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried extracting all the link libraries but it isn't trivial. There are some old cmake libraries that attempt to do this but they're not maintained. Personally my preference would be to drop support for pkgconfig then this problem goes away. There is a ticket open https://gitlab.kitware.com/cmake/cmake/-/issues/22621 that discusses adding support for exporting pkgconfig files natively. But nothing's happened. Realistically, how many users are using dlib via pkgconfig? My guess is not very many.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue with the link libraries is that you have to do it recursively for all dependencies. It can get a bit nasty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this sounds at least like a solution if pkgconfig cannot be dropped.
Long-term wise, a solution is probably needed anyway, as CMake linking on targets will very likely stay.
Notice: this issue has been closed because it has been inactive for 51 days. You may reopen this issue if it has been closed in error. |
This PR migrates the current implementation from find_package(CUDA) to find_package(CUDAToolkit) .
It removes the old path, most notably the test compilation with CUDA. Here, I am not sure if it follows the project practices, but I am personally not the biggest fan of those test compilation, this is why I omitted them. Please let me know if I should re-add them.
It also adds the OpenMP finder of CMake.
Backward incompatibilities are the required CMake version. This PR changes the note for the current release, which raised the CMake version already to 3.8.0.
This PR is based on and closes #2833 and closely the suggested implementation with some tweaks.
Edit: tested it locally under Windows.