From f0975396d90a13c9cfd8f4019fab1edffb0ca156 Mon Sep 17 00:00:00 2001 From: dhood Date: Tue, 5 Dec 2017 22:07:35 -0800 Subject: [PATCH] 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/rmw_implementation_identifier_check.c | 91 +++++++++++++++++-- rcl/test/CMakeLists.txt | 35 +++++++ rcl/test/rcl/test_rmw_impl_id_check.py.in | 76 ++++++++++++++++ rcl/test/rcl/test_rmw_impl_id_check_exe.cpp | 24 +++++ 4 files changed, 216 insertions(+), 10 deletions(-) create mode 100644 rcl/test/rcl/test_rmw_impl_id_check.py.in create mode 100644 rcl/test/rcl/test_rmw_impl_id_check_exe.cpp diff --git a/rcl/src/rcl/rmw_implementation_identifier_check.c b/rcl/src/rcl/rmw_implementation_identifier_check.c index 49479c4..aee97fb 100644 --- a/rcl/src/rcl/rmw_implementation_identifier_check.c +++ b/rcl/src/rcl/rmw_implementation_identifier_check.c @@ -21,8 +21,10 @@ extern "C" #include #include +#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_ret_t ret = rcl_impl_getenv("RCL_ASSERT_RMW_ID_MATCHES", &expected); + rcl_allocator_t allocator = rcl_get_default_allocator(); + 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(expected) > 0 && strcmp(rmw_get_implementation_identifier(), expected) != 0) { + if (strlen(asserted_rmw_impl_env) > 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.", - expected, - rmw_get_implementation_identifier(), - RCL_RET_MISMATCHED_RMW_ID + "Values of RMW_IMPLEMENTATION ('%s') and RCL_ASSERT_RMW_ID_MATCHES ('%s') environment " + "variables do not match, exiting with %d.", + expected_rmw_impl, asserted_rmw_impl, RCL_RET_ERROR ) - 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); } } diff --git a/rcl/test/CMakeLists.txt b/rcl/test/CMakeLists.txt index f090b36..45b9e1c 100644 --- a/rcl/test/CMakeLists.txt +++ b/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 "$") + 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}_$.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}_$.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 diff --git a/rcl/test/rcl/test_rmw_impl_id_check.py.in b/rcl/test/rcl/test_rmw_impl_id_check.py.in new file mode 100644 index 0000000..dc4649c --- /dev/null +++ b/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() diff --git a/rcl/test/rcl/test_rmw_impl_id_check_exe.cpp b/rcl/test/rcl/test_rmw_impl_id_check_exe.cpp new file mode 100644 index 0000000..5914124 --- /dev/null +++ b/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; +}