search

InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.38k stars 661 forks source link

UseITK uses include_directories and link_directories #1450

Open rijobro opened 4 years ago

rijobro commented 4 years ago

Description

UseITK.cmake uses include_directories and link_directories.

Steps to Reproduce

N/A

Expected behavior

Variables should be returned such that target_include_directories and target_link_libraries can be used.

Actual behavior

https://github.com/InsightSoftwareConsortium/ITK/blob/c95b99852a9109a8fc176a06f88b2aa6d442154b/CMake/UseITK.cmake#L26-L30

https://github.com/InsightSoftwareConsortium/ITK/blob/c95b99852a9109a8fc176a06f88b2aa6d442154b/CMake/UseITK.cmake#L232

include_directores pollutes the include path. Our code (https://github.com/CCPPETMR/SIRF) builds against both ITK and NiftyReg, both of which have a copy of nifti1.h. For a particular library, we want to be using the NiftyReg files, but because of this issue, ITK includes get prepended. We therefore get into problems with the itk_nifti_mangle.h.

You can see this problem, which is present in one of our Travis builds: https://travis-ci.org/CCPPETMR/SIRF/builds/618272131?utm_source=github_status&utm_medium=notification. There is one matrix in red, due to this problem.

Reproducibility

N/A

Versions

From at least 4.13. Still present in master.

Environment

Linux Ubuntu 14.04 (and potentially other distributions).

Additional Information

N/A

rijobro commented 4 years ago

Any movement on this one?

dzenanz commented 4 years ago

Not really. ITK's build system pre-dates CMake 3 and its enablement of modern practices. Updating it is a huge undertaking, with lousy cost/benefit ratio. It will probably have to wait for the next big maintenance grant.

blowekamp commented 4 years ago 
Expected behavior

Variables should be returned such that target_include_directories and target_link_libraries can be used.

This can currently be done by inspecting the ITK modules variables. Here is what I did in SimpleITK:

https://github.com/SimpleITK/SimpleITK/blob/master/CMake/sitkTargetUseITK.cmake

Perhaps it should be made more widely available.

rijobro commented 4 years ago

This can currently be done by inspecting the ITK modules variables. Here is what I did in SimpleITK:

Do you use this for IO? I gave it a go lifting some relevant functionality from the UseITK.cmake file, but got into trouble - I think because the ITKFactoryRegistration was empty.

blowekamp commented 4 years ago

Yes, it for every thing. From this commit in SimpleITK, the factory registration can be explicitly done when using the "ITKImageIO" and "ITKTransfromIO" modules.

rijobro commented 4 years ago

@KrisThielemans info RE including ITK. SimpleITK's method looks good but potentially a lot of work to switch over.

KrisThielemans commented 4 years ago

thanks @rijobro. I have only very briefly glanced at this, and have no good idea what it would involve for us. Possibly that discussion should be held at https://github.com/UCL/STIR/issues/432 to avoid cluttering this, unless it's of general benefit of course?

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.