[Insight-developers] proposal: remove the gdcm build options from ITK

[Mark Roden]

Hi Alex,

As this is getting a bit long, I'll snip out previously present text.

<snip>
>> I'd think that the core group should always be built,
>> and any user choosing not to do so is just shooting themselves in the
>> foot.  You can just build testing, but that leads to my second point:
>> the tests aren't (yet) organized into the proper submodules.
>
> Modularisation is not done with yet. It s in need of love from us all.
> Can you tell us more about this, what kind of experiment you did, what
> where your expectations and what was the observed behavior?

I expect that the tests for a particular operation be included in that
module.  So, for instance, all IO tests should be built when the
Testing flag is on and the IO module is included.  If the IO module is
not included, no IO tests.  GDCM tests would be in this category.  But
it would be true for other modules, like vnl (which I imagine should
be in the core), registration, etc-- all of their tests should be
included iff the module is included.

>
>>
>> Second, when I build just ITKGroup-Core and ITKGroup-IO, the gdcm
>> tests aren't present, but the gdcm build options show up.  That
>> suggests to me that the gdcm tests have been moved to another module.
>> If I include Utilities, the gdcm tests are still not included. Same
>> with Bridge.  Using Luis' grep -r -n trick, I find that the nonunit
>> module has the gdcm tests.
>
> From my quick investigation it looks like:
> - gdcm source code is in <source>/ITK/Utilities/GDCM
> - itk layer for gdcm is in <source>/ITK/IO/GDCM
> - tests for gdcm are in nonunit.
>
> First of, the gdcm source seems to be a full copy of gdcm which would
> explain why you see so much GDCM variables popping up even though they
> might not be relevant (*We* need to test them!)

It's almost a full copy.  It does not include the testing directory,
but does include the testing include.

Looking at the other options:
1) applications-- this is ITK, not gdcm proper.  I see no particular
need to build gdcm applications, but I also don't see it as a huge
speed/space burden on the user to include them.  But perhaps these
should be rolled into ITKApps, if they are present at all?
2) examples-- again, this is not gdcm proper.  Why would an ITK user
need to know how to use gdcm?  The wrappers should obfuscate any
interaction with the dicom handler, and sample code for one particular
dicom handler (in this case, gdcm) is, imo, superfluous.
3) testing-- this is a bit of a gray area.  Certain tests should
absolutely be included; I don't think that the tests that are in ITK
are sufficient coverage at the moment.  However, the ITK test suites
should only cover those code paths that expose gdcm functionality.
I'm not sufficiently adept at that interaction yet to decide if there
should be more tests; however, I'd propose that they should be ITK
specific tests of ITK functions, rather than of gdcm functions.  I
don't think that gdcm specific tests are relevant to the general ITK
user.
4) shared libs-- this value should match the ITK shared libs flag,
yes?  If there's a scenario where they should differ (and the user is
not using system gdcm), then I'd like to know.

The other options-- java/C#/python wrapping, etc-- are entirely
superfluous, as again, those should not be exposed to the user.

In effect, I think that all gdcm build options should either mimic
ITK's build options or simply not be present.  I think that the way to
do this is to have the gdcm cmakelists.txt file have some variable in
it that is 'are you compiling as part of ITK?', and if that variable
is true, choose reasonable defaults for all options.

I'm a big fan of giving users less unnecessary choices.  The trick is
figuring out which choices are unnecessary, and in this case, I think
all of them are.


>
> Then, I suppose that the GDCM tests don't have their proper and final
> test holder yet and that it's still work in progress. With the current
> layout, I guess we should expect GDCM test to be part of the IO tests.
> Again, just a wild guess.

I agree.  I think that they should be there, and we need to figure out
the coverage.  If the single test I worked on last week (two weeks
ago?) was any indication, my instinct tells me that there are several
scenarios that aren't covered.  I'm not yet adept enough at
understanding the ctest codepaths yet to determine if my instinct has
any match with reality.

<snipping the rest>

Mark