Skip to content

Block FetchContent in CMake style checks #9124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alexreinking merged 3 commits into from
May 7, 2026
+114 −24
Merged

Block FetchContent in CMake style checks #9124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d64d6aa
Block FetchContent in CMake style checks
alexreinking May 7, 2026
5b3148d
Let uv lock update the lockfile during pre-commit validation
alexreinking May 7, 2026
346f800
Update documentation to prohibit FetchContent
alexreinking May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,8 @@ repos:
files: (^|/)(CMakeLists\.txt|.*\.cmake)$

- id: uv-lock
name: check uv.lock is up to date
entry: uv lock --check
name: ensure uv.lock is up to date
entry: uv lock
Comment on lines -130 to +131
Copy link
Copy Markdown
Member Author

@alexreinking alexreinking May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the drive-by, but I noticed I had to manually run uv lock on a different branch after this caught my mistake and I thought that was annoying. This does the right thing (fails but updates).

language: system
pass_filenames: false
files: ^(pyproject\.toml|uv\.lock)$
123 changes: 101 additions & 22 deletions doc/CodeStyleCMake.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ modern CMake best practices.

- [Contributing CMake code to Halide](#contributing-cmake-code-to-halide)
- [General guidelines and best practices](#general-guidelines-and-best-practices)
- [Prohibited modules list](#prohibited-modules-list)
- [FetchContent](#fetchcontent)
- [Prohibited commands list](#prohibited-commands-list)
- [Prohibited variables list](#prohibited-variables-list)
- [Adding tests](#adding-tests)
Expand Down Expand Up @@ -67,6 +69,83 @@ The following are some common mistakes that lead to subtly broken builds.
`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 modules list

All deprecated, legacy, and "miscellaneous" (internal) modules are prohibited.
The list of these may be found in the upstream documentation:
https://cmake.org/cmake/help/latest/manual/cmake-modules.7.html#deprecated-modules

### FetchContent

At the moment, only one supported module is prohibited: `FetchContent`. There
are many reasons to avoid its use:

01. It brings third-party CMake code into the build, which can cause all sorts
of issues. In the common case, third-party projects hard-code incompatible
build settings, which are tricky to work around in CMake. In the worst case,
they can set cache variables or directory properties that break the
including project's build. Worse still, those cache variables persist in
`CMakeCache.txt` even after the dependency is removed or replaced, so the
only reliable fix is a clean reconfigure. Because configuration is expected
to be idempotent, these failures can be difficult to diagnose.
02. It is a poor fit for cross-compilation scenarios that require separate host
and target artifacts. FetchContent inlines the dependency's project into the
including build, so it is configured with the same toolchain as the rest of
that build. For instance, a project may need both the flatbuffers compiler
for the host system and the flatbuffers library for the target system. This
scenario is not supported by FetchContent's population model.
03. It performs network access at configure time. This makes air-gapped and
offline builds awkward. `FETCHCONTENT_FULLY_DISCONNECTED=ON` only works
after a successful first configure and adds latency to every fresh
configure. Source pinning is also weak: only commit SHAs are truly
immutable, branch and tag refs can be moved server-side, and `URL_HASH` is
opt-in. Package managers like vcpkg require hash-pinned archives by default
and produce a baseline that can be locked.
04. It does not maintain a persistent source or binary cache outside the build
tree. Populated sources and build products live under the build directory by
default, so deleting the build directory also deletes them. A fresh build
can therefore require another download and rebuild of dependencies, rather
than just rebuilding the top-level project. This also couples dependency
iteration to the parent project: tweaking a dependency's options forces a
parent reconfigure, and the dependency cannot be built or tested in
isolation.
05. The above issues exacerbate diamond dependency problems. Even if a
consistent version happens to be chosen, different intermediate dependencies
along each branch might impose incompatible build settings. For instance,
one project might try to enable an optional feature while another project
disables it.
06. Applying local fixes to dependencies is awkward. `FetchContent_Declare`'s
`PATCH_COMMAND` runs an arbitrary shell snippet that is hard to review,
version, or attribute. vcpkg ports keep patches as versioned `.patch` files
alongside the portfile, so they appear in code review and survive upstream
version bumps cleanly.
07. It pollutes the cache and target environment, even when steps are taken to
exclude test and utility targets. This clutters both graphical IDE
interfaces and the diagnostic output of build tools like Ninja (e.g. its
dependency graph and build profiler).
08. Targets created by FetchContent are considered _first-party_ targets,
meaning that special care must be taken when writing installation and
packaging rules. This complexity compounds when simultaneously supporting
other dependency resolution mechanisms that create third-party (i.e.
`IMPORTED`) targets.
09. It produces no provenance, license, or SBOM metadata. Package managers like
vcpkg and Conan emit machine-readable manifests of versions, licenses, and
source hashes that compliance tooling can consume. FetchContent emits
nothing, so every audit becomes a manual exercise.
10. FetchContent requests can be intercepted by a Dependency Provider which can
only be chosen by the top-level project. That means code that appears to
vendor a specific source tree can instead be redirected to some other
dependency resolution mechanism, such as a package manager. This makes the
resulting targets and build settings less predictable, and it compounds the
first-party versus imported-target packaging issues described above.

After broader approval, third-party dependencies must be consumed with
`find_package`. This also lets packagers and distributors substitute a system or
pre-built copy, rather than forcing every downstream to rebuild dependencies
from source. We use vcpkg in CI to manage our dependencies. If vcpkg lacks a
port, you must write a custom port in `cmake/vcpkg` (for the main Halide build)
or `apps/vcpkg/ports` (for the apps).

## Prohibited commands list

As mentioned above, using directory properties is brittle, and they are
Expand Down Expand Up @@ -125,28 +204,28 @@ If common properties need to be grouped together, use an INTERFACE target

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

| Command | Reason | Alternative |
| ------------------------------- | --------------------------------------------------------------------------------- | ------------------------------------------------------------------------------ |
| `aux_source_directory` | Interacts poorly with incremental builds and Git | List source files explicitly |
| `build_command` | CTest internal function | Use CTest build-and-test mode via [`CMAKE_CTEST_COMMAND`][cmake_ctest_command] |
| `cmake_host_system_information` | Usually misleading information. | Inspect [toolchain][cmake-toolchains] 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_sourcelist` | We use our own unit testing solution | See the [adding tests](#adding-tests) section. |
| `define_property` | Adds unnecessary complexity | Use a cache variable. Exceptions under special circumstances. |
| `enable_language` | Halide is C/C++ only | [`FindCUDAToolkit`][findcudatoolkit], appropriately guarded. |
| `file(GLOB ...)` | Interacts poorly with incremental builds and Git | List source files explicitly. Allowed if not globbing for source files. |
| `fltk_wrap_ui` | Halide does not use FLTK | None |
| `include_external_msproject` | Halide must remain portable | Write a CMake package config file or find module. |
| `include_guard` | Use of recursive inclusion is not allowed | Write (recursive) functions. |
| `include_regular_expression` | Changes default dependency checking behavior | None |
| `load_cache` | Superseded by [`FetchContent`][fetchcontent]/[`ExternalProject`][externalproject] | Write a vcpkg port or present a case for an exception. |
| `macro` | CMake macros are not hygienic and are therefore error-prone | Use functions instead. |
| `site_name` | Privacy: do not want leak host name information | Provide a cache variable, generate a unique name. |
| `variable_watch` | Debugging helper | None. Not needed in production. |

Do not introduce any dependencies via [`find_package`][find_package] without
broader approval. Importantly, never introduce a use of `FetchContent`. Add
dependencies to `vcpkg.json` or create a custom port.
| Command | Reason | Alternative |
| ------------------------------- | ----------------------------------------------------------- | ------------------------------------------------------------------------------ |
| `aux_source_directory` | Interacts poorly with incremental builds and Git | List source files explicitly |
| `build_command` | CTest internal function | Use CTest build-and-test mode via [`CMAKE_CTEST_COMMAND`][cmake_ctest_command] |
| `cmake_host_system_information` | Usually misleading information. | Inspect [toolchain][cmake-toolchains] 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_sourcelist` | We use our own unit testing solution | See the [adding tests](#adding-tests) section. |
| `define_property` | Adds unnecessary complexity | Use a cache variable. Exceptions under special circumstances. |
| `enable_language` | Halide is C/C++ only | [`FindCUDAToolkit`][findcudatoolkit], appropriately guarded. |
| `file(GLOB ...)` | Interacts poorly with incremental builds and Git | List source files explicitly. Allowed if not globbing for source files. |
| `fltk_wrap_ui` | Halide does not use FLTK | None |
| `include_external_msproject` | Halide must remain portable | Write a CMake package config file or find module. |
| `include_guard` | Use of recursive inclusion is not allowed | Write (recursive) functions. |
| `include_regular_expression` | Changes default dependency checking behavior | None |
| `load_cache` | Superseded by [`ExternalProject`][externalproject] | Write a vcpkg port or present a case for an exception. |
| `macro` | CMake macros are not hygienic and are therefore error-prone | Use functions instead. |
| `site_name` | Privacy: do not want leak host name information | Provide a cache variable, generate a unique name. |
| `variable_watch` | Debugging helper | None. Not needed in production. |

Do not introduce new dependencies without broader approval. Once approved, add
dependencies to `vcpkg.json` or create a custom port, and consume them with
[`find_package`][find_package] rather than `FetchContent`.

## Prohibited variables list

Expand Down
11 changes: 11 additions & 0 deletions tools/check_cmake_style.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
# fmt: off

PROHIBITED_COMMANDS: dict[str, str] = {
# keep-sorted start case=no
"add_compile_definitions": "use target_compile_definitions",
"add_compile_options": "use target_compile_options",
"add_definitions": "use target_compile_definitions",
Expand All @@ -18,6 +19,11 @@
"create_test_sourcelist": "use Halide's own testing solution",
"define_property": "use a cache variable",
"enable_language": "Halide is C/C++ only",
"FetchContent_Declare": "use find_package instead",
"FetchContent_GetProperties": "use find_package instead",
"FetchContent_MakeAvailable": "use find_package instead",
"FetchContent_Populate": "use find_package instead",
"FetchContent_SetPopulated": "use find_package instead",
"fltk_wrap_ui": "Halide does not use FLTK",
"include_directories": "use target_include_directories",
"include_external_msproject": "write a CMake package config file",
Expand All @@ -31,6 +37,7 @@
"set_directory_properties": "use cache variables or target properties",
"site_name": "privacy: do not leak host name",
"variable_watch": "debugging helper, not for production",
# keep-sorted end
}

# fmt: on
Expand All @@ -41,6 +48,10 @@
re.compile(r"\bcmake_policy\s*\(.*\bOLD\b", re.IGNORECASE),
"cmake_policy(... OLD) is deprecated; fix code for new policy",
),
(
re.compile(r"\binclude\s*\(\s*FetchContent\s*\)", re.IGNORECASE),
"include(FetchContent) is prohibited; use find_package instead",
),
(
re.compile(r"\bfile\s*\(\s*GLOB", re.IGNORECASE),
"file(GLOB ...) interacts poorly with incremental builds; list files explicitly",
Expand Down
Loading