[CMake] FindDCMTK Not setting up DCMTK_INCLUDE_DIRECTORIES Correctly?

[Thomas Sondergaard]

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.