OpenXLSX icon indicating copy to clipboard operation
OpenXLSX copied to clipboard

Fix OPENXLSX_LIBRARY_TYPE checks

Open ts826848 opened this issue 2 years ago • 0 comments

CMake's if() statement works a bit strangely for historical reasons. In particular, variable evaluation can be somewhat unintuitive, which can lead to strange behavior.

In particular, the docs state^0:

Note that normal variable evaluation with ${} applies before the if
command even receives the arguments. Therefore code like

    set(var1 OFF)
    set(var2 "var1")
    if(${var2})

appears to the if command as

    if(var1)

and is evaluated according to the if(<variable>) case documented
above. The result is OFF which is false. However, if we remove the
${} from the example then the command sees

    if(var2)

which is true because var2 is defined to var1 which is not a false
constant.

OpenXLSX checks OPENXLSX_LIBRARY_TYPE by using a bare ${} in an if statement. I believe this would work when OpenXLSX is used on its own, but it can result in very confusing errors if one is unlucky.

In particular, if "STATIC" or "SHARED" exist as CMake variables (and are not set to the strings "STATIC" or "SHARED"), the checks involving OPENXLSX_LIBRARY_TYPE will break. This is because OPENXLSX_LIBRARY_TYPE is first evaluated to STATIC/SHARED, then the if() statement checks to see if that is a variable. Since it is in this case, the value of that variable is used instead of the string "STATIC" or "SHARED", and the value of that variable is probably not what OpenXLSX expects.

The fix is to either remove the bare ${} when checking variables in if() statements or to quote it (i.e., ${} becomes "${}"). I chose the latter in this case because I feel it's a bit more obvious what is going on.

Chances are most CMake projects don't have STATIC or SHARED as variables floating around, but seems I happen to be the unlucky one.

ts826848 avatar Aug 04 '23 06:08 ts826848