InsightSoftwareConsortium / InsightSoftwareConsortium/ITK
ITKModuleMacros: _SYSTEM_INCLUDE_DIRS does not support in-source paths (blocks SYSTEM treatment of vendored Eigen)
- Dominant language
- C++
- Stars
- 1.7k
- Forks
- 748
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 64
Description
`CMake/ITKModuleMacros.cmake` adds entries from `${itk-module}_SYSTEM_INCLUDE_DIRS` into `${itk-module}_SYSTEM_GENEX_INCLUDE_DIRS` as raw absolute paths with no `$` / `$` guards, so the variable cannot be used for in-source paths (export step rejects unguarded absolute paths in `INTERFACE_INCLUDE_DIRECTORIES`). This blocks the cleanest fix for warning-suppression on vendored Eigen and any future in-source SYSTEM-include need.
Surfaced from PR #6223's Greptile P2 review (warning leak from `${ITKEigen3_SOURCE_DIR}/src/itkeigen` exposed as regular `-I` rather than `-isystem`).
Reproducer
In `Modules/ThirdParty/Eigen3/CMakeLists.txt` (vendored branch), replace the regular-include addition with the structurally-correct SYSTEM equivalent:
```diff
- set(ITKEigen3_INCLUDE_DIRS ${ITKEigen3_SOURCE_DIR}/src ${ITKEigen3_SOURCE_DIR}/src/itkeigen)
+ set(ITKEigen3_INCLUDE_DIRS ${ITKEigen3_SOURCE_DIR}/src)
+ set(ITKEigen3_SYSTEM_INCLUDE_DIRS ${ITKEigen3_SOURCE_DIR}/src/itkeigen)
```
CMake fails the **generate** step:
```
CMake Error in Modules/ThirdParty/Eigen3/CMakeLists.txt:
Target "ITKEigen3Module" INTERFACE_INCLUDE_DIRECTORIES property contains
path: "/path/to/Modules/ThirdParty/Eigen3/src/itkeigen"
```
Root cause
`CMake/ITKModuleMacros.cmake:264-289` handles `_INCLUDE_DIRS` and `_SYSTEM_INCLUDE_DIRS` asymmetrically:
```cmake
# Lines 264-271: regular includes get $ + $
foreach(_dir ${${itk-module}_INCLUDE_DIRS})
list(APPEND ${itk-module}_GENEX_INCLUDE_DIRS "$")
endforeach()
list(APPEND ${itk-module}_GENEX_INCLUDE_DIRS
"$/${${itk-module}_INSTALL_INCLUDE_DIR}>")
# Lines 273-279: SYSTEM includes added raw — comment explicitly opts out
# of install-interface support.
if(${itk-module}_SYSTEM_INCLUDE_DIRS)
foreach(_dir ${${itk-module}_SYSTEM_INCLUDE_DIRS})
list(APPEND ${itk-module}_SYSTEM_GENEX_INCLUDE_DIRS ${_dir})
endforeach()
endif()
```
Comment at line 273-274 makes the design intent explicit:
> *System include directories are assumed to be external dependencies that are not installed, and thus do not have separate install interface paths.*
So `_SYSTEM_INCLUDE_DIRS` is structurally a `/usr/include`-style escape hatch for paths to libraries that exist independently of the ITK install tree. In-source paths (anything under `${ITKEigen3_SOURCE_DIR}/...`) violate this assumption: at export time CMake walks `INTERFACE_INCLUDE_DIRECTORIES` looking for unguarded absolute paths that aren't valid in the install interface and rejects them.
Why this matters now
PR #6223 unblocks proxTV / InsightSoftwareConsortium/ITKTotalVariation#57 by adding `${ITKEigen3_SOURCE_DIR}/src/itkeigen` to `ITKEigen3_INCLUDE_DIRS` so external consumers using `>` resolve via ITK's vendored Eigen. That works (verified end-to-end), but it exposes the path as `-I` (regular include) rather than `-isystem`. Eigen 5's templates emit `-Wshadow` / `-Wmaybe-uninitialized` / `-Wctad-maybe-unsupported` etc. that downstream consumers compiling with `-Wall -Wextra` will start seeing.
The neighboring vendored declaration in `Modules/ThirdParty/Eigen3/src/itkeigen/CMakeLists.txt` already uses the correct mechanism for the same physical directory:
```cmake
target_include_directories (eigen_internal SYSTEM INTERFACE
$
"$/${ITK3P_INSTALL_INCLUDE_DIR}/itkeigen>;"
)
```
This is what the macro version *should* produce when given an in-source SYSTEM path; today it cannot.
By symmetry the long-standing `${ITKEigen3_SOURCE_DIR}/src` regular include also probably wants SYSTEM treatment — it leaks the same Eigen warnings to any non-third-party consumer of `ITK::ITKEigen3Module` (e.g., ITKCommon and every downstream module that ultimately resolves an `ITK_EIGEN()` macro to `>`). It's been regular-include for the entire 4.x/5.x/6.x history and the codebase apparently doesn't trip on the warnings under ITK's own CI flags, but a careful audit would resolve whether it should also be SYSTEM.
Three approaches, in increasing scope
#### Approach A — Bypass the macro for the specific eigen path
Add the SYSTEM include directly on the `eigen_internal` target inside `Modules/ThirdParty/Eigen3/src/itkeigen/CMakeLists.txt`, mirroring the existing `${CMAKE_CURRENT_SOURCE_DIR}` line:
```cmake
target_include_directories (eigen_internal SYSTEM INTERFACE
$
$ # <-- add for path
"$/${ITK3P_INSTALL_INCLUDE_DIR}/itkeigen>;"
)
```
Wait — actually that's the same dir. The new path is `${ITKEigen3_SOURCE_DIR}/src/itkeigen` which equals `${CMAKE_CURRENT_SOURCE_DIR}` of the inner CMakeLists. So this *already exists*. Need to verify why the existing SYSTEM INTERFACE on `eigen_internal` doesn't propagate to external `>` consumers via `ITK::ITKEigen3Module → ITK::eigen_internal` link transitively. May be path normalization differences during target export (`.../itkeigen` vs `.../itkeigen/..`).
**Pros**: smallest change; bypasses the broken macro path.
**Cons**: needs investigation — the existing SYSTEM declaration may already be doing the right thing and PR #6223's regular-include addition may be redundant. Or the export representation reorders paths in a way that breaks resolution.
#### Approach B — Fix the macro for in-source SYSTEM paths
Extend the SYSTEM-include loop in `CMake/ITKModuleMacros.cmake:275-279` to mirror the regular-include treatment:
```cmake
foreach(_dir ${${itk-module}_SYSTEM_INCLUDE_DIRS})
list(APPEND ${itk-module}_SYSTEM_GENEX_INCLUDE_DIRS "$")
endforeach()
```
(With `$` only added when an install-side mapping makes sense — the existing comment about external dependencies suggests the install side should typically be empty or `find_package`-driven.)
**Pros**: Makes `_SYSTEM_INCLUDE_DIRS` work for in-source paths, which is what most module authors actually want. Closes the gap between the regular and SYSTEM behaviors.
**Cons**: touches the module macro every ITK module uses. Need to audit existing `_SYSTEM_INCLUDE_DIRS` consumers — if any module depends on the raw-path behavior (e.g., to inject a system path that *should* survive into the install interface), that consumer breaks. Quick grep suggests few in-tree consumers but every third-party module is a potential dependency.
#### Approach C — Move every Eigen include to SYSTEM, with audit
Once Approach B is in place, also move `${ITKEigen3_SOURCE_DIR}/src` from `_INCLUDE_DIRS` to `_SYSTEM_INCLUDE_DIRS` for symmetry. The existing `>` consumers (ITKCommon and friends) start receiving `-isystem` instead of `-I`, suppressing whatever warnings might leak. Lower priority because nothing's currently breaking.
**Pros**: full warning-suppression parity; one consistent SYSTEM treatment for the vendored Eigen.
**Cons**: changes warning surface for the whole tree. Needs `-Werror` CI on a representative platform set to surface any latent issues. Possibly trivial in practice but should be its own PR with a clean test cycle.
Recommended sequencing
1. **First**: investigate Approach A (does the existing `eigen_internal SYSTEM INTERFACE` already do the right thing once we understand the path-normalization quirk?). Cheap, may resolve the warning concern entirely without touching `ITKModuleMacros.cmake`.
2. **If Approach A is insufficient**: do Approach B as a standalone PR. Touch only the macro, audit existing consumers, ensure no regression in install-interface validity.
3. **Optionally after B**: do Approach C as a separate PR with `-Werror` CI verification.
PR #6223 ships the regular-`_INCLUDE_DIRS` form because it's the only one that actually generates today; this issue tracks the SYSTEM-treatment cleanup so it doesn't fall through.
Files / lines of interest
| File | Line | Role |
|---|---|---|
| `CMake/ITKModuleMacros.cmake` | 264-271 | Regular `_INCLUDE_DIRS` loop with `$` |
| `CMake/ITKModuleMacros.cmake` | 273-279 | SYSTEM `_SYSTEM_INCLUDE_DIRS` loop without generator-expression guards (the bug) |
| `CMake/ITKModuleMacros.cmake` | 281-289 | THIRD_PARTY consumption of both variables via `include_directories(...)` and `include_directories(SYSTEM ...)` |
| `Modules/ThirdParty/Eigen3/CMakeLists.txt` | 56-69 | Where this issue first surfaced; PR #6223 sets `_INCLUDE_DIRS` for both paths today |
| `Modules/ThirdParty/Eigen3/src/itkeigen/CMakeLists.txt` | (the `target_include_directories(eigen_internal SYSTEM INTERFACE ...)` block) | The existing in-source SYSTEM declaration; reference for what the macro version should produce |
Contributor guide
Assessment
This issue has not been assessed yet.