MantisBT - CMake
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0015909CMakeModulespublic2016-01-11 16:072016-06-10 14:21
Reporterdkuegler 
Assigned ToMatt McCormick 
PrioritynormalSeverityminorReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product VersionCMake 3.4.1 
Target VersionCMake 3.5Fixed in VersionCMake 3.5 
Summary0015909: FindDCMTK.cmake outdated
DescriptionThe FindDCMTK in CMake is very outdated. It produces errors and all the libraries and paths have to be set up manually.

The script straight up, does not find DCMTK, even if specifically pointed to the right path with DCMTK_DIR.

But there is a up-to-date buildscript, that works properly in the CTK toolkit:
https://github.com/commontk/CTK/blob/master/Utilities/CMake/FindDCMTK.cmake [^]

So I would propose to use that file.
Steps To Reproduceinstall DCMTK
build DCMTK

create a CMakeLists.txt with
set(DCMTK path/to/dcmtk/build/folder)
find_package(DCMTK)
Additional Informationtested with Windows 8.1, CMake 3.3, 3.4
TagsNo tags attached.
Relationships
Attached Files
Issue History
Date ModifiedUsernameFieldChange
2016-01-11 16:07dkueglerNew Issue
2016-01-12 10:26Brad KingNote Added: 0040198
2016-01-12 11:18Matthew McCormickNote Added: 0040202
2016-01-12 12:08Jean-Christophe Fillion-RobinNote Added: 0040204
2016-01-12 12:28Matthew McCormickNote Added: 0040205
2016-01-12 13:48Brad KingNote Added: 0040210
2016-01-12 16:56Matthew McCormickNote Added: 0040211
2016-01-13 09:22Brad KingNote Added: 0040216
2016-01-13 10:56Matthew McCormickNote Added: 0040217
2016-01-13 11:23Jean-Christophe Fillion-RobinNote Added: 0040218
2016-01-13 14:18Matthew McCormickNote Added: 0040222
2016-01-13 14:23Jean-Christophe Fillion-RobinNote Added: 0040224
2016-01-14 11:29Brad KingNote Added: 0040240
2016-01-14 11:51Matthew McCormickNote Added: 0040241
2016-01-14 11:51Jean-Christophe Fillion-RobinNote Added: 0040242
2016-01-14 13:31Brad KingNote Added: 0040244
2016-01-14 13:32Brad KingAssigned To => Matt McCormick
2016-01-14 13:32Brad KingStatusnew => resolved
2016-01-14 13:32Brad KingResolutionopen => fixed
2016-01-14 13:32Brad KingFixed in Version => CMake 3.5
2016-01-14 13:32Brad KingTarget Version => CMake 3.5
2016-06-10 14:21Kitware RobotNote Added: 0041288
2016-06-10 14:21Kitware RobotStatusresolved => closed

Notes
(0040198)
Brad King   
2016-01-12 10:26   
Matt, Jc, there is a FindDCMTK in CTK as pointed out in the description, and there is also one in ITK at "Modules/ThirdParty/DCMTK/CMake/FindDCMTK.cmake".

Please look at choosing/integrating these to update the upstream CMake copy.
(0040202)
Matthew McCormick   
2016-01-12 11:18   
The FindDCMTK from CTK master has been pushed to the CMake stage:

  https://cmake.org/gitweb?p=stage/cmake.git;a=shortlog;h=refs/heads/FindDCMTK-update [^]

This version had been copied to ITK. After Jc's review, it should be good to merge.
(0040204)
Jean-Christophe Fillion-Robin   
2016-01-12 12:08   
@Matt: Thanks for creating the topic. Looks good to me.

Nitpicks: doc could be converted to rst.
(0040205)
Matthew McCormick   
2016-01-12 12:28   
Thanks for the review. Merged to next.
(0040210)
Brad King   
2016-01-12 13:48   
Re 0015909:0040205: Thanks. The ModuleNotices test fails because the copyright notice block format was not retained. Also please revise the documentation to process as reStructuredText correctly and render properly.
(0040211)
Matthew McCormick   
2016-01-12 16:56   
The ModulesNotices test has been fixed and the documentation now renders as reStructuredText.
(0040216)
Brad King   
2016-01-13 09:22   
Re 0015909:0040211: Thanks. Actually it looks like the documentation has a whole bunch of content specific to CTK and its effort to make DCMTKConfig.cmake available from upstream DCMTK. That is not relevant to CMake's documentation of the module. Please revise.
(0040217)
Matthew McCormick   
2016-01-13 10:56   
The content had been updated to only be relevant to CMake's documentation of the module with CTK-specific material removed.

I pushed an additional commit that simplifies the content.
(0040218)
Jean-Christophe Fillion-Robin   
2016-01-13 11:23   
Just to clarify, the documentation was not CTK specific. By not CTK specific, I mean that it doesn't mention any information specific to the commontk/CTK project itself.

Would it be possible to re-include the documentation by only updating part of it ?

To clarify, this text only pointed to the fork of DCMTK hosted on commontk organization.

  The set of patches is listed here: https://github.com/commontk/DCMTK/compare/79030ba...f461865 [^]


Now the patched have been integrated in uptstream DCMTK, let's point out to the specific DCMTK revision 662ae18 indicating after which DCMTK version support for Config file have been added. The feature is included start with the DCMTK official snapshot "DCMTK-3.6.1_20140617"

See http://git.dcmtk.org/web?p=dcmtk.git;a=commit;h=662ae187c493c6b9a73dd5e3875372cebd0c11fe [^]


That said, the "Details" section at the end should be removed entirely since it reference some past discussion on the Slicer list and his now obsolete.

Thanks for the help,
Jc
(0040222)
Matthew McCormick   
2016-01-13 14:18   
Thanks to all for the feedback.

A new patch has been pushed to the branch on next. This patch re-adds the compatibility table, re-organizes content, removes the Details section, and adds references to the upstream DCMTK versions that provide the DCMTKConfig.cmake. This patch could be squashed with its parent before integration.
(0040224)
Jean-Christophe Fillion-Robin   
2016-01-13 14:23   
+100

Looks great. Thanks
(0040240)
Brad King   
2016-01-14 11:29   
Matt, Jc, thanks for working on this. While the DCMTK CMakeification progress information is useful, I do not think it belongs in the public-facing documentation of the module. Only information needed by those using the module should be there. Notes about future maintenance of the module belong in separate comments that are not part of the documentation.
(0040241)
Matthew McCormick   
2016-01-14 11:51   
> Only information needed by those using the module should be there.

This is what his there. There are not notes about future maintenance of the module.
(0040242)
Jean-Christophe Fillion-Robin   
2016-01-14 11:51   
> Only information needed by those using the module / Notes about future maintenance

To clarify, the compatibility table is here to inform the user about which version of DCMTK they can use. It is not here to help maintaininh the "FindDCMTK.cmake" module itself.

As a user of DCMTK developing system using DCMTK, this is the type of information I am looking for.

It wouldn't be fair to simply mention that the user can call "find_package(DCMTK ...)" without the documentation providing details. Every user would then have to do a detective work to learn about issues.
(0040244)
Brad King   
2016-01-14 13:31   
Okay, thanks. I've squashed the fixups and merged to 'master'.
(0041288)
Kitware Robot   
2016-06-10 14:21   
This issue tracker is no longer used. Further discussion of this issue may take place in the current CMake Issues page linked in the banner at the top of this page.