if(something) strikes again

Development
1

The behavior of if(something) continues to be a source of ongoing gotchas, even with CMP0054 set to NEW. Consider this code sample (run it with cmake -P <filename>):

cmake_minimum_required(VERSION 3.21)

# Show this just to confirm PATH is not empty
message("PATH = $ENV{PATH}")

# Should always print "Defined" because PATH will always be in the environment
if(DEFINED ENV{PATH})
    message("Defined")
else()
    message("Not defined")
endif()

# This always prints False  <----- probably unexpected
if(ENV{PATH})
    message("True")
else()
    message("False")
endif()

# This always prints True
set(someVar "$ENV{PATH}")
if(someVar)
    message("True")
else()
    message("False")
endif()

As noted in the comments, if(ENV{PATH}) always evaluates to false. The relevant part of the docs for the if() command state the following:

if(<variable|string>)

True if given a variable that is defined to a value that is not a false constant. False otherwise. (Note macro arguments are not variables.)

Technically speaking, ENV{PATH} is not the name of a CMake variable, but it kinda feels like it should be treated as such. Nevertheless, we’ve had the current behavior for a long time and introducing yet another policy to change it is probably only going to add to the confusion for developers.

I’m mostly pointing this out after seeing a code review that fell victim to this. At the very least, we should highlight this specific case in the docs (which I’m happy to do), but thought it worth bringing up here for discussion first in case others feel like we should do something about it.

1 Like
2

I do see the following at the very end of the page for the if() docs though:

There is no automatic evaluation for environment or cache Variable References. Their values must be referenced as $ENV{<name>} or $CACHE{<name>} wherever the above-documented condition syntax accepts <variable|string> .

That’s too easy to miss, we need to mention this earlier, up in the Basic Expressions section where the if(<variable|string>) form is documented.

1 Like
3

To be clear, part of the inconsistency for me is that if(DEFINED) does consider the ENV{...} and CACHE{...} forms. There is an understandable mental association between if(variable) and if(DEFINED variable), but this scenario highlights cases where that is going to cause problems.

4

I think it is not a good idea to do something about it. The current pattern can be easily spotted by a linter and notify the developer about it. Introducing a new policy makes this much harder.

5

The DEFINED predicate also only takes a variable name. Alas, it is the only one. Maybe its docs should be <variable|ENV{variable}|CACHE{variable}> to call it out as the special one?

3 Likes
6

How about a simple warning if if(ENV{something}) is used/seen without extra quotes e.g. if("ENV{something}") should not warn. I can just tell you it has also been used wrongly in vcpkg.

7

Merge request clarifying this case is here: https://gitlab.kitware.com/cmake/cmake/-/merge_requests/6768