ENH: Use target based includes#1426
Conversation
blowekamp
force-pushed
the
target_based_includes
branch
from
eadf1dc to
e160eff
Compare
|
@N-Dekker Please review. |
|
|
||
| include_directories(../MovingGenericPyramid) | ||
|
|
||
| if(USE_OpenCLMovingdGenericPyramid) |
There was a problem hiding this comment.
There is a typo here! A letter d in the middle of USE_OpenCLMovingdGenericPyramid! Obviously not your fault, @blowekamp! It was introduced with 2ce0a0a, more than 11 years ago. But as a consequence, the target_include_directories that you added is never executed here!
There was a problem hiding this comment.
@dpshamonin Hi Denis! Do you remember this code? I guess we should just remove the letter d from the middle of USE_OpenCLMovingdGenericPyramid, right?
N-Dekker
left a comment
N-Dekker
left a comment
There was a problem hiding this comment.
Very interesting, thanks @blowekamp
It does not yet work when OpenCL support is enabled, so it still needs to be adjusted a little bit 🤷
|
@blowekamp Can you please add the word CMake or CMakeLists to your commit subject line? For example: |
|
|
||
| target_include_directories(elxOpenCL PUBLIC ${OPENCL_INCLUDE_DIRS}) | ||
| target_compile_definitions(elxOpenCL PUBLIC ELASTIX_USE_OPENCL) | ||
|
|
There was a problem hiding this comment.
I'm getting compile errors saying:
elastix\Common\OpenCL\ITKimprovements\itkOpenCLContext.cxx(19,10): error C1083: Cannot open include file: 'itkOpenCLKernels.h': No such file or directory
To fix these, maybe add the following line:
target_include_directories(elxOpenCL PRIVATE ${OPENCL_KERNELS_DEBUG_PATH})
("itkOpenCLKernels.h.in" is configured to ${OPENCL_KERNELS_DEBUG_PATH}/itkOpenCLKernels.h)
There was a problem hiding this comment.
I just tried and FindOpenCL didn't find the headers on my linux or osx systems, and requires further investigations.
There was a problem hiding this comment.
@blowekamp I have a few fixes that should be applied before your PR:
- Pull request Fix USE_OpenCLMovingdGenericPyramid typo, restore virtual, improve "ITKimprovements"
#include's #1427
Note that the USE_OpenCLMovingdGenericPyramid typo fix might have a git merge conflict with your proposed change in "Components/MovingImagePyramids/OpenCLMovingGenericPyramid/CMakeLists.txt", but it's really very simple: just remove the letter d from the middle!
There was a problem hiding this comment.
@blowekamp Can you please rebase your PR onto the latest SuperElastix/elastix main branch? (Including the USE_OpenCLMovingdGenericPyramid typo fix.)
Replace directly level includes, with target based includes. This allows for setting include properties on targets which allows for propagation to users of the libraries. Additionally, private and public scope can be applied.
blowekamp
force-pushed
the
target_based_includes
branch
2 times, most recently
from
9127270 to
94e0f97
Compare
Thanks for your attempt to address my suggestion! I say "attempt", because it appeared in the wrong branch! target_Based_includes is not the branch of this pull request! It is target_based_includes !!! Please check your blowekamp/elastix/branches! |
blowekamp
force-pushed
the
target_based_includes
branch
from
94e0f97 to
cbda205
Compare
|
OK, things look correct now. |
N-Dekker
left a comment
N-Dekker
left a comment
There was a problem hiding this comment.
Thank you very much, @blowekamp !
2 participants
Replace directly level includes, with target based includes. This allows for setting include properties on targets which allows for propagation to users of the libraries. Additionally, private and public scope can be applied.