[CMake] FindDCMTK Not setting up DCMTK_INCLUDE_DIRECTORIES Correctly?

[Michael Hertling]

On 12/01/2011 08:22 AM, Thomas Sondergaard wrote:
> On 2011-11-30 07:23, Michael Hertling wrote:
>> On 11/29/2011 08:49 PM, Thomas Sondergaard wrote:
>>> On 2011-11-29 18:19, kent williams wrote:
>>>> I actually install DCMTK in a subdirectory of my top-level build, and
>>>> then my program that uses DCMTK is configured with DCMTK_DIR.
>>>
>>> What is DCMTK_DIR pointing at? The installation dir or the source dir?
>>> As you can see in the description of the string it is meant to point at
>>> the root of the source directory.
>>
>> A source tree beneath /usr/include? That's a very strange default, IMO.
> 
> Yes, that doesn't make a lot of sense.
> 
>>>> The problem isnt MY source, the problem is that the DCMTK headers use
>>>> the dcmtk/x/y.h path form to include OTHER DCMTK headers.
>>>
>>> That shouldn't be a problem as long as the headers are actually
>>> available in a directory structure that matches that.
>>
>> AFAICS, the problem isn't the subpath in the #include directives, but
>> the fact that FIND_PATH() in FindDCMTK.cmake doesn't return suitable
>> paths. E.g., if osconfig.h is included via "dcmtk/config/osconfig.h",
>> FIND_PATH() must return the path up to but *excluding* dcmtk/config;
>> however, FindDCMTK.cmake has FIND_PATH() search for "osconfig.h", so
>> it'll yield the header's full path *including* dcmtk/config. If this
>> is passed to INCLUDE_DIRECTORIES(), one will end up with -I.../dcmtk/
>> config, and beneath this path, dcmtk/config/osconfig.h can't be found,
>> i.e. the headers' inclusions among each other can't work in this way
>> unless the root of DCMTK's header tree is found by other means. The
>> DCMTK_dcmtk_INCLUDE_DIR variable nearly adds the crucial path to
>> DCMTK_INCLUDE_DIRS, but seems to keep one directory too much.
>>
>> @Kent: Including headers by use of subpaths is not bad, IMO;
>> instead, it's rather clever, e.g. for the following reasons:
>>
>> (1) It has an additional documentation effect because the #include
>>      directive indicates which module the included header stems from.
>>
>> (2) It separates the namespaces of the package's modules, so multiple
>>      modules might have headers of the same name. In fact, this applies
>>      to DCMTK as the djdecode.h header appears twice; how would you use
>>
>>      "dcmtk/dcmjpls/djdecode.h" and "dcmtk/dcmjpeg/djdecode.h"
>>
>>      within the same compilation unit without specifying their
>>      respective subpath in the #include directive? You'd need to use
>>      further measures like configured pseudo headers to accomplish this.
>>
>> (3) It reduces the number of necessary -I switches, i.e. arguments to
>>      INCLUDE_DIRECTORIES(), and that's particularly true for DCMTK. If
>>      all of the package's headers and all client code consistently use
>>      subpaths in the #include directives, you need just one single -I
>>      switch per installed header tree, and DCMTK has only one, so it
>>      would actually suffice to have<DCMTK_INSTALL_PREFIX>/include
>>      in DCMTK_INCLUDE_DIRS.
>>
>> Due to the latter point, and as long as client code uses subpaths
>> for header inclusion, the FIND_PATH() command in FindDCMTK.cmake
>> for the population of DCMTK_INCLUDE_DIRS might be simplified to
>>
>> FIND_PATH(DCMTK_DCMTK_INCLUDE_DIR dcmtk PATHS ${DCMTK_ROOT}/include)
>> MARK_AS_ADVANCED(DCMTK_DCMTK_INCLUDE_DIR)
>> IF(DCMTK_DCMTK_INCLUDE_DIR)
>>      SET(DCMTK_INCLUDE_DIRS ${DCMTK_DCMTK_INCLUDE_DIR})
>> ENDIF()
>>
>> with DCMTK_ROOT pointing at a DCMTK installation tree. If one wants to
>> still have individual DCMTK_*_INCLUDE_DIR variables, e.g. as indicators
>> if the installation is complete, one could retain the loop and possibly
>> invoke LIST(REMOVE_DUPLICATES DCMTK_INCLUDE_DIRS). In either case, the
>> headers should be denoted with subpaths. In order to have this work if
>> FindDCMTK.cmake is searching for a source tree, the PATHS need to be
>> modified a bit, but is there a special reason why this find module
>> should work with source trees, too? AFAIK, that's quite unusual.
> 
> On Windows some packages don't have an install target, so it is not 
> unusual. DCMTK does have one, so I agree that it is not necessary or 
> desirable for FindDCMTK. Cannot be removed now without the risk of 
> breaking some packages though.

If FindDCMTK.cmake is expected to work with a source tree containing an
in-source-build, it's OK as far as I'm concerned, but shouldn't it work
with an installation tree, too? Anyway, IMO, the actual issue is that
none of the include directories returned by FindDCMTK.cmake suits to
find a header if the latter is included with subpath - neither from a
source tree nor from an installation tree - and AFAICS, the inclusions
among DCMTK's headers are done with subpaths. So, how could a directive

#include "dcmtk/config/osconfig.h"

e.g., ever work if all include directories do contain the "dcmtk" part?

A possible approach to fix this issue while respecting source trees and
installation trees and without harming backward compatibility might be:

(1) Don't provide a default value for DCMTK_DIR as there simply isn't
    one; /usr/include/dcmtk is totally wrong, and even /usr/src/dcmtk
    or the like doesn't suit because due to the FHS, it's recommended
    to not build packages beneath /usr/src, so one cannot expect to
    find an in-source build there.

(2) Instead, use DCMTK_DIR as indicator whether FindDCMTK.cmake is to
    search an installation tree or a source tree: If it is set, look
    for the latter as documented; otherwise, look for the former as
    usual.

(3) Take this into account when searching for include directories
    and, possibly, also for libraries, e.g. roughly as follows:

IF(DCMTK_DIR)
    SET(DCMTK_config_TEST_HEADER dcmtk/config/osconfig.h)
    ...
    SET(DCMTK_ofstd_TEST_HEADER dcmtk/ofstd/ofstdinc.h)
    FOREACH(dir IN ITEMS config ... ofstd)
        FIND_PATH(DCMTK_${dir}_INCLUDE_DIR
                  ${DCMTK_${dir}_TEST_HEADER}
                  PATHS ${DCMTK_DIR}/${dir}/include
        )
        MARK_AS_ADVANCED(DCMTK_${dir}_INCLUDE_DIR)
        IF(DCMTK_${dir}_INCLUDE_DIR)
            LIST(APPEND DCMTK_INCLUDE_DIRS ${DCMTK_${dir}_INCLUDE_DIR})
        ENDIF()
    ENDFOREACH()
ELSE()
    FIND_PATH(DCMTK_INCLUDE_DIR dcmtk})
    MARK_AS_ADVANCED(DCMTK_INCLUDE_DIR)
    IF(DCMTK_INCLUDE_DIR)
        LIST(APPEND DCMTK_INCLUDE_DIRS ${DCMTK_INCLUDE_DIR})
    ENDIF()
ENDIF()

BTW, what does the module's maintainer, Mathieu Malaterre, say?

Some further remarks:

(1) Why are just the {config,ofstd,dcmdata,dcmimgle}-related results
    passed to FPHSA()? Might the user disable the remaining parts of
    DCMTK at configuration time? If so - I haven't checked that - is
    DCMTK actually a multi-component package, and shouldn't its find
    module be component-aware then?

(2) DCMTK can be built using CMake, so shouldn't the ultimate solution
    be to ask the developers to provide a configuration file for usage
    with FIND_PACKAGE() - perhaps, one for the source/build tree and
    one for the installation tree - and retire FindDCMTK.cmake?

(3) If a project contains/installs a header tree, it should document
    how these headers are supposed to be used, in particular whether
    to be included with or without a subpath. E.g., one of the very
    first facts one learns about a Qt class is the required include
    directive, and the same holds for the manpages of *nix system
    and library calls. AFAICS, the DCMTK documentation does not
    provide such information, and perhaps, one should file an
    appropriate request with the developers.

W.r.t. the latter issue, my personal rule of thumb is: If a project's
installed headers use subpaths for their inclusions among each other,
any client code should include these headers with subpaths, too, and
find modules / config files should provide include directories with
due regard to this.

Regards,

Michael