-
Notifications
You must be signed in to change notification settings - Fork 0
Description
Here's a quick breakdown of some CMake best practices
Add upper-bound to CMake policies
cmake_minimum_required(VERSION 3.18) |
Should be
cmake_minimum_required(VERSION 3.18...4.0)
The upper-bound is typically the latest version of the CMake you have tested. For typical pip installs it's always the latest non-rc version. (It does not mean only up to CMake 4.0 can be used, it's just a setting to setup the backwards-compatibility behaviors)
CMAKE_BUILD_TYPE
scikit-build-pybind11-template/CMakeLists.txt
Lines 10 to 14 in 64e995f
# Use Release builds by default for better performance | |
if(NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES) | |
set(CMAKE_BUILD_TYPE "Release" CACHE STRING "Build type" FORCE) | |
set_property(CACHE CMAKE_BUILD_TYPE PROPERTY STRINGS "Debug" "Release" "MinSizeRel" "RelWithDebInfo") | |
endif() |
This one should be moved before project
(don't remember the reasoning tbh), but the logic there is generally superfluous. You can simplify it to:
set(CMAKE_BUILD_TYPE "Release" CACHE STRING "Build type")
CMAKE_EXPORT_COMPILE_COMMANDS
is not useful
scikit-build-pybind11-template/CMakeLists.txt
Lines 16 to 17 in 64e995f
# Enable faster incremental builds | |
set(CMAKE_EXPORT_COMPILE_COMMANDS ON) |
This thing is only useful if you have a tool that it is reading it, and in those cases, the tools will generally inject it anyways.
ccache
usage rant
scikit-build-pybind11-template/CMakeLists.txt
Lines 19 to 25 in 64e995f
# Use ccache if available for faster rebuilds | |
find_program(CCACHE_PROGRAM ccache) | |
if(CCACHE_PROGRAM) | |
set(CMAKE_CXX_COMPILER_LAUNCHER "${CCACHE_PROGRAM}") | |
set(CMAKE_C_COMPILER_LAUNCHER "${CCACHE_PROGRAM}") | |
message(STATUS "Using ccache: ${CCACHE_PROGRAM}") | |
endif() |
This makes it always be used if available, which might not be desired for users. Generally I would say let the user/dev/CI decide on what to use and inject the CMAKE_*_COMPILER_LAUNCHER
needed. Another pattern is to gate it by an option()
Make use of generator expressions
E.g.:
scikit-build-pybind11-template/CMakeLists.txt
Lines 41 to 46 in 64e995f
if(MSVC) | |
# Windows: Enable parallel compilation and faster linking | |
target_compile_options(hello_pybind11 PRIVATE | |
/MP # Multi-processor compilation | |
/bigobj # Large object files | |
) |
can be changed to
target_compile_options(hello_pybind11 PRIVATE
$<$<CXX_COMPILER_ID:MSVC>:/MP> # Multi-processor compilation
$<$<CXX_COMPILER_ID:MSVC>:/bigobj> # Large object files
)
Reference: https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html
Compiler flags
Some are already provided by CMake itself, e.g.:
scikit-build-pybind11-template/CMakeLists.txt
Lines 57 to 58 in 64e995f
$<$<CONFIG:Debug>:-g -O0> | |
$<$<CONFIG:Release>:-O3 -DNDEBUG> |
Is in the
CMAKE_<LANG>_FLAGS_<CONFIG>
cache variable
Please do not add -Wall
to your projects by default. Put it in your CI, e.g. via CXXFLAGS
environment variable or CMake presets. These are only meaningful to the developer, not the user/packager.
New way of specifying the linker tool
scikit-build-pybind11-template/CMakeLists.txt
Lines 64 to 66 in 64e995f
if(LLD_PROGRAM) | |
target_link_options(hello_pybind11 PRIVATE -fuse-ld=lld) | |
endif() |
I have not used it much, but it is now supported directly by CMake with CMAKE_<LANG>_COMPILER_LINKER
. In gneral though, you should not hard-code it, because the user might not have lld
actually, just the compiler, or they might want to use yet another linker.
Be careful how you handle RPATH
INSTALL_RPATH_USE_LINK_PATH ON |
RPATHs are very tricky subject to get it right, and for pyprojects, it is generally not what you want. Generally ignore these until you find that you actually need them. We do want to support them better out-of-the-box with scikit-build-core so that you don't need to manually define these, it would be better to keep an eye out on that feature and help us move forward with it.
Do not install directly in site-packages
This is a more scikit-build-core
thing, but
scikit-build-pybind11-template/CMakeLists.txt
Lines 89 to 90 in 64e995f
# Install target | |
install(TARGETS hello_pybind11 DESTINATION .) |
installs directly in
site-packages
, i.e. it installs site-packages/hello_pybind11.so
. This is generally frowned upon because it can easily lead to file conflicts as the project grows. Instead you should have a site-packages/hello_pybind11/__init__.py
and import submodules from there.
Overall though, it looks like a good start, these are mostly just nitpicks that I would like the community to adapt to as early as possible.