(0040198)
Brad King (manager)
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 (reporter)
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 (reporter)
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 (reporter)
2016-01-12 12:28

Thanks for the review. Merged to next.

(0040210)
Brad King (manager)
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 (reporter)
2016-01-12 16:56

The ModulesNotices test has been fixed and the documentation now renders as reStructuredText.

(0040216)
Brad King (manager)
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 (reporter)
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 (reporter)
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 (reporter)
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 (reporter)
2016-01-13 14:23

+100

Looks great. Thanks

(0040240)
Brad King (manager)
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 (reporter)
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 (reporter)
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 (manager)
2016-01-14 13:31

Okay, thanks. I've squashed the fixups and merged to 'master'.

(0041288)
Kitware Robot (administrator)
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.

Issue History
Date Modified	Username	Field	Change
2016-01-11 16:07	dkuegler	New Issue
2016-01-12 10:26	Brad King	Note Added: 0040198
2016-01-12 11:18	Matthew McCormick	Note Added: 0040202
2016-01-12 12:08	Jean-Christophe Fillion-Robin	Note Added: 0040204
2016-01-12 12:28	Matthew McCormick	Note Added: 0040205
2016-01-12 13:48	Brad King	Note Added: 0040210
2016-01-12 16:56	Matthew McCormick	Note Added: 0040211
2016-01-13 09:22	Brad King	Note Added: 0040216
2016-01-13 10:56	Matthew McCormick	Note Added: 0040217
2016-01-13 11:23	Jean-Christophe Fillion-Robin	Note Added: 0040218
2016-01-13 14:18	Matthew McCormick	Note Added: 0040222
2016-01-13 14:23	Jean-Christophe Fillion-Robin	Note Added: 0040224
2016-01-14 11:29	Brad King	Note Added: 0040240
2016-01-14 11:51	Matthew McCormick	Note Added: 0040241
2016-01-14 11:51	Jean-Christophe Fillion-Robin	Note Added: 0040242
2016-01-14 13:31	Brad King	Note Added: 0040244
2016-01-14 13:32	Brad King	Assigned To	=> Matt McCormick
2016-01-14 13:32	Brad King	Status	new => resolved
2016-01-14 13:32	Brad King	Resolution	open => fixed
2016-01-14 13:32	Brad King	Fixed in Version	=> CMake 3.5
2016-01-14 13:32	Brad King	Target Version	=> CMake 3.5
2016-06-10 14:21	Kitware Robot	Note Added: 0041288
2016-06-10 14:21	Kitware Robot	Status	resolved => closed

Notes
(0040198) Brad King (manager) 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 (reporter) 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 (reporter) 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 (reporter) 2016-01-12 12:28	Thanks for the review. Merged to next.

(0040210) Brad King (manager) 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 (reporter) 2016-01-12 16:56	The ModulesNotices test has been fixed and the documentation now renders as reStructuredText.

(0040216) Brad King (manager) 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 (reporter) 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 (reporter) 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 (reporter) 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 (reporter) 2016-01-13 14:23	+100 Looks great. Thanks

(0040240) Brad King (manager) 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 (reporter) 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 (reporter) 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 (manager) 2016-01-14 13:31	Okay, thanks. I've squashed the fixups and merged to 'master'.

(0041288) Kitware Robot (administrator) 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.

Relationships