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

ENH: Allow the developer to pass ITK build options #1012

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

phcerdan
Copy link

@phcerdan phcerdan commented Sep 3, 2018

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;

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

Example:
-DSlicer_ITK_EXTRA_CMAKE_ARGS:STRING=-DModule_XXX:BOOL=ON;
@jcfr
Copy link
Member

jcfr commented Dec 15, 2018

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

@jcfr jcfr added the wip label Dec 15, 2018
@lassoan
Copy link
Contributor

lassoan commented Dec 15, 2018

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

jcfr commented Dec 15, 2018

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

pieper commented Dec 15, 2018

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

lassoan commented Dec 16, 2018

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

pieper commented Dec 16, 2018

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

lassoan commented Dec 16, 2018

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

pieper commented Dec 16, 2018

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

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

lassoan commented Dec 18, 2018

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

jcfr commented Jun 21, 2019

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

sjh26 commented Oct 21, 2019

A constrained version of this has been implemented here:

Slicer/Slicer@3d01fcd

Allows for enabling modules by name.

@jcfr
Copy link
Member

jcfr commented Jan 22, 2020

Cc: @RafaelPalomar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

6 participants