Cmake modernization #102
Cmake modernization #102
Conversation
- fix of generator expression syntax - `INTERFACE` should only be used with header only libraries
See https://cmake.org/cmake/help/v3.12/command/export.html """ The file created by this command is specific to the build tree and should never be installed. See the install(EXPORT) command to export targets from an installation tree. """
See https://cmake.org/cmake/help/v3.12/command/install.html#command:install """ Note that the values specified for this option only apply to options listed AFTER the CONFIGURATIONS """
This enables users to use the `find_package(cuda-api-wrappers)` function in their `CMakeLists.txt`.
It is good practice to give the export set a different name (in comparison to the project and target names).
This enables the external use without the installation of the project. Thus in an external project one can use ``` find_package(cuda-api-wrappers) ``` because CMake finds the package in the local CMake package registry (often `~/.cmake/packages`)
CMakeLists.txt
Outdated
| @@ -150,19 +157,38 @@ add_dependencies(examples examples_by_runtime_api_module modified_cuda_samples) | |||
|
|
|||
| install( | |||
| TARGETS cuda-api-wrappers | |||
| EXPORT cuda-api-wrappers | |||
| EXPORT cuda-api-wrappersExport | |||
There was a problem hiding this comment.
is there a particular reason you're using "Export" rather than _export as a suffix? cuda-api-wrappers_export makes sense to me, or CudaApiWrappersExport, or CUDAAPIWrappersExport but not this particular choice.
There was a problem hiding this comment.
No there is no particular reason. I just think it is reasonable to give the export not the name of a target or the project.
| DESTINATION "share/cmake/${PROJECT_NAME}" | ||
| NAMESPACE "${PROJECT_NAME}::" | ||
| # In this CMake config file no dependencies are considered. But since we | ||
| # do not use any `find_package` call here this approach is sufficient. |
There was a problem hiding this comment.
What about the CUDA dependency? Can we ignore that?
There was a problem hiding this comment.
Right now, everything works fine if you use the CUDA compiler nvcc from the CUDA toolkit. In future it might be more relevant to distinguish between a CUDA compiler and the CUDA libraries, like the runtime libraries or CUBLAS etc. I think it is right now not worth it to write CMake find modules by ourselves for the CUDA libraries. Moreover this problem is not introduced by my PR, but already exists in basically every project with CMake and CUDA libraries.
|
|
||
| export( | ||
| EXPORT ${PROJECT_NAME}Export | ||
| NAMESPACE "${PROJECT_NAME}::" |
There was a problem hiding this comment.
So what's actually going to be inside that namespace?
There was a problem hiding this comment.
Every target, which has been added at the installation to the export set. So right now only cuda-api-wrappers.
| NAMESPACE "${PROJECT_NAME}::" | ||
| # In this CMake config file no dependencies are considered. But since we | ||
| # do not use any `find_package` call here this approach is sufficient. | ||
| FILE "${PROJECT_BINARY_DIR}/${PROJECT_NAME}Config.cmake" |
There was a problem hiding this comment.
Again, shouldn't this be either cuda-api-wrappers_config or CudaApiWrappersConfig.cmake?
There was a problem hiding this comment.
How do you want the user to add the dependency to this project. I think the most intuitive way would be (this is how it is working right now):
find_package(cuda-api-wrappers)
...
target_link_libraries(
foo
cuda-api-wrappers::cuda-api-wrappers
)
If you name this file CudaApiWrappersConfig.cmake it would be
find_package(CudaApiWrappers)
...
target_link_libraries(
foo
cuda-api-wrappers::cuda-api-wrappers
)
and if you name this file cuda-api-wrappers_config it is useless, because CMake expects -config or Config suffix.
Rename export set
Main features:
cuda-api-wrappers)cuda-api-wrappersis used by other CMake projects andcuda-api-wrapperswas installed somewhere.See
This enhanced my personal work with CMake a lot.
I do not care if you merge all the commits into one commit. I just did it that way to keep the changes understandable.
#60 (comment)
So here is a smaller PR.