InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

ITKModuleMacros: _SYSTEM_INCLUDE_DIRS does not support in-source paths (blocks SYSTEM treatment of vendored Eigen)

Open
#6,224 4 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.