Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UseITK uses include_directories and link_directories #1450

Open
rijobro opened this issue Nov 29, 2019 · 9 comments
Open

UseITK uses include_directories and link_directories #1450

rijobro opened this issue Nov 29, 2019 · 9 comments
Assignees
Labels
type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots

Comments

@rijobro
Copy link

rijobro commented Nov 29, 2019

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

ITK/CMake/UseITK.cmake

Lines 26 to 30 in c95b998

# Add include directories needed to use ITK.
include_directories(BEFORE ${ITK_INCLUDE_DIRS})
# Add link directories needed to use ITK.
link_directories(${ITK_LIBRARY_DIRS})

include_directories(BEFORE ${CMAKE_CURRENT_BINARY_DIR}/ITKFactoryRegistration)

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 rijobro added the type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances label Nov 29, 2019
@thewtex thewtex added type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots and removed type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances labels Nov 30, 2019
@thewtex thewtex added this to the ITK v5.2.0 milestone Dec 5, 2019
@rijobro
Copy link
Author

rijobro commented Mar 28, 2020

Any movement on this one?

@dzenanz
Copy link
Member

dzenanz commented Mar 28, 2020

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
Copy link
Member

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
Copy link
Author

rijobro commented Mar 30, 2020

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
Copy link
Member

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
Copy link
Author

rijobro commented Apr 14, 2020

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

@KrisThielemans
Copy link
Contributor

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 UCL/STIR#432 to avoid cluttering this, unless it's of general benefit of course?

@stale
Copy link

stale bot commented Aug 13, 2020

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

@stale stale bot added the status:Use_Milestone_Backlog Use "Backlog" milestone instead of label for issues without a fixed deadline label Aug 13, 2020
@thewtex thewtex modified the milestones: ITK v5.2.0, ITK v5.3.0 Nov 24, 2020
@stale stale bot removed the status:Use_Milestone_Backlog Use "Backlog" milestone instead of label for issues without a fixed deadline label Nov 24, 2020
@stale
Copy link

stale bot commented Jun 11, 2021

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

@stale stale bot added the status:Use_Milestone_Backlog Use "Backlog" milestone instead of label for issues without a fixed deadline label Jun 11, 2021
@thewtex thewtex modified the milestones: ITK 5.3.0, ITK 5.4.0 Jan 4, 2022
@stale stale bot removed status:Use_Milestone_Backlog Use "Backlog" milestone instead of label for issues without a fixed deadline labels Jan 4, 2022
@thewtex thewtex modified the milestones: ITK 5.4.0, ITK 6.0.0 Mar 12, 2024
@hjmjohnson hjmjohnson modified the milestones: ITK 6.0.0, ITK 6.0 Beta 1 Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots
Projects
None yet
Development

No branches or pull requests

6 participants