Skip to content

Commit e58aa4e

Browse files
authored
pre-commit: check CMake sources for style guide violations (#9020)
1 parent f700a71 commit e58aa4e

8 files changed

Lines changed: 148 additions & 33 deletions

.pre-commit-config.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,12 @@ repos:
8484
language: system
8585
files: ^test/.*CMakeLists\.txt$
8686

87+
- id: check-cmake-style
88+
name: check CMake style
89+
entry: python tools/check_cmake_style.py
90+
language: system
91+
files: (^|/)(CMakeLists\.txt|.*\.cmake)$
92+
8793
- id: uv-lock
8894
name: check uv.lock is up to date
8995
entry: uv lock --check

apps/hannk/CMakeLists.txt

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,10 @@ set(TFLITE_VERSION "${TFLITE_VERSION_MAJOR}.${TFLITE_VERSION_MINOR}.${TFLITE_VER
3535

3636
# ----------------------------
3737

38-
add_compile_definitions(TFLITE_VERSION_MAJOR=${TFLITE_VERSION_MAJOR})
39-
add_compile_definitions(TFLITE_VERSION_MINOR=${TFLITE_VERSION_MINOR})
40-
add_compile_definitions(TFLITE_VERSION_PATCH=${TFLITE_VERSION_PATCH})
41-
add_compile_definitions(HANNK_BUILD_TFLITE=$<BOOL:${HANNK_BUILD_TFLITE}>)
38+
add_compile_definitions(TFLITE_VERSION_MAJOR=${TFLITE_VERSION_MAJOR}) # nolint -- TODO: move to target_compile_definitions
39+
add_compile_definitions(TFLITE_VERSION_MINOR=${TFLITE_VERSION_MINOR}) # nolint
40+
add_compile_definitions(TFLITE_VERSION_PATCH=${TFLITE_VERSION_PATCH}) # nolint
41+
add_compile_definitions(HANNK_BUILD_TFLITE=$<BOOL:${HANNK_BUILD_TFLITE}>) # nolint
4242

4343
# ----------------------------
4444

cmake/FindHalide_LLVM.cmake

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -113,9 +113,9 @@ find_package_handle_standard_args(
113113
HANDLE_VERSION_RANGE
114114
)
115115

116-
function(_Halide_LLVM_link target visibility)
116+
function(_Halide_LLVM_link target)
117117
llvm_map_components_to_libnames(comps ${ARGN})
118-
target_link_libraries("${target}" "${visibility}" ${comps})
118+
target_link_libraries("${target}" INTERFACE ${comps})
119119
endfunction()
120120

121121
if (Halide_LLVM_FOUND)
@@ -154,7 +154,7 @@ if (Halide_LLVM_FOUND)
154154
if (Halide_LLVM_SHARED_LIBS)
155155
target_link_libraries(Halide_LLVM::Core INTERFACE LLVM ${CMAKE_DL_LIBS})
156156
else ()
157-
_Halide_LLVM_link(Halide_LLVM::Core INTERFACE orcjit bitwriter linker passes)
157+
_Halide_LLVM_link(Halide_LLVM::Core orcjit bitwriter linker passes)
158158
endif ()
159159
endif ()
160160

@@ -164,7 +164,7 @@ if (Halide_LLVM_FOUND)
164164
target_link_libraries(Halide_LLVM::${comp} INTERFACE Halide_LLVM::Core)
165165

166166
if (NOT Halide_LLVM_SHARED_LIBS)
167-
_Halide_LLVM_link(Halide_LLVM::${comp} INTERFACE ${comp})
167+
_Halide_LLVM_link(Halide_LLVM::${comp} ${comp})
168168
endif ()
169169

170170
if (comp STREQUAL "WebAssembly")

cmake/HalideGeneratorHelpers.cmake

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,27 +5,27 @@ option(Halide_NO_DEFAULT_FLAGS "When enabled, suppresses recommended flags in ad
55
include(${CMAKE_CURRENT_LIST_DIR}/HalideTargetHelpers.cmake)
66
include(${CMAKE_CURRENT_LIST_DIR}/TargetExportScript.cmake)
77

8-
define_property(TARGET PROPERTY Halide_RT_TARGETS
8+
define_property(TARGET PROPERTY Halide_RT_TARGETS # nolint
99
BRIEF_DOCS "On a Halide runtime target, lists the targets the runtime backs"
1010
FULL_DOCS "On a Halide runtime target, lists the targets the runtime backs")
1111

12-
define_property(TARGET PROPERTY Halide_GENERATOR_HAS_POST_BUILD
12+
define_property(TARGET PROPERTY Halide_GENERATOR_HAS_POST_BUILD # nolint
1313
BRIEF_DOCS "On a Halide generator target, true if Halide.dll copy command has already been added."
1414
FULL_DOCS "On a Halide generator target, true if Halide.dll copy command has already been added.")
1515

16-
define_property(TARGET PROPERTY Halide_PYTHON_GENERATOR_SOURCE
16+
define_property(TARGET PROPERTY Halide_PYTHON_GENERATOR_SOURCE # nolint
1717
BRIEF_DOCS "Used to store the source file(s) for a Python Generator"
1818
FULL_DOCS "Used to store the source file(s) for a Python Generator")
1919

20-
define_property(TARGET PROPERTY Halide_LIBRARY_RUNTIME_TARGET
20+
define_property(TARGET PROPERTY Halide_LIBRARY_RUNTIME_TARGET # nolint
2121
BRIEF_DOCS "On a Halide library target, the runtime it uses."
2222
FULL_DOCS "On a Halide library target, the runtime it uses.")
2323

24-
define_property(TARGET PROPERTY Halide_LIBRARY_PYTHON_EXTENSION_CPP
24+
define_property(TARGET PROPERTY Halide_LIBRARY_PYTHON_EXTENSION_CPP # nolint
2525
BRIEF_DOCS "On a Halide library target, the .py.cpp generated for it (absent if none)."
2626
FULL_DOCS "On a Halide library target, the .py.cpp generated for it (absent if none).")
2727

28-
define_property(TARGET PROPERTY Halide_LIBRARY_FUNCTION_NAME
28+
define_property(TARGET PROPERTY Halide_LIBRARY_FUNCTION_NAME # nolint
2929
BRIEF_DOCS "On a Halide library target, the FUNCTION_NAME used."
3030
FULL_DOCS "On a Halide library target, the FUNCTION_NAME used.")
3131

@@ -227,7 +227,7 @@ function(_Halide_library_from_generator TARGET)
227227
"${ARG_TARGETS}"
228228
)
229229

230-
macro(_Halide_add_output type base_name)
230+
macro(_Halide_add_output type base_name) # nolint
231231
list(APPEND outputs "${type}")
232232
list(APPEND output_files "${base_name}${${type}_extension}")
233233
endmacro()
@@ -1039,12 +1039,12 @@ function(_Halide_target_link_gpu_libs TARGET VISIBILITY)
10391039
if ("${ARGN}" MATCHES "metal")
10401040
find_library(FOUNDATION_LIBRARY Foundation REQUIRED)
10411041
find_library(METAL_LIBRARY Metal REQUIRED)
1042-
target_link_libraries(${TARGET} ${VISIBILITY} "${FOUNDATION_LIBRARY}" "${METAL_LIBRARY}")
1042+
target_link_libraries("${TARGET}" "${VISIBILITY}" "${FOUNDATION_LIBRARY}" "${METAL_LIBRARY}") # nolint
10431043
endif ()
10441044

10451045
if ("${ARGN}" MATCHES "webgpu" AND NOT "${ARGN}" MATCHES "wasm")
10461046
find_package(Halide_WebGPU REQUIRED)
1047-
target_link_libraries(${TARGET} ${VISIBILITY} Halide::WebGPU)
1047+
target_link_libraries("${TARGET}" "${VISIBILITY}" Halide::WebGPU) # nolint
10481048
endif ()
10491049
endfunction()
10501050

cmake/HalidePackageConfigHelpers.cmake

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,8 @@ function(_Halide_pkgdep PKG)
5252
")")
5353
endif ()
5454

55-
set_property(DIRECTORY "${PROJECT_SOURCE_DIR}" APPEND PROPERTY pkgdeps "${PKG}")
56-
set_property(DIRECTORY "${PROJECT_SOURCE_DIR}" PROPERTY "pkgdeps[${PKG}]" "${code}")
55+
set_property(DIRECTORY "${PROJECT_SOURCE_DIR}" APPEND PROPERTY pkgdeps "${PKG}") # nolint
56+
set_property(DIRECTORY "${PROJECT_SOURCE_DIR}" PROPERTY "pkgdeps[${PKG}]" "${code}") # nolint
5757
endfunction()
5858

5959
##

cmake/HalideTargetHelpers.cmake

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,28 +4,31 @@ cmake_minimum_required(VERSION 3.28)
44
# Utilities for manipulating Halide target triples
55
##
66

7-
macro(_Halide_target_arch_os arch os)
8-
string(TOLOWER "${arch}" arch)
7+
function(_Halide_target_arch_os OUT_ARCH OUT_OS raw_arch raw_os)
8+
string(TOLOWER "${raw_arch}" arch)
99
list(TRANSFORM arch REPLACE "^.*(x86|arm|powerpc|hexagon|wasm|riscv).*$" "\\1")
1010
list(TRANSFORM arch REPLACE "^i.?86.*$" "x86")
1111
list(TRANSFORM arch REPLACE "^(amd|ia|em)64t?$" "x86")
1212
list(TRANSFORM arch REPLACE "^ppc(64(le)?)?$" "powerpc")
1313
list(TRANSFORM arch REPLACE "^aarch(64)?$" "arm")
1414

15-
string(TOLOWER "${os}" os)
15+
string(TOLOWER "${raw_os}" os)
1616
list(TRANSFORM os REPLACE "^darwin$" "osx")
1717
list(TRANSFORM os REPLACE "^emscripten$" "wasmrt")
1818

1919
# Fix up emscripten usage
2020
if (os STREQUAL "wasmrt" AND arch STREQUAL "x86")
2121
set(arch "wasm")
2222
endif ()
23-
endmacro()
23+
24+
set(${OUT_ARCH} "${arch}" PARENT_SCOPE)
25+
set(${OUT_OS} "${os}" PARENT_SCOPE)
26+
endfunction()
2427

2528
function(_Halide_host_target OUTVAR)
26-
_Halide_target_arch_os("${CMAKE_HOST_SYSTEM_PROCESSOR}" "${CMAKE_HOST_SYSTEM_NAME}")
29+
_Halide_target_arch_os(arch os "${CMAKE_HOST_SYSTEM_PROCESSOR}" "${CMAKE_HOST_SYSTEM_NAME}")
2730

28-
cmake_host_system_information(RESULT is_64bit QUERY IS_64BIT)
31+
cmake_host_system_information(RESULT is_64bit QUERY IS_64BIT) # nolint -- no alternative for host bitness
2932
if (is_64bit)
3033
set(bits 64)
3134
else ()
@@ -40,12 +43,12 @@ function(_Halide_cmake_target OUTVAR)
4043
if (CMAKE_OSX_ARCHITECTURES)
4144
set(${OUTVAR} "")
4245
foreach (processor IN LISTS CMAKE_OSX_ARCHITECTURES)
43-
_Halide_target_arch_os("${processor}" "${CMAKE_SYSTEM_NAME}")
46+
_Halide_target_arch_os(arch os "${processor}" "${CMAKE_SYSTEM_NAME}")
4447
list(APPEND ${OUTVAR} "${arch}-${bits}-${os}")
4548
endforeach ()
4649
list(REMOVE_DUPLICATES ${OUTVAR}) # defensive
4750
else ()
48-
_Halide_target_arch_os("${CMAKE_SYSTEM_PROCESSOR}" "${CMAKE_SYSTEM_NAME}")
51+
_Halide_target_arch_os(arch os "${CMAKE_SYSTEM_PROCESSOR}" "${CMAKE_SYSTEM_NAME}")
4952
set(${OUTVAR} "${arch}-${bits}-${os}")
5053
endif ()
5154
set(${OUTVAR} "${${OUTVAR}}" PARENT_SCOPE)

packaging/common/HalideConfig.cmake

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
cmake_minimum_required(VERSION 3.28)
22
@PACKAGE_INIT@
33

4-
macro(Halide_fail message)
4+
macro(Halide_fail message) # nolint -- required for find_package scoping
55
set(${CMAKE_FIND_PACKAGE_NAME}_NOT_FOUND_MESSAGE "${message}")
66
set(${CMAKE_FIND_PACKAGE_NAME}_FOUND FALSE)
77
return()
88
endmacro()
99

10-
macro(Halide_find_component_dependency comp dep)
10+
macro(Halide_find_component_dependency comp dep) # nolint
1111
set(Halide_quiet)
1212
if (${CMAKE_FIND_PACKAGE_NAME}_FIND_QUIETLY)
1313
set(Halide_quiet QUIET)
@@ -81,7 +81,7 @@ set(Halide_shared_targets "${CMAKE_CURRENT_LIST_DIR}/Halide-shared-targets.cmake
8181
set(Halide_static_deps "${CMAKE_CURRENT_LIST_DIR}/Halide-static-deps.cmake")
8282
set(Halide_shared_deps "${CMAKE_CURRENT_LIST_DIR}/Halide-shared-deps.cmake")
8383

84-
macro(Halide_load_targets type)
84+
macro(Halide_load_targets type) # nolint
8585
if (NOT EXISTS "${Halide_${type}_targets}")
8686
Halide_fail("Halide `${type}` libraries were requested but not found.")
8787
endif ()
@@ -141,15 +141,15 @@ unset(Halide_static_targets)
141141
# _Halide_fail after one redefinition. Doing it twice overwrites both since the
142142
# saving behavior doesn't continue past the first.
143143
foreach (i RANGE 0 1)
144-
macro(Halide_fail)
144+
macro(Halide_fail) # nolint -- poisoning internal APIs
145145
message(FATAL_ERROR "Cannot call internal API: Halide_fail")
146146
endmacro()
147147

148-
macro(Halide_find_component_dependency)
148+
macro(Halide_find_component_dependency) # nolint
149149
message(FATAL_ERROR "Cannot call internal API: Halide_find_component_dependency")
150150
endmacro()
151151

152-
macro(Halide_load_targets)
152+
macro(Halide_load_targets) # nolint
153153
message(FATAL_ERROR "Cannot call internal API: Halide_load_targets")
154154
endmacro()
155155
endforeach ()

tools/check_cmake_style.py

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
#!/usr/bin/env python3
2+
"""Enforce Halide's CMake coding standards from doc/CodeStyleCMake.md."""
3+
4+
import re
5+
import sys
6+
from pathlib import Path
7+
8+
# fmt: off
9+
10+
PROHIBITED_COMMANDS: dict[str, str] = {
11+
"add_compile_definitions": "use target_compile_definitions",
12+
"add_compile_options": "use target_compile_options",
13+
"add_definitions": "use target_compile_definitions",
14+
"add_link_options": "use target_link_options",
15+
"aux_source_directory": "list source files explicitly",
16+
"build_command": "use CMAKE_CTEST_COMMAND",
17+
"cmake_host_system_information": "inspect toolchain variables instead",
18+
"create_test_sourcelist": "use Halide's own testing solution",
19+
"define_property": "use a cache variable",
20+
"enable_language": "Halide is C/C++ only",
21+
"fltk_wrap_ui": "Halide does not use FLTK",
22+
"include_directories": "use target_include_directories",
23+
"include_external_msproject": "write a CMake package config file",
24+
"include_guard": "use functions, not recursive inclusion",
25+
"include_regular_expression": "changes default dependency checking",
26+
"link_directories": "use target_link_libraries",
27+
"link_libraries": "use target_link_libraries",
28+
"load_cache": "write a vcpkg port instead",
29+
"macro": "use function() instead",
30+
"remove_definitions": "use target_compile_definitions with genexes",
31+
"set_directory_properties": "use cache variables or target properties",
32+
"site_name": "privacy: do not leak host name",
33+
"variable_watch": "debugging helper, not for production",
34+
}
35+
36+
# fmt: on
37+
38+
# Patterns that need special handling beyond simple command detection.
39+
SPECIAL_PATTERNS: list[tuple[re.Pattern[str], str]] = [
40+
(
41+
re.compile(r"\bcmake_policy\s*\(.*\bOLD\b", re.IGNORECASE),
42+
"cmake_policy(... OLD) is deprecated; fix code for new policy",
43+
),
44+
(
45+
re.compile(r"\bfile\s*\(\s*GLOB", re.IGNORECASE),
46+
"file(GLOB ...) interacts poorly with incremental builds; list files explicitly",
47+
),
48+
(
49+
re.compile(r"\bset_property\s*\(\s*DIRECTORY\b", re.IGNORECASE),
50+
"set_property(DIRECTORY) is prohibited; use cache variables or target properties",
51+
),
52+
(
53+
re.compile(
54+
r"\btarget_link_libraries\s*\(\s*\S+\s+(?!PRIVATE|PUBLIC|INTERFACE)\S",
55+
re.IGNORECASE,
56+
),
57+
"target_link_libraries without visibility specifier; add PRIVATE, PUBLIC, or INTERFACE",
58+
),
59+
]
60+
61+
# Build a single regex for all prohibited commands: matches the command name
62+
# followed by '(' with optional whitespace, case-insensitive.
63+
_CMD_PATTERN = re.compile(
64+
r"\b(" + "|".join(re.escape(c) for c in PROHIBITED_COMMANDS) + r")\s*\(",
65+
re.IGNORECASE,
66+
)
67+
68+
_COMMENT_RE = re.compile(r"^\s*#")
69+
70+
71+
def check_file(path: Path):
72+
text = path.read_text()
73+
74+
for lineno, line in enumerate(text.splitlines(), start=1):
75+
if _COMMENT_RE.match(line):
76+
continue
77+
78+
if "#" in line:
79+
code, comment = line.split("#", 1)
80+
else:
81+
code, comment = line, ""
82+
83+
if "nolint" in comment:
84+
continue
85+
86+
for m in _CMD_PATTERN.finditer(code):
87+
cmd = m.group(1).lower()
88+
reason = PROHIBITED_COMMANDS[cmd]
89+
yield f"{path}:{lineno}: {cmd}() is prohibited; {reason}"
90+
91+
for pattern, message in SPECIAL_PATTERNS:
92+
if pattern.search(code):
93+
yield f"{path}:{lineno}: {message}"
94+
95+
96+
def main():
97+
status = 0
98+
for arg in sys.argv[1:]:
99+
for error in check_file(Path(arg)):
100+
print(error, file=sys.stderr)
101+
status = 1
102+
return status
103+
104+
105+
if __name__ == "__main__":
106+
sys.exit(main())

0 commit comments

Comments
 (0)