Fail if RMW_IMPLEMENTATION is set but doesn't match (#198)

* If RMW_IMPLEMENTATION is set, it must match
rmw_get_implementation_identifier

* Refactor so easier to read

* Add tests

* Use strdup from rcutils

* Don't assume retrieved rmw id is not null

* Add comment clarifying why a check is needed

* Fix long line in test

* Remove test that wouldn't work with multiple rmw impls

* warning -> error

* Check if test exists before setting properties

* Copyright year

* only build executable once

* hopefully fix tests for windows

* Check return code of rcl_init

* Remove another test that wasn't supposed to pass w/ multiple rmw impls

We aren't explicitly setting RMW_IMPLEMENTATION in the cmake for this test (unlike the others)

* Skip tests on windows because of https://github.com/ros2/launch/issues/66

rcl/src/rcl/rmw_implementation_identifier_check.c

@@ -21,8 +21,10 @@ extern "C"
 #include <stdlib.h>
 #include <string.h>
 
+#include "rcl/allocator.h"
 #include "rcl/error_handling.h"
 #include "rcutils/logging_macros.h"
+#include "rcutils/strdup.h"
 #include "rmw/rmw.h"
 
 #include "rcl/types.h"
@@ -51,10 +53,33 @@ extern "C"
 #endif
 
 INITIALIZER(initialize) {
-  // If the environement variable RCL_ASSERT_RMW_ID_MATCHES is set,
+  // If the environment variable RMW_IMPLEMENTATION is set, or
+  // the environment variable RCL_ASSERT_RMW_ID_MATCHES is set,
   // check that the result of `rmw_get_implementation_identifier` matches.
-  const char * expected = NULL;
+  rcl_allocator_t allocator = rcl_get_default_allocator();
-  rcl_ret_t ret = rcl_impl_getenv("RCL_ASSERT_RMW_ID_MATCHES", &expected);
+  char * expected_rmw_impl = NULL;
+  const char * expected_rmw_impl_env = NULL;
+  rcl_ret_t ret = rcl_impl_getenv("RMW_IMPLEMENTATION", &expected_rmw_impl_env);
+  if (ret != RCL_RET_OK) {
+    RCUTILS_LOG_ERROR_NAMED(
+      ROS_PACKAGE_NAME,
+      "Error getting environement variable 'RMW_IMPLEMENTATION': %s",
+      rcl_get_error_string_safe()
+    )
+    exit(ret);
+  }
+  if (strlen(expected_rmw_impl_env) > 0) {
+    // Copy the environment variable so it doesn't get over-written by the next getenv call.
+    expected_rmw_impl = rcutils_strdup(expected_rmw_impl_env, allocator);
+    if (!expected_rmw_impl) {
+      RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "allocation failed")
+      exit(RCL_RET_BAD_ALLOC);
+    }
+  }
+
+  char * asserted_rmw_impl = NULL;
+  const char * asserted_rmw_impl_env = NULL;
+  ret = rcl_impl_getenv("RCL_ASSERT_RMW_ID_MATCHES", &asserted_rmw_impl_env);
   if (ret != RCL_RET_OK) {
     RCUTILS_LOG_ERROR_NAMED(
       ROS_PACKAGE_NAME,
@@ -63,16 +88,62 @@ INITIALIZER(initialize) {
     )
     exit(ret);
   }
-  // If the environment variable is set, and it does not match, print a warning and exit.
+  if (strlen(asserted_rmw_impl_env) > 0) {
-  if (strlen(expected) > 0 && strcmp(rmw_get_implementation_identifier(), expected) != 0) {
+    // Copy the environment variable so it doesn't get over-written by the next getenv call.
+    asserted_rmw_impl = rcutils_strdup(asserted_rmw_impl_env, allocator);
+    if (!asserted_rmw_impl) {
+      RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "allocation failed")
+      exit(RCL_RET_BAD_ALLOC);
+    }
+  }
+
+  // If both environment variables are set, and they do not match, print an error and exit.
+  if (expected_rmw_impl && asserted_rmw_impl && strcmp(expected_rmw_impl, asserted_rmw_impl) != 0) {
     RCUTILS_LOG_ERROR_NAMED(
       ROS_PACKAGE_NAME,
-      "Expected RMW implementation identifier of '%s' but instead found '%s', exiting with %d.",
+      "Values of RMW_IMPLEMENTATION ('%s') and RCL_ASSERT_RMW_ID_MATCHES ('%s') environment "
-      expected,
+      "variables do not match, exiting with %d.",
-      rmw_get_implementation_identifier(),
+      expected_rmw_impl, asserted_rmw_impl, RCL_RET_ERROR
-      RCL_RET_MISMATCHED_RMW_ID
     )
-    exit(RCL_RET_MISMATCHED_RMW_ID);
+    exit(RCL_RET_ERROR);
+  }
+
+  // Collapse the expected_rmw_impl and asserted_rmw_impl variables so only expected_rmw_impl needs
+  // to be used from now on.
+  if (expected_rmw_impl && asserted_rmw_impl) {
+    // The strings at this point must be equal.
+    // No need for asserted_rmw_impl anymore, free the memory.
+    allocator.deallocate((char *)asserted_rmw_impl, allocator.state);
+  } else {
+    // One or none are set.
+    // If asserted_rmw_impl has contents, move it over to expected_rmw_impl.
+    if (asserted_rmw_impl) {
+      expected_rmw_impl = asserted_rmw_impl;
+    }
+  }
+
+  // If either environment variable is set, and it does not match, print an error and exit.
+  if (expected_rmw_impl) {
+    const char * actual_rmw_impl_id = rmw_get_implementation_identifier();
+    if (!actual_rmw_impl_id) {
+      RCUTILS_LOG_ERROR_NAMED(
+        ROS_PACKAGE_NAME,
+        "Error getting RMW implementation identifier."
+      )
+      exit(RCL_RET_ERROR);
+    }
+    if (strcmp(actual_rmw_impl_id, expected_rmw_impl) != 0) {
+      RCUTILS_LOG_ERROR_NAMED(
+        ROS_PACKAGE_NAME,
+        "Expected RMW implementation identifier of '%s' but instead found '%s', exiting with %d.",
+        expected_rmw_impl,
+        actual_rmw_impl_id,
+        RCL_RET_MISMATCHED_RMW_ID
+      )
+      exit(RCL_RET_MISMATCHED_RMW_ID);
+    }
+    // Free the memory now that all checking has passed.
+    allocator.deallocate((char *)expected_rmw_impl, allocator.state);
   }
 }
 

rcl/test/CMakeLists.txt

@@ -176,6 +176,36 @@ function(test_target_function)
     APPEND_LIBRARY_DIRS ${extra_lib_dirs}
     TIMEOUT 15
   )
+
+  set(SKIP_TEST "")
+  if(WIN32)
+    # TODO(dhood): launch does not set the return code correctly for these tests on Windows.
+    # See https://github.com/ros2/launch/issues/66
+    set(SKIP_TEST "SKIP_TEST")
+  endif()
+  set(TEST_RMW_IMPL_ID_CHECK_EXECUTABLE_NAME "$<TARGET_FILE:test_rmw_impl_id_check_exe>")
+  configure_file(
+    rcl/test_rmw_impl_id_check.py.in
+    ${CMAKE_CURRENT_BINARY_DIR}/test_rmw_impl_id_check${target_suffix}.py.configure
+    @ONLY
+  )
+  file(GENERATE
+    OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/test/test_rmw_impl_id_check${target_suffix}_$<CONFIG>.py"
+    INPUT "${CMAKE_CURRENT_BINARY_DIR}/test_rmw_impl_id_check${target_suffix}.py.configure"
+  )
+  ament_add_pytest_test(
+    test_rmw_impl_id_check${target_suffix}
+    "${CMAKE_CURRENT_BINARY_DIR}/test/test_rmw_impl_id_check${target_suffix}_$<CONFIG>.py"
+    APPEND_LIBRARY_DIRS "${extra_lib_dirs}"
+    ${SKIP_TEST}
+  )
+  if(TEST test_rmw_impl_id_check${target_suffix})
+    set_tests_properties(
+      test_rmw_impl_id_check${target_suffix}
+      PROPERTIES DEPENDS test_rmw_impl_id_check_exe
+    )
+  endif()
+
 endfunction()
 
 macro(connext_workaround target)
@@ -189,6 +219,11 @@ macro(connext_workaround target)
   endif()
 endmacro()
 
+# Build simple executable for using in the test_rmw_impl_id_check
+add_executable(test_rmw_impl_id_check_exe
+  rcl/test_rmw_impl_id_check_exe.cpp)
+target_link_libraries(test_rmw_impl_id_check_exe ${PROJECT_NAME})
+
 call_for_each_rmw_implementation(test_target)
 
 rcl_add_custom_gtest(test_validate_topic_name

rcl/test/rcl/test_rmw_impl_id_check.py.in

@@ -0,0 +1,76 @@
+# generated from rcl/test/test_rmw_impl_id_check.py.in
+
+import os
+
+from launch import LaunchDescriptor
+from launch.launcher import DefaultLauncher
+
+
+def launch_test(
+    rmw_implementation_env=None, rcl_assert_rmw_id_matches_env=None, expect_failure=False
+):
+    ld = LaunchDescriptor()
+
+    env = dict(os.environ)
+    if rmw_implementation_env is not None:
+      env['RMW_IMPLEMENTATION'] = rmw_implementation_env
+    if rcl_assert_rmw_id_matches_env is not None:
+      env['RCL_ASSERT_RMW_ID_MATCHES'] = rcl_assert_rmw_id_matches_env
+
+    ld.add_process(
+        cmd=['@TEST_RMW_IMPL_ID_CHECK_EXECUTABLE_NAME@'],
+        name='@TEST_RMW_IMPL_ID_CHECK_EXECUTABLE_NAME@',
+        env=env,
+    )
+
+    launcher = DefaultLauncher()
+    launcher.add_launch_descriptor(ld)
+    rc = launcher.launch()
+
+    if expect_failure:
+      assert rc != 0, 'The executable did not fail as expected.'
+    else:
+      assert rc == 0, "The executable failed with exit code '" + str(rc) + "'. "
+
+
+def test_rmw_implementation_env():
+    launch_test(rmw_implementation_env='@rmw_implementation@', expect_failure=False)
+    launch_test(rmw_implementation_env='', expect_failure=False)
+    launch_test(rmw_implementation_env='garbage', expect_failure=True)
+
+
+def test_rcl_assert_rmw_id_matches_env():
+    # Note(dhood): we don't test _only_ setting RCL_ASSERT_RMW_ID_MATCHES because if support for
+    # multiple RMW implementations is available then RMW_IMPLEMENTATION must be used in order to
+    # get non-default RMW implementation(s).
+    launch_test(rcl_assert_rmw_id_matches_env='', expect_failure=False)
+    launch_test(rcl_assert_rmw_id_matches_env='garbage', expect_failure=True)
+
+
+def test_both():
+    launch_test(
+        rmw_implementation_env='@rmw_implementation@',
+        rcl_assert_rmw_id_matches_env='@rmw_implementation@',
+        expect_failure=False)
+    launch_test(
+        rmw_implementation_env='',
+        rcl_assert_rmw_id_matches_env='',
+        expect_failure=False)
+    launch_test(
+        rmw_implementation_env='@rmw_implementation@',
+        rcl_assert_rmw_id_matches_env='garbage',
+        expect_failure=True)
+    launch_test(
+        rmw_implementation_env='garbage',
+        rcl_assert_rmw_id_matches_env='@rmw_implementation@',
+        expect_failure=True)
+    launch_test(
+        rmw_implementation_env='garbage',
+        rcl_assert_rmw_id_matches_env='garbage',
+        expect_failure=True)
+
+
+if __name__ == '__main__':
+    test_rmw_impl_env()
+    test_rcl_assert_rmw_id_matches_env()
+    test_both()

rcl/test/rcl/test_rmw_impl_id_check_exe.cpp

@@ -0,0 +1,24 @@
+// Copyright 2017 Open Source Robotics Foundation, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "rcl/rcl.h"
+
+int main(int, char **)
+{
+  rcl_ret_t ret = rcl_init(0, nullptr, rcl_get_default_allocator());
+  if (ret != RCL_RET_OK) {
+    return ret;
+  }
+  return 0;
+}