[REVIEW] CMake target cleanup, formatting, linting #604
Conversation
|
Please update the changelog in order to start CI tests. View the gpuCI docs here. |
There was a problem hiding this comment.
Overall I think it's a good thing to clean this up and add standard formatting.
One general comment is that it'd have been nice to have the formatting changes separate from actual changes (targets, moving things around), etc, as it's difficult to review in this form.
Other than that I just have a couple of small comments.
|
|
||
| if(CUDA_STATIC_RUNTIME) | ||
| message(STATUS "Enabling static linking of cudart") | ||
| message(STATUS "RMM: Enabling static linking of cudart") |
There was a problem hiding this comment.
Is this option even needed anymore? RMM doesn't produce a binary object anymore, so if someone wants to statically link against cudart, they can do that when compiling their binary.
There was a problem hiding this comment.
If we don't have this then the CUDA dependency isn't properly enforced on the RMM target and it becomes the downstream project's responsibility to add it which is problematic.
There was a problem hiding this comment.
Even though the library is header-only, it still needs to know what libraries to link against when it's being used -- either in the tests or in a downstream project. The choice of static vs dynamic runtime does get recorded together with the target, and picked up downstream.
Maybe another question is whether the static option is still used / needed anywhere in the first place?
There was a problem hiding this comment.
You typically want to static link the CUDA libraries if you're not in a conda controlled environment. This allows shipping your application / binary without the user needing the CUDA toolkit installed.
kkraus14
added
the
5 - Merge After Dependencies
kkraus14
removed
the
5 - Merge After Dependencies
kkraus14
changed the title
kkraus14
added
3 - Ready for review
|
This should be ready for review now, thanks! |
There was a cmake formatting change in #604 and the ci script that bumps versions at release time was not updated to account for it properly. This fixes the script. Authors: - Keith Kraus <[email protected]> Approvers: - Ray Douglass URL: #639
Sets up framework for automating formatting + linting of CMake files via pre-commit, and fixes all known formatting + linting issues.
Additionally, moves compiler / linker / etc. options to the RMM target instead of using CMake global options.
Depends on: rapidsai/integration#156