Back to Halide

Contributing CMake code to Halide

doc/CodeStyleCMake.md

22.0.0.dev020.6 KB
Original Source

Contributing CMake code to Halide

This document specifies the coding standards we adhere to when authoring new CMake code. If you need directions for building Halide, see BuildingHalideWithCMake.md. If you are looking for Halide's CMake package documentation, see HalideCMakePackage.md.

This document is necessary for two major reasons. First, due to its long history, size, and dedication to backwards compatibility, CMake is incredibly difficult to learn and full of traps. Second, Halide bundles its own LLVM-based native code generator, which CMake deeply does not expect. This means we routinely push CMake's build model to its limit.

Therefore, we must be careful to write high-quality CMake code so that it is clear when CMake's limitations are being tested. While not comprehensive, the guide outlines the code quality expectations we have as they apply to CMake.

When contributing new CMake code to Halide, keep in mind that the minimum version is 3.28. Therefore, it is not only possible, but required, to use modern CMake best practices.

<!-- TOC --> <!-- TOC -->

General guidelines and best practices

The following are some common mistakes that lead to subtly broken builds.

  • Reading the build directory. While setting up the build, the build directory should be considered write only. Using the build directory as a read/write temporary directory is acceptable as long as all temp files are cleaned up by the end of configuration.
  • Not using generator expressions. Declarative is better than imperative and this is no exception. Conditionally adding to a target property can leak unwanted details about the build environment into packages. Some information is not accurate or available except via generator expressions, e.g. the build configuration.
  • Using the wrong variable. CMAKE_SOURCE_DIR doesn't always point to the Halide source root. When someone uses Halide via FetchContent, it will point to their source root instead. The correct variable is Halide_SOURCE_DIR. If you want to know if the compiler is MSVC, check it directly with the MSVC variable; don't use WIN32. That will be wrong when compiling with clang on Windows. In most cases, however, a generator expression will be more appropriate.
  • Using directory properties. Directory properties have vexing behavior and are essentially deprecated from CMake 3.0+. Propagating target properties is the way of the future.
  • Using the wrong visibility. Target properties can be PRIVATE, INTERFACE, or both (aka PUBLIC). Pick the most conservative one for each scenario. Refer to the transitive usage requirements docs for more information.
  • Needlessly expanding variables The if and foreach commands generally expand variables when provided by name. Expanding such variables manually can unintentionally change the behavior of the command. Use foreach (item IN LISTS list) instead of foreach (item ${list}). Similarly, use if (varA STREQUAL varB) instead of if ("${varA}" STREQUAL "${varB}") and definitely don't use if (${varA} STREQUAL ${varB}) since that will fail (in the best case) if either variable's value contains a semicolon (due to argument expansion).

Prohibited commands list

As mentioned above, using directory properties is brittle, and they are therefore not allowed. The following functions may not appear in any new CMake code.

CommandAlternative
add_compile_definitionsUse target_compile_definitions
add_compile_optionsUse target_compile_options
add_definitionsUse target_compile_definitions
add_link_optionsUse target_link_options, but prefer not to use either
include_directoriesUse target_include_directories
link_directoriesUse target_link_libraries
link_librariesUse target_link_libraries
remove_definitionsGenerator expressions in target_compile_definitions
set_directory_propertiesUse (cache) variables or target properties
set_property(DIRECTORY)Use (cache) variables or target properties (custom properties excluded, but require justification)
target_link_libraries(target lib)Use target_link_libraries with a visibility specifier (eg. PRIVATE)

As an example, it was once common practice to write code similar to this:

cmake
# WRONG: do not do this
include_directories(include)
add_library(my_lib source1.cpp ..)

However, this has two major pitfalls. First, it applies to all targets created in that directory, even those before the call to include_directories and those created in include()-ed CMake files. As CMake files get larger and more complex, this behavior gets harder to pinpoint. This is particularly vexing when using the link_libraries or add_definitions commands. Second, this form does not provide a way to propagate the include directory to consumers of my_lib. The correct way to do this is:

cmake
# CORRECT
add_library(my_lib source1.cpp ...)
target_sources(
    my_lib
    PUBLIC
    FILE_SET HEADERS
    BASE_DIRS include
    FILES include/header1.h
)

This is better in many ways. It only affects the target in question. It propagates the include path to the targets linking to it (via PUBLIC). It also correctly exports the host-filesystem-specific include path when installing or packaging the target and installs the headers themselves, too.

If common properties need to be grouped together, use an INTERFACE target (better) or write a function (worse).

There are also several functions that are disallowed for other reasons:

CommandReasonAlternative
aux_source_directoryInteracts poorly with incremental builds and GitList source files explicitly
build_commandCTest internal functionUse CTest build-and-test mode via CMAKE_CTEST_COMMAND
cmake_host_system_informationUsually misleading information.Inspect toolchain variables and use generator expressions.
cmake_policy(... OLD)OLD policies are deprecated by definition.Instead, fix the code to work with the new policy.
create_test_sourcelistWe use our own unit testing solutionSee the adding tests section.
define_propertyAdds unnecessary complexityUse a cache variable. Exceptions under special circumstances.
enable_languageHalide is C/C++ onlyFindCUDAToolkit, appropriately guarded.
file(GLOB ...)Interacts poorly with incremental builds and GitList source files explicitly. Allowed if not globbing for source files.
fltk_wrap_uiHalide does not use FLTKNone
include_external_msprojectHalide must remain portableWrite a CMake package config file or find module.
include_guardUse of recursive inclusion is not allowedWrite (recursive) functions.
include_regular_expressionChanges default dependency checking behaviorNone
load_cacheSuperseded by FetchContent/ExternalProjectWrite a vcpkg port or present a case for an exception.
macroCMake macros are not hygienic and are therefore error-proneUse functions instead.
site_namePrivacy: do not want leak host name informationProvide a cache variable, generate a unique name.
variable_watchDebugging helperNone. Not needed in production.

Do not introduce any dependencies via find_package without broader approval. Importantly, never introduce a use of FetchContent. Add dependencies to vcpkg.json or create a custom port.

Prohibited variables list

Any variables that are specific to languages that are not enabled should, of course, be avoided. But of greater concern are variables that are easy to misuse or should not be overridden for our end-users. The following (non-exhaustive) list of variables shall not be used in code merged into main.

VariableReasonAlternative
CMAKE_ROOTCode smellRely on find_package search options; include HINTS if necessary
CMAKE_DEBUG_TARGET_PROPERTIESDebugging helperNone
CMAKE_FIND_DEBUG_MODEDebugging helperNone
CMAKE_RULE_MESSAGESDebugging helperNone
CMAKE_VERBOSE_MAKEFILEDebugging helperNone
CMAKE_BACKWARDS_COMPATIBILITYDeprecatedNone
CMAKE_BUILD_TOOLDeprecated${CMAKE_COMMAND} --build or CMAKE_MAKE_PROGRAM (but see below)
CMAKE_CACHEFILE_DIRDeprecatedCMAKE_BINARY_DIR, but see below
CMAKE_CFG_INTDIRDeprecated$<CONFIG>, $<TARGET_FILE:..>, target resolution of add_custom_command, etc.
CMAKE_CL_64DeprecatedCMAKE_SIZEOF_VOID_P
CMAKE_COMPILER_IS_*DeprecatedCMAKE_<LANG>_COMPILER_ID
CMAKE_HOME_DIRECTORYDeprecatedCMAKE_SOURCE_DIR, but see below
CMAKE_DIRECTORY_LABELSDirectory propertyNone
CMAKE_BUILD_TYPEOnly applies to single-config generators.$<CONFIG>
CMAKE_*_FLAGS* (w/o _INIT)User-onlyWrite a toolchain file with the corresponding _INIT variable
CMAKE_COLOR_MAKEFILEUser-onlyNone
CMAKE_ERROR_DEPRECATEDUser-onlyNone
CMAKE_CONFIGURATION_TYPESWe only support the four standard build typesNone

Of course feel free to insert debugging helpers while developing but please remove them before review. Finally, the following variables are allowed, but their use must be motivated:

VariableReasonAlternative
CMAKE_SOURCE_DIRPoints to global source root, not Halide's.Halide_SOURCE_DIR or PROJECT_SOURCE_DIR
CMAKE_BINARY_DIRPoints to global build root, not Halide'sHalide_BINARY_DIR or PROJECT_BINARY_DIR
CMAKE_MAKE_PROGRAMCMake abstracts over differences in the build tool.Prefer CTest's build and test mode or CMake's --build mode
CMAKE_CROSSCOMPILINGOften misleading.Inspect relevant variables directly, eg. CMAKE_SYSTEM_NAME
BUILD_SHARED_LIBSCould override user settingNone, but be careful to restore value when overriding for a dependency

Any use of these functions or variables will block a PR.

Adding tests

When adding a file to any of the folders under test, be aware that CI expects that every .c and .cpp appears in the CMakeLists.txt file on its own line, possibly as a comment. This is to avoid globbing and also to ensure that added files are not missed.

For most test types, it should be as simple as adding to the existing lists. Generator tests are trickier, but following the existing examples is a safe way to go.

Adding apps

If you're contributing a new app to Halide: great! Thank you! There are a few guidelines you should follow when writing a new app.

  • Write the app as if it were a top-level project. You should call find_package(Halide) and set the C++ version to 11.
  • Call enable_testing() and add a small test that runs the app.
  • Don't assume your app will have access to a GPU. Write your schedules to be robust to varying buildbot hardware.
  • Don't assume your app will be run on a specific OS, architecture, or bitness. Write your apps to be robust (ideally efficient) on all supported platforms.
  • If you rely on any additional packages, don't include them as REQUIRED, instead test to see if their targets are available and, if not, call return() before creating any targets. In this case, print a message(STATUS "[SKIP] ..."), too.
  • Look at the existing apps for examples.
  • Test your app with ctest before opening a PR. Apps are built as part of the test, rather than the main build.