surround variables with quotes inside if ?

[VRichardJP]

I guess I won't surprise anyone saying cmake if() is quite a funny lad.

For example, let's consider this one:

ament_cmake/ament_cmake_auto/cmake/ament_auto_package.cmake

Line 69 in e78ed7e

if(NOT ${PROJECT_NAME}_LIBRARIES STREQUAL "")

• if ${${PROJECT_NAME}_LIBRARIES} is not an empty string, it evaluates to TRUE
• if ${${PROJECT_NAME}_LIBRARIES} is the empty string "", it evaluates to FALSE
• if ${PROJECT_NAME}_LIBRARIES variable has not been defined, it evaluates to... TRUE!

This is because in cmake, if(var ...) first check if a variable named var exists and if it is not the case, it considers var is the string "var" (which is not ""!)

A simple example:

# prints "VAR_THAT_DOES_NOT_EXIST!"
if (NOT VAR_THAT_DOES_NOT_EXIST STREQUAL "")
  message(WARNING "VAR_THAT_DOES_NOT_EXIST!")
endif()

# does not print (expected behavior)
set (VAR_THAT_IS_EMPTY "")
if (NOT VAR_THAT_IS_EMPTY STREQUAL "")
  message(WARNING "VAR_THAT_IS_EMPTY!")
endif()

So for example, if ament_auto_package() is called on a package that does not define any library, the ${PROJECT_NAME}_LIBRARIES variable is not defined but the command will still call ament_export_libraries and install (with no target). Although it is mostly harmless, this is bug-prone at quite head-scratching.

A minimal example:

1. Create a new ros pkg with ros2 pkg create --build-type ament_cmake test.
2. Edit CMakeLists.txt as such:

cmake_minimum_required(VERSION 3.14)
project(test)

find_package(ament_cmake_auto REQUIRED)
ament_auto_find_build_dependencies()

ament_auto_package()

3. add some message in the ament_auto_package.cmake if() shown above. For example:

  if(NOT ${PROJECT_NAME}_LIBRARIES STREQUAL "")
    message(FATAL_ERROR "Hello there! ${${PROJECT_NAME}_LIBRARIES}")
    # ...

4. Build.

You will get the following warning:

CMake Error at /home/sig/autoware/install/ament_cmake_auto/share/ament_cmake_auto/cmake/ament_auto_package.cmake:86 (message):
  Hello there!:
Call Stack (most recent call first):
  CMakeLists.txt:7 (ament_auto_package)

Note that many other if in the code base have this issue.

The usual workaround is to always surround variables with quotes. For example:

if(NOT "${${PROJECT_NAME}_LIBRARIES}" STREQUAL "")

[vineet131]

Thanks for this issue!
I am new to CMake and I am trying to integrate the Python bindings for RVIZ. I am getting errors not so different from what's stated here.
I am wondering if my error has anything to do with what you said. Because in my case, the function that needs to be involved is ament_get_recursive_properties(), and there are many areas where if conditions could possibly cause an error.

[clalancette]

We discussed this, and unfortunately this is a case-by-case basis kind of thing. Certainly there are times where we want to use quotes to be able to deal with things like semicolons (lists) in CMake, but there are other times (like macros) where we specifically do not want quotes.

My suggestion here is to open PR (or PRs) that specifically fix the places that you see as issues, and we can review them one-by-one.

[VRichardJP]

I see, I have not played enough with cmake to find cases where unquoted if are preferrable.
During the experimentation I described here, I have met this case:

diff --git a/ament_cmake_auto/cmake/ament_auto_package.cmake b/ament_cmake_auto/cmake/ament_auto_package.cmake
index 3c6c7bb..a69ce90 100644
--- a/ament_cmake_auto/cmake/ament_auto_package.cmake
+++ b/ament_cmake_auto/cmake/ament_auto_package.cmake
@@ -66,14 +66,16 @@ macro(ament_auto_package)
   endif()
 
   # export and install all libraries
-  if(NOT ${PROJECT_NAME}_LIBRARIES STREQUAL "")
+  if(NOT "${${PROJECT_NAME}_LIBRARIES}" STREQUAL "")
     ament_export_libraries(${${PROJECT_NAME}_LIBRARIES})
     install(
       TARGETS ${${PROJECT_NAME}_LIBRARIES}
+      EXPORT ${PROJECT_NAME}Targets
       ARCHIVE DESTINATION lib
       LIBRARY DESTINATION lib
       RUNTIME DESTINATION bin
     )
+    ament_export_targets(${PROJECT_NAME}Targets)
   endif()
 
   # install all executables

Where if the package does not define any library, we still get into the if and the install fails because of EXPORT ${PROJECT_NAME}Targets (if I remember correctly).
With the current code (and unquoted if) there seems to be no issue, because installing empty target is maybe ok for cmake (but exporting non-existing target would not be).