-
Notifications
You must be signed in to change notification settings - Fork 22
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
Superbuild: Pass additional ITK CMake configuration options #100
Superbuild: Pass additional ITK CMake configuration options #100
Conversation
endif() | ||
endforeach() | ||
# Todo, also pass all Module_* variables | ||
message(STATUS "ITK CMake Cache Args - ${ep_itk_cmake_cache_args}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could simplify this by using https://cmake.org/cmake/help/v3.11/prop_dir/CACHE_VARIABLES.html
You could then automatically propagate all variables prefixed with either ITK_WRAP_
or ITKGroup_
, Modules_
prefix
The one starting with ITK_
should still be explicitly passed i think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcfr The documentation says that this property is intended for debug purposes. Do you think it is a good idea to use it in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this property is intended for debug purposes
This is a hint. It makes sense to use it here.
We should submit a MR to cmake and update the documentation with something like:
this property is useful for debug purposes
Other references:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Thanks for your input.
CMakeLists.txt
Outdated
@@ -153,6 +153,46 @@ if(ITKPythonPackage_SUPERBUILD) | |||
message(STATUS "SuperBuild - Searching for python[OK]") | |||
message(STATUS "SuperBuild - DOXYGEN_EXECUTABLE: ${DOXYGEN_EXECUTABLE}") | |||
|
|||
# CMake configuration variables to pass to ITK's build | |||
set(ep_itk_cmake_cache_args "") | |||
foreach(var ITK_WRAP_DOC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ITK_WRAP_DOC
should be indented like the other variables
Sorting alphabetically also helps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Alternative approach for this: use a Git submodule. |
As soon as we release a new version of scikit-build with the following fix, this will work. See scikit-build/scikit-build#321 |
@thewtex What's the status of this patch? |
@hjmjohnson taking a look -- pushing an update that needs more testing. |
c6273b2
to
62dc173
Compare
62dc173
to
2e91988
Compare
Updated and tested. Hats off 🎩 to @jcfr for the nice cache variable pattern matching suggestion. |
@fbudin69500 Could you please take this branch and run with it?
Todos:
Module_*
options to the inner ExternalProject- [ ] Only enablepushed to Issue Improve Doxygen dependency use #116ITK_WRAP_DOC
ifDOXYGEN_EXECUTABLE
is available- [ ] Do a better job at findingpushed to Issue Improve Doxygen dependency use #116DOXYGEN_EXECUTABLE