`ifdef NO_FEATURE` vs `ifeq ($(NO_FEATURE), 1)`

[christoph-conrads]

At compile time, you can enable and disable features by giving parameters to make (or exporting environment variables) such as NO_SHARED, NO_STATIC, and NOFORTRAN. In Makefile, the variables are sometimes treated as booleans, e.g., Makefile, line 133-144, sometimes their existence is checked, e.g., Makefile:88.

Which of these operations is correct?

I strongly prefer the interpretation as booleans because I find definitions like NO_SHARED=1 very suggestive of booleans. In fact, I report this issue because I have a script that broke when it called make with NO_SHARED=0.

[martin-frbg]

Both are correct as far as "intended" usage is concerned, but I believe the nonintuitive (mis)behaviour of NO_SHARED=0 and similar has tripped up people before so using ifeq everywhere is probably better.

[brada4]

You are absolutely right, some -D/-U "booleans" probably could encompass more than 2 configurations in future.

[christoph-conrads]

I looked into a patch and now I feel it would be better to just check if a variable is defined.

In the programming language C every non-zero value is taken to mean true when interpreted as a boolean. If we wanted to have these semantics for the Makefile-based builds, a proper check would look like this in pseudo code:

if not defined NO_FOO or NO_FOO=0; then enable foo; fi
if defined NO_FOO and NO_FOO!=0; then disable foo; fi

This is ugly, not easy to implement as there is no simple logical or operator in Make, and it forces the programmer to think about double negations all the time.

We can now pretend that a variable NO_FOO can only take values zero and one but this hardly improves the situation:

if not defined NO_FOO or NO_FOO=0; then enable foo; fi
if NO_FOO=1; then disable foo; fi

Only the check for disabling a feature was made shorter.

For simplicity, I propose to check only if the variable is defined or not.

[christoph-conrads]

Looking more closely at the Makefile makes me more and more reluctant to submit a patch because variables enabling features like USE_OPENMP or BUILD_RELAPACK are intermixed with variables disabling features like NOFORTRAN (note the absence of an underscore), NO_SHARED, and NO_STATIC; checks with the former are better done by comparing with zero while only existence of a definition should be checked for the latter.

[martin-frbg]

Guess you found out why it has grown to this stage...
Perhaps it would be possible to just add just the or NO_foo = 0 for the variables that seem most likely to cause confusion, such as NO_SHARED. However, the comments in Makefile.rule already hint at the fact that using NO_foo=0 will not work as expected ("if you don't need this, please comment it in"). Perhaps this just needs to be pointed out more clearly - I suspect CMAKE users will not come into contact with Makefile.rule at all.