ENH: Allow the developer to pass ITK build options

[phcerdan]

trafficstars

Using CACHE variable: Slicer_ITK_EXTRA_CMAKE_ARGS:STRING This allows to build modules that might be needed for Modules, or custom apps.

Example: -DSlicer_ITK_EXTRA_CMAKE_ARGS:STRING=-DModule_XXX:BOOL=ON;

[jcfr]

Let me know what you think of this more general approach. the idea is the following:

Use with get_directory_property to get list of all CACHE_VARIABLES

Pass to project <projectName> the option -D<optionName>:<optionType>=<optionValue> if there was an option like the following passed to Slicer: -DSlicer_<projectName>_<optionName>:<optionType>=<optionValue>

Since this could apply to any project, this should probably be added to the artichoke project (this is where the module ExternalProjectDependency is maintained). See https://github.com/commontk/Artichoke, that way it would be available for any external project.

We could have the following convention:

<SUPERBUILD_TOPLEVEL_PROJECT>_EP_<projecName>_<OptionName>:<optionType>=<optionValue>

For example:

-DSlicer_EP_ITK_ITK_USE_MKL:BOOL=ON

[lassoan]

Having a general mechanism would be good. The …ITK_ITK... duplication is not very nice (and it would happen often, as variable names are often prefixed by the library name). Maybe we could add something in between, such as:

-DSlicer_EP_ITK_SET_ITK_USE_MKL:BOOL=ON -DSlicer_EP_ITK_FORWARD_ITK_USE_MKL:BOOL=ON -DSlicer_FORWARD_ITK_EP_ITK_USE_MKL:BOOL=ON -DSlicer_ITK_EP_ITK_USE_MKL:BOOL=ON

[jcfr]

The …ITK_ITK... duplication is not very nice

You read my mind ... I initially came up with -DSlicer_ITK_EP_ITK_USE_MKL:BOOL=ON but backtracked thinking it wouldn't be an issue to have the duplication.

That said, you are right ... being explicit makes sense.

I like -DSlicer_EP_ITK_SET_ITK_USE_MKL:BOOL=ON

[pieper]

I like -DSlicer_EP_ITK_SET_ITK_USE_MKL:BOOL=ON

At first glance I wouldn't know what EP meant and It's not clear which is a directive to Slicer and which is a directive to ITK.

Something like this would be more self-documenting to me:

-DSlicer_External_ITK_SET__ITK_USE_MKL:BOOL=ON

[lassoan]

Is the double-underscore a typo? I would not use it (it is typically reserved for machine use).

EP is used very commonly for "external project". See CMake (EP_PREFIX, EP_BASE, … - https://cmake.org/cmake/help/v3.0/module/ExternalProject.html) and many places in CTK and Slicer.

If "external" is spelled out then most of the places "project" is included as well (for example "..._EXTERNAL_PROJECT_ARGS").

So, if we want to be consistent with existing naming convention then we could use -DSlicer_EXTERNAL_PROJECT_ITK_SET_ITK_USE_MKL:BOOL=ON. It is a bit long, but if it greatly improves readability then we might choose to use it.

[pieper]

Yes, the double underscore was intentional because without it the symbols run together into an unreadable blob. Is there another separator that's valid? @phcerdan 's original suggested syntax was actually much clearer - the intent was obvious just looking at the line.

The build system is already crazy complex so anything we can do to make things more clear and explicit is helpful.

[lassoan]

Yes, I agree that @phcerdan's syntax is simpler, but passing several parameters as a string blob would make it hard to interpret the values.

For example, sometimes we set default values if no defaults are passed in from external sources. It would be difficult to implement this reliably if we accepted such low-level manual overrides.

[pieper]

How about using a different character between the semantic blocks of the variable name? It looks like dash, dot, or slash are actually valid but I don't remember seeing them in use.

https://cmake.org/cmake/help/v3.13/manual/cmake-language.7.html#variable-references

[ihnorton]

-DSlicer_EP_ITK_SET_ITK_USE_MKL:BOOL=ON -DSlicer_EXTERNAL_PROJECT_ITK_SET_ITK_USE_MKL:BOOL=ON

👎

@phcerdan 's original suggested syntax was actually much clearer - the intent was obvious just looking at the line.

👍. It would be great to generalize to Slicer_*_EXTRA_CMAKE_ARGS (and then leave it alone!)

[lassoan]

I suspect that Slicer_*_EXTRA_CMAKE_ARGS would be too low-level access to be exposed (it would be difficult to make it future-proof, it could interfere with options set using higher-level APIs, it is hard to parse the content, etc.), but @jcfr can confirm why he recommended the higher-level approach.

[jcfr]

can confirm why he recommended the higher-level approach.

Explicitly passing each option allows to have a one-to-one mapping with what would be set in the CMakeCache.txt and avoid to have to implement some complex parsing.

[sjh26]

A constrained version of this has been implemented here:

https://github.com/Slicer/Slicer/commit/3d01fcd4aeccecff5b7b05b24df3adc1f2619a7b

Allows for enabling modules by name.

[jcfr]

Cc: @rafaelpalomar