-
-
Notifications
You must be signed in to change notification settings - Fork 716
Modernize ITK Module System with CMake Interface Libraries #5721
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.
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
base: main
Are you sure you want to change the base?
Modernize ITK Module System with CMake Interface Libraries #5721
Conversation
|
PR description sounds amazing! Now diving into the review. |
dzenanz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am glad you are working on this! Does this work? What else is needed, besides changing the rest of the examples?
hjmjohnson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blowekamp AMAZING! Thank you for tackling this. It is long overdue!
|
f8488a7 configures and builds locally, in Debug and RelWithDebInfo modes, including examples. CMake 4.2.1, VS 2022. Tests in RelWithDebInfo mode succeed. |
f8488a7 to
e648c27
Compare
|
Woohoo! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR modernizes the ITK module system by introducing CMake interface libraries (${ModuleName}Module) that provide target-based dependency management and proper transitive propagation of include directories and dependencies. This replaces the legacy directory-level include_directories() approach with modern target-specific target_include_directories() calls using generator expressions.
Key Changes:
- Created interface libraries for each ITK module with automatic transitive dependency linking
- Migrated from directory-level includes to target-based includes using BUILD_INTERFACE/INSTALL_INTERFACE generator expressions
- Updated 60+ filtering examples to explicitly link against specific module targets instead of the monolithic
${ITK_LIBRARIES}
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Utilities/Maintenance/WhatModulesITK.py | Enhanced script with argparse and new --link option to generate target_link_libraries commands; improved regex pattern for module detection |
| CMake/ITKModuleMacros.cmake | Added interface library creation for modules, migrated to target-based include directories with generator expressions, added transitive dependency linking |
| CMake/ITKModuleInfo.cmake.in | Added INCLUDE2_DIRS variable to support generator expression-based include directories |
| CMake/ITKModuleHeaderTest.cmake | Updated header tests to use target-based include directories |
| CMake/ITKModuleEnablement.cmake | Created factory meta-module interface libraries (ITKImageIO, ITKMeshIO, ITKTransformIO) |
| CMake/ITKModuleAPI.cmake | Added INCLUDE2_DIRS propagation through module configuration |
| Modules/ThirdParty/MetaIO/src/CMakeLists.txt | Added directory-level includes as compatibility workaround for third-party modules |
| Modules/ThirdParty/HDF5/CMakeLists.txt | Added directory-level includes as compatibility workaround for third-party modules |
| Examples/Filtering/CMakeLists.txt | Converted all examples from ${ITK_LIBRARIES} to explicit module targets with Module suffix and factory meta-modules |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e648c27 to
4b20ef5
Compare
|
Thank you @copilot for the thorough review! I've addressed the actionable feedback in the latest commit: Fixed:
Regarding directory-level includes in third-party modules (MetaIO/HDF5): |
|
In SimpleITK the PR SimpleITK/SimpleITK#2475 is using this branch along with cmake FetchContent with ITK. Additionally it is build ITK in a super build and separate build are using ITK from the source or from the build tree. They seem to be working. The one exception is the build with Elastic, which is encountering a Sobel filter related error 😸 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4b20ef5 to
ea12c51
Compare
|
@bradking Would you be able to provide feedback on this PR? |
mathstuf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some high-level recommendations based on VTK module experience:
- add alias targets for all targets that match what
find_package(ITK)users see - highly recommend exporting with
ITK::namespace (it will help with CPS support in the future) - use
EXPORT_NAMEproperty to name the post-::install target rather than the core target name (a mistake from VTK's module system because I was unaware of the property) - use
FILE_SETsource listings so that you don't even needtarget_include_directoriesand header installation Just Works™ even with subdirectories (VTK opportunistically does it, but still supports even CMake 3.12(!))
CMake/ITKModuleMacros.cmake
Outdated
| set( | ||
| itk-module-RUNTIME_LIBRARY_DIRS-install2 | ||
| "$<INSTALL_INTERFACE:\${ITK_INSTALL_PREFIX}/${ITK_INSTALL_RUNTIME_DIR}>" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an open CMake feature request for runtime usage requirements like this. But it's not implemented yet.
|
Some of those recommendations might be out-of-scope for this PR. |
ea12c51 to
5f1281c
Compare
5f1281c to
384da23
Compare
|
Thank you @mathstuf. You comments have lead me to clean thing up significantly and still finding more things to remove too. |
mathstuf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much cleaner. Some more cleanups are still available though.
CMake/ITKModuleEnablement.cmake
Outdated
| ${itk_module} | ||
| PROPERTIES | ||
| EXPORT_NAME | ||
| ITK::${itk_module} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just be ${itk_module} and the namespace come from the install(EXPORT) and export(EXPORT) calls.
CMake/ITKModuleMacros.cmake
Outdated
| ${itk-module}Module | ||
| PROPERT | ||
| EXPORT_NAME | ||
| ITK::${itk-module}Module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same stuff as above.
bc17e0b to
4b56f7a
Compare
|
Did you close the PR intentionally? |
|
The current install and export in ITK are done on a per target basis, so changing that would be a separate effort. With the current set of target property EXPORT_NAME only the selected targets are getting the ITK:: namespace. This is appealing because it only add a namespace to the new ITK interface libraries. However, if one call to export() contains the NAMESPACE keyword to a select target, then ALL export ITK targets get prefixed with the ITK:: namespace. This was unexpected. I am not sure how adding the ITK:: namespace to all exported and install components would be for compatibility for ITK. I'd expected it to be disruptive and not favored by the community. There may need to be some compatibility flags to disable the namespace or customize the namespace. I am unsure of the expectations and best practices here. |
|
Maybe the namespace customization effort can be combined with, or otherwise benefit from the work done in Slicer's fork? |
|
For compat, make |
47c0501 to
65f3c4d
Compare
Replace directory-level include_directories() with target-based
include properties to enable modern CMake practices and support
proper transitive include propagation.
Changes:
1. Created ${itk-module}_INCLUDE2_DIRS and ${itk-module}_SYSTEM_INCLUDE2_DIRS
- Use generator expressions for BUILD_INTERFACE and INSTALL_INTERFACE paths
- Variables produced in each module's CMake configuration file
- Enables proper include path handling in build and install trees
2. Modified itk_module_add_library() to apply target properties
- Uses target_include_directories() instead of directory-level includes
- Uses target_link_directories() for system library directories
- Ensures proper PUBLIC include propagation
3. Updated ITKModuleHeaderTest.cmake
- Added module include directories to header test targets
- Fixes compilation errors for header-only modules
4. Fixed specific module CMakeLists.txt
- IO/MINC, IO/TransformMINC: Removed directory-level includes
- Nonunit/Review: Updated for target-based includes
- ThirdParty modules: Added necessary include_directories where needed
Compatibility Notes:
- Modules not using itk_module_add_library() may need directory-level
include_directories() added explicitly
- External modules should update to use itk_module_add_library()
Each ITK CMake interface module includes properties of the module,
include the libraries, include directories, and other ITK interface
modules that are dependecies.
Example usage:
target_link_libraries(
GradientMagnitudeRecursiveGaussianImageFilter
PRIVATE
ITK::ITKImageGradientModule
ITK::ITKImageIntensityModule
ITK::ITKImageIO
)
This approach uses target specific requirements and properties to
avoid the need for the directory level properies.
Update all examples to use specific ITK CMake interface module targets
instead of ${ITK_LIBRARIES}. Examples now link to minimal required
modules with PRIVATE visibility and ITKImageIO for I/O.
Enhanced WhatModulesITK.py script with --link option to output
target_link_libraries commands. Fixed regex to parse module names across
multiple lines.
AI was use to generate a script to run WhatModules.py and update
the CMakeLists.txt, then manual assisted corrections were made.
Add ITK_*_FACTORY_REGISTER_MANAGER compile definitions and factory registration header includes to factory meta-module interface targets. Use itk_generate_factory_registration in Examples to register factories.
Introduce ITK_INTERFACE_LIBRARIES variable to allow users to link to
the interface libraries with properties.
Add ITK_LIBRARY_NAMESPACE configuration (defaults to 'ITK') that enables
namespaced targets for all ITK modules (e.g., ITK::ITKCommonModule).
Introduce LIBRARIES_DEPRECATED variable alongside LIBRARIES to maintain
backward compatibility. Create INTERFACE IMPORTED wrappers for old target
names that link to new namespaced targets with DEPRECATION property set.
Update module system to use ${module-targets-namespace} throughout:
- itk_module_impl builds both deprecated and namespaced library lists
- itk_module_link_dependencies uses namespaced targets
- itk_module_test uses namespaced targets
- Factory meta-modules use namespaced targets
Update external itk_modile_config api to use ITK interface module
libraries.
Add init_module_vars() macro to set per-module namespace configuration.
Add documentation for itk_module_target_export() explaining export
mechanism and namespace handling for library targets.
65f3c4d to
f0f88cb
Compare
| set(@itk-module@_FACTORY_NAMES "@itk-module-FACTORY_NAMES@") | ||
|
|
||
| # Create deprecated library interface wrappers | ||
| # TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blowekamp AWESOME WORK!!!
I think that this TODO should be removed. Either we need the deprecated library interface wrappers for backwards compatibility not at all moving forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
This PR add a namespace (optional) to the libraries. So for example ITKCommon is now ITK::ITKCommon. Do you think we need to go through the effort to create another interface library linking to the namespaces library with a deprecation property?
For example:
add_library(ITKCommon INTERFACE IMPORTED)
set_target_properties(ITKCommon PROPERTIES
INTERFACE_LINK_LIBRARIES ITK::ITKCommon
DEPRECATION "Use ITK::ITKCommon instead")
The users directly specifying the library files without using the ITK configuration would likely not be effected and this depreciate would not help them. Only users who manually specified libraries through CMake , but not using the produce variables might be affected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
manually specified libraries through CMake
Does this hold for remote modules? Will we have to update all the remote modules to keep them working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a remote module is using itk_module to specify dependencies on ITK modules, setting {module{_LIBRARIES, and using itk_module_add_library as expected of a module then the namespace and Module interface library should all be created as expected.
There were a couple ITK internal modules that were doing manual creation of libraries that needed to be updated e.g. #5720
| set(_libraries "") | ||
| foreach(dep IN LISTS ${itk-module}_LIBRARIES) | ||
| # check if dep already has namespace and Module suffix | ||
| if("${dep}" MATCHES "^(.*)::(.*)$") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alas I know next to nothing about cmake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(ITK_USE_SYSTEM_EIGEN)
set(_eigen_itk_target Eigen3::Eigen)
set(_Eigen3_SYSTEM_OR_INTERNAL "Eigen3")
else()
set(_eigen_itk_target ITKInternalEigen3::Eigen)
set(_Eigen3_SYSTEM_OR_INTERNAL "ITKInternalEigen3")
endif()I think we can switch the ITKInternalEigen3 namespace to the new one.
The eigen module exports the variable:
set(${_Eigen3_SYSTEM_OR_INTERNAL}_DIR \"${Eigen3_DIR_INSTALL}\")i.e "ITKInternalEigen3_DIR" is a variable that should not be modified. Used for example in https://github.com/InsightSoftwareConsortium/ITKTotalVariation/blob/94b85b4890bfad16a9861d76eb25563d034c328a/CMakeLists.txt
But the namespace itself, it should be fine to switch it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VTK does it by always using VTK::eigen as the target. This is then either the vendored copy or it links to the external package's target.
This PR introduces modern CMake interface libraries for ITK modules, enabling clean target-based linking and proper transitive dependency propagation.
Key Changes
CMake Interface Libraries - Created
${ModuleName}Moduleinterface library for each ITK module with:Target-Based Includes - Replaced directory-level
include_directories()with target properties:target_include_directories()withBUILD_INTERFACE/INSTALL_INTERFACEExample Modernization - Updated examples to use specific module targets:
Enhanced Tooling - Improved
WhatModulesITK.pywith--linkoption to generate updatedtarget_link_librariescommandsFactory Registration - Properties part of factory meta-modules
Interface meta-modules for factories: ITKImageIO, ITKMeshIO, ITKTransformIO and FFTImageFilterInit factories. Factory modules will be added as dependencies to these meta-module. When itk_generate_factory_registration() is called, it adds the include directory containing the generated FactoryRegisterManager header file.
Example Usage:
Benefits
${ITK_LIBRARIES})Compatibility
itk_module_add_library()for automatic target properties${ITK_LIBRARIES}pattern still supported