Adding support for customizing ITK namespace

1

Plan

We propose to update ITK to support customizing the ITK namespace.

Motivation

These change will allow importing itk python modules into application themselves built against ITK and ensure filter execution return valid results.

While prior outstanding efforts (PR-3156, PR-3330 , PR-3478 and many more) helped to allow importing itk modules into application like Slicer, the root issue is that symbols are bound to the one already available in the ITK libraries loaded by Slicer.

Running the application, specifying LD_DEBUG_OUTPUT=/tmp/reloc-bindings LD_DEBUG=reloc,bindings and importing itk allows to confirm this.

Here is a portion of the output illustrating that the symbol itk::Object::New() is resolved against libITKCommon-5.3.so.1 built by Slicer instead of _ITKCommonPython.so library provided in the wheel.

   2595154:     binding file 
     /home/jcfr/Downloads/itk-wheels-installed/lib/python3.9/site-packages/itk/_ITKCommonPython.so [0] 
   to 
     /home/jcfr/Projects/Slicer-Debug/ITK-build/lib/./libITKCommon-5.3.so.1 [0]: normal symbol `itk::Object::New()'

And here the expected symbol that should be resolved against:

$ readelf --wide -s _ITKCommonPython.so  | c++filt  | ack "itk::Object::New"
 13688: 0000000002d9f8a0   315 FUNC    GLOBAL DEFAULT    9 itk::Object::New()

For more details, see The LD_DEBUG environment variable | B. Nikolic Software and Computing Blog

Proposed solution

Similarly to the namespace support available in Qt (See qtnamespacemacros.h), we are considering introducing macros like the following:

#define ITK_NAMESPACE itk  // default namespace that can be configured
#define ITK_BEGIN_NAMESPACE  namespace ITK_NAMESPACE {
#define ITK_END_NAMESPACE }

Consider solution

We also considered creating a monolithic itk module (e.g _ITKPython.so) but that would increase its size and would lead to similar issue for ITK based modules.

Next steps

The ITK fork integrated in Slicer will be used to test and develop the namespace support.

We will also plan to provide scripts and instruction to (1) update existing code and (2) developer ITK classes and (3) use ITK classes to be compatible with arbitrary value of ITK_NAMESPACE

3 Likes
2

This sounds like a good approach, and is common in many of ITKā€™s third party libraries.

Is there a known motivation for ITK_BEGIN_NAMESPACE and ITK_END_NAMESPACE? It seems like it may obscure things, and inhibit reality some.

Also most of the test are outside of the itk name space, so including ā€œITK_NAMESPACEā€ as a prefix is going to get rather verbose quickly.

3

Is there a known motivation for ITK_BEGIN_NAMESPACE and ITK_END_NAMESPACE?

This is inspired from what Qt has been doing.

References:

4

most of the test are outside of the itk name space

Code aiming to support using ITK built with a namespace different from itk would have to be updated.

For example, itkSampleTest4.cxx would have to be updated like this:

diff --git a/Modules/Numerics/Statistics/test/itkSampleTest4.cxx b/Modules/Numerics/Statistics/test/itkSampleTest4.cxx
index 866dded14e..4ded363948 100644
--- a/Modules/Numerics/Statistics/test/itkSampleTest4.cxx
+++ b/Modules/Numerics/Statistics/test/itkSampleTest4.cxx
@@ -21,7 +21,7 @@
 #include "itkObjectFactory.h"
 #include "itkMath.h"
 
-namespace itk
+namespace ITK_NAMESPACE
 {
 namespace Statistics
 {
@@ -132,7 +132,7 @@ itkSampleTest4(int, char *[])
 
   using MeasurementVectorType = std::vector<float>;
 
-  using SampleType = itk::Statistics::SampleTest::MySample<MeasurementVectorType>;
+  using SampleType = ITK_NAMESPACE::Statistics::SampleTest::MySample<MeasurementVectorType>;
 
   auto sample = SampleType::New();
 
@@ -175,7 +175,7 @@ itkSampleTest4(int, char *[])
 
   for (unsigned int j = 0; j < MeasurementVectorSize; ++j)
   {
-    if (itk::Math::NotExactlyEquals(measureBack[j], measure[j]))
+    if (ITK_NAMESPACE::Math::NotExactlyEquals(measureBack[j], measure[j]))
     {
       std::cerr << "Error in Set/Get MeasurementVector()" << std::endl;
       return EXIT_FAILURE;
@@ -191,7 +191,7 @@ itkSampleTest4(int, char *[])
     std::cerr << "Sample failed to throw an exception when calling SetMeasurementVectorSize()" << std::endl;
     return EXIT_FAILURE;
   }
-  catch (const itk::ExceptionObject & excp)
+  catch (const ITK_NAMESPACE::ExceptionObject & excp)
   {
     std::cout << "Expected exception caught: " << excp << std::endl;
   }
@@ -205,7 +205,7 @@ itkSampleTest4(int, char *[])
   {
     sample->SetMeasurementVectorSize(MeasurementVectorSize + 5);
   }
-  catch (const itk::ExceptionObject & excp)
+  catch (const ITK_NAMESPACE::ExceptionObject & excp)
   {
     std::cerr << excp << std::endl;
     return EXIT_FAILURE;
5

And since ITK already has a namespace, the introduction of ITK_BEGIN_NAMESPACE and ITK_END_NAMESPACE is likely not needed, and using only ITK_NAMESPACE macro (or an intermediate alias set based on the macro) would be sufficient.

1 Like
6

In looking into my own question I came upon this SO:

Which looks like one advantage of the BEGIN/END macro is defining nested namespace. However C++17 has improved the syntax to allow for defining nested names spaced with `::``, so the additional benefits are less significant.

7

Is there any friendlier alternative for the macro name? ITK_NAMESPACE seems extremely long for having a lot of copies of it in the code.

itkns maybe? having 4 consonant in a row seems a good protection against name collides out there in the wild.

1 Like
8

We should indeed have a way to optimize and reduce the verbosity by using something like this:

using itkns = ITK_NAMESPACE

or

#if !defined(ITK_NAMESPACE_IS_ITK)
using itk = ITK_NAMESPACE_IS_ITK;
#endif
1 Like
9

Updates

  • The Slicer pull request #6564 adding support for ITK custom namespace has been integrated into Slicer.

  • Use of functionality relying on either ITK C++ or SimpleITK python package built within Slicer and against the customer ITK are working on macOS/Linux/Windows.

  • Use of filter like discrete_gaussian_image_filter provided by the ITK Python package itk 5.3rc4.post3 installed from PyPI is working on Linux and Windows. An issue has been identified on macOS and is been discussed under a different topic here.

Implementation

Slicer/ITK has been updated in Slicer/ITK@69e52f05c. In a nutshell, the itkNamespace.h header was introduced. It provides the logic for redefining ā€œitkā€ only if a custom namespace has been provided. This ensures that the default behavior remains unchanged.

To minimize the number of files to update, the itkNamespace.h may be included
through three mechanisms:

(1) Through itkConfigure.h: included either directly or indirectly by most files

(2) Through Export header: support case of header like itkFEMSolution.h only including the export header

(3) Through explicit include: support case of header without any includes (e.g itkMakeFilled.h)

cc: @dzenanz @matt.mccormick

Similarly, Slicer/SimpleITK has also been updated introducing sitkNamespace.h. See Slicer/SimpleITK@c7f151412.

cc: @blowekamp

Next steps

The implementation associated with Slicer/ITK and Slicer/SimpleITK unconditionally define:

Logic should be generalized by introducing the CMake options ITK_NAMESPACE and SITK_ITK_NAMESPACE, if associated value is itk ā€¦ no re-definition should happen.

Within ITK, mangling logic should be applied to remaining third party (zlib, gdcm, ā€¦) (thanks @dzenanz for the suggestion)

3 Likes
10

I would expect the ITKā€™s namespace to be defined in the ITKConfig.cmake file used by find package to allow consuming packages to use the value in CMake. For SimpleITK that would allow the new namespace to be in sitkConfigure.h.in.

Also consider just making definitions in sitkMacro rather than a separate file.

11

would expect the ITKā€™s namespace to be defined in the ITKConfig.cmake

Thanks for the feedback, to allow client projects to implement CMake logic based on the ITK internal namespace, adding its value to ITKConfig.cmake is sensible.

For SimpleITK that would allow the new namespace to be in sitkConfigure.h.in.
Also consider just making definitions in sitkMacro rather than a separate file.

For header that are not already including header, the original rational was to avoid having include like:

#include "sitkMacro.h" // For overriding ITK namespace

and make things more explicit by having the name of the file describe its function.

Now, since the redefinition of the ITK namespace already happen for the entire SimpleITK expect a few headers (sitkImageIOUtilities.h, sitkPixelIDTypes.h, sitkConditional.h and sitkCommon.h), I will simplify the approach and add the logic to sitkConfigure.h.in

@blowekamp Would you be okay making it requirement for every SimpleITK headers to always include sitkConfigure.h ?

12

Name of macros ?

ITK_NAMESPACE vs ITK_ABI_NAMESPACE ?

To capitalize what was done recently in VTK with the integration of a similar feature through the introduction of vtkABINamespace.h.in (See vtk@9519ee65b), I am considering:

  • itkABINamespace.h.in
  • ITK_ABI_NAMESPACE (instead of ITK_NAMESPACE)

This would be consistent with the fact this new option only impact the generated symbols and now how developer write C++ code using either ITK or SimpleITK.

13

I would be fine with either.

14

Yes, or sitkMacro.h. The policy could be that each file must include ā€œsitk{library}.hā€ e.g. sitkCommon.h or sitkMacro.h/sitkConfigure.h. It looks like most of the compiled library methods follow this but many meta/template programming files may not directly for allow it.

Your SimpleITK change showed that " sitkImageIOUtilities.h" was using sitkMacro.h definitions without explicitly including it.