fatal error wasn't being triggered until after list(SORT ...) error would happen

[codebot]

No description provided.

[codebot]

My commit description was awful. The problem is that the list(SORT ...) command would return an error if the "list" was an empty string.

[jacquelinekay]

+1

[dirk-thomas]

get_available_rmw_implementations should be able to return an empty list if there is no rmw implementation instead of raising a fatal error.

[codebot]

ok, so perhaps the change should be to only call list(SORT ...) if the "list" is indeed non-empty ? if so, I'll put the error message back where it was, and put a conditional around the call to list(SORT ...)

[dirk-thomas]

I don't know why you would need a conditional - the following works for me with CMake:

set(foo "")
list(SORT foo)

[codebot]

at least on cmake 2.8.12.2, it prints this:

CMake Error at /home/mquigley/ros2_ws/install/share/rmw_implementation/cmake/get_available_rmw_implementations.cmake:23 (list):
list sub-command SORT requires list to be present.

i assumed that meant that the parameter to list(SORT) was empty, but perhaps it's a different failure mode

[codebot]

Ah, I think the difference is that variable in this case isn't defined: it's not an empty string, it's undefined.

[codebot]

sorry, i'm just really confused. I think it is actually defined, but it's indeed a zero-length string.

actually, i have no idea what's going on

[dirk-thomas]

While it is being set in https://github.com/ament/ament_cmake/blob/9048899acba0a095dfbddf7f2645d3c8f2937bfa/ament_cmake_core/cmake/index/ament_index_get_resources.cmake#L60 it looks like that the variable is not defined in the parent scope afterwards.

A simple test proves that:

set(foo "")
if(DEFINED foo)
  message("foo defined")
endif()
list(SORT foo)
message("sort ok")

function(func)
  set(foo "" PARENT_SCOPE)
endfunction()
func()
if(NOT DEFINED foo)
  message("foo NOT defined")
endif()
list(SORT foo)

[dirk-thomas]

The easiest fix is probable to add the following lines after the ament_index_get_resources() invocation:

# ensure that the variable is defined if the function tries to set an empty variable in the parent scope
set(middleware_implementations "${middleware_implementations}")

[codebot]

ok thanks, that indeed makes it behave as expected. I'll update the PR.

[codebot]

what I still don't quite understand is that, unless you include that set() call you suggest, the following conditional doesn't get triggered when there aren't any rmw implementations:

  if("${middleware_implementations} " STREQUAL " ")
    message(FATAL_ERROR "Could not find any ROS middleware implementation.")
  endif()

I thought that the STREQUAL would be true even if ${middleware_implementations} was undefined. Since it doesn't go into that conditional, it keeps plowing onward, trying to find_package() an empty string, etc.

[dirk-thomas]

Yes, https://github.com/ros2/rmw_implementation/pull/2/files#diff-9e242b1962e7673fca91ac5667e8e57cL27 will result in a fatal error message when middleware_implementations is either empty or undefined.

The error message is intended if you ask for the default implementation and there is none. But there should be no fatal error when only asking for a list of available rmw implementations.