Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test load and lookup functionality. #135

Merged
merged 3 commits into from
Sep 22, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions rmw_implementation/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -52,18 +52,37 @@ else()

add_library(${PROJECT_NAME} SHARED
src/functions.cpp)
target_include_directories(${PROJECT_NAME} PUBLIC
"$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>"
"$<INSTALL_INTERFACE:include>")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to say that I'm a bit concerned about making this header file "public". If we put this in, then it seems like load_library and lookup_symbol are APIs that downstream consumers should be able to call. And as far as I can tell, that is not the case.

One alternative here is to do something similar to what rviz_default_plugins does. That is, put the header file in the src directory, and don't install it. Then have the tests include the header file via relative paths. I don't love that scheme, but it has the property that it keeps this implementation private (as it looks like it should be), but also makes the symbols available to tests. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion in favor or against exposing rmw implementation lookup.

One alternative here is to do something similar to what rviz_default_plugins does.

I'm OK with that scheme too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though that doesn't change the fact that the symbols will be public.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though that doesn't change the fact that the symbols will be public.

Just to recap a private conversation: while that is true, it is more around the messaging to external consumers. If a function is in a publicly-accessible, installed header, then that seems like something a user might want to call. If it is hidden away in a src directory, and the user has to go looking through symbols to find it, then it is pretty clear they are doing something unsupported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. See b885711. Went the extra mile to only make symbols public if building for testing.

ament_target_dependencies(${PROJECT_NAME}
"rcpputils"
"rcutils"
"rmw")
target_compile_definitions(${PROJECT_NAME}
PUBLIC "DEFAULT_RMW_IMPLEMENTATION=${RMW_IMPLEMENTATION}")

# Causes the visibility macros to use dllexport rather than dllimport,
# which is appropriate when building the dll but not consuming it.
target_compile_definitions(${PROJECT_NAME} PRIVATE "RMW_IMPLEMENTATION_BUILDING_DLL")

configure_rmw_library(${PROJECT_NAME})

ament_export_libraries(${PROJECT_NAME})
ament_export_targets(${PROJECT_NAME})
ament_export_dependencies(rcpputils rcutils)

if(BUILD_TESTING)
find_package(ament_cmake_gtest REQUIRED)
ament_add_gtest(test_functions test/test_functions.cpp)
ament_target_dependencies(test_functions rcutils rmw)
target_link_libraries(test_functions ${PROJECT_NAME})
endif()

install(
DIRECTORY include/
DESTINATION include)

install(
TARGETS ${PROJECT_NAME} EXPORT ${PROJECT_NAME}
ARCHIVE DESTINATION lib
Expand Down
32 changes: 32 additions & 0 deletions rmw_implementation/include/rmw_implementation/functions.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright 2020 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.

#ifndef RMW_IMPLEMENTATION__FUNCTIONS_HPP_
#define RMW_IMPLEMENTATION__FUNCTIONS_HPP_

#include <memory>

#include "rcpputils/shared_library.hpp"

#include "rmw_implementation/visibility_control.h"

RMW_IMPLEMENTATION_PUBLIC
std::shared_ptr<rcpputils::SharedLibrary>
load_library();

RMW_IMPLEMENTATION_PUBLIC
void *
lookup_symbol(std::shared_ptr<rcpputils::SharedLibrary> lib, const char * symbol_name);

#endif // RMW_IMPLEMENTATION__FUNCTIONS_HPP_
58 changes: 58 additions & 0 deletions rmw_implementation/include/rmw_implementation/visibility_control.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// Copyright 2020 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.

#ifndef RMW_IMPLEMENTATION__VISIBILITY_CONTROL_H_
#define RMW_IMPLEMENTATION__VISIBILITY_CONTROL_H_

#ifdef __cplusplus
extern "C"
{
#endif

// This logic was borrowed (then namespaced) from the examples on the gcc wiki:
// https://gcc.gnu.org/wiki/Visibility

#if defined _WIN32 || defined __CYGWIN__
#ifdef __GNUC__
#define RMW_IMPLEMENTATION_EXPORT __attribute__ ((dllexport))
#define RMW_IMPLEMENTATION_IMPORT __attribute__ ((dllimport))
#else
#define RMW_IMPLEMENTATION_EXPORT __declspec(dllexport)
#define RMW_IMPLEMENTATION_IMPORT __declspec(dllimport)
#endif
#ifdef RMW_IMPLEMENTATION_BUILDING_DLL
#define RMW_IMPLEMENTATION_PUBLIC RMW_IMPLEMENTATION_EXPORT
#else
#define RMW_IMPLEMENTATION_PUBLIC RMW_IMPLEMENTATION_IMPORT
#endif
#define RMW_IMPLEMENTATION_PUBLIC_TYPE RMW_IMPLEMENTATION_PUBLIC
#define RMW_IMPLEMENTATION_LOCAL
#else
#define RMW_IMPLEMENTATION_EXPORT __attribute__ ((visibility("default")))
#define RMW_IMPLEMENTATION_IMPORT
#if __GNUC__ >= 4
#define RMW_IMPLEMENTATION_PUBLIC __attribute__ ((visibility("default")))
#define RMW_IMPLEMENTATION_LOCAL __attribute__ ((visibility("hidden")))
#else
#define RMW_IMPLEMENTATION_PUBLIC
#define RMW_IMPLEMENTATION_LOCAL
#endif
#define RMW_IMPLEMENTATION_PUBLIC_TYPE
#endif

#ifdef __cplusplus
}
#endif

#endif // RMW_IMPLEMENTATION__VISIBILITY_CONTROL_H_
1 change: 1 addition & 0 deletions rmw_implementation/package.xml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

<depend>rmw_implementation_cmake</depend>

<test_depend>ament_cmake_gtest</test_depend>
<test_depend>ament_lint_auto</test_depend>
<test_depend>ament_lint_common</test_depend>

Expand Down
105 changes: 66 additions & 39 deletions rmw_implementation/src/functions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
// See the License for the specific language governing permissions and
// limitations under the License.

#include "rmw_implementation/functions.hpp"

#include <cstddef>
#include <stdexcept>

Expand Down Expand Up @@ -39,67 +41,92 @@
#define STRINGIFY_(s) #s
#define STRINGIFY(s) STRINGIFY_(s)

std::shared_ptr<rcpputils::SharedLibrary>
load_library()
{
std::string env_var;
try {
env_var = rcpputils::get_env_var("RMW_IMPLEMENTATION");
} catch (const std::exception & e) {
RMW_SET_ERROR_MSG_WITH_FORMAT_STRING(
"failed to fetch RMW_IMPLEMENTATION "
"from environment due to %s", e.what());
return nullptr;
}

if (env_var.empty()) {
env_var = STRINGIFY(DEFAULT_RMW_IMPLEMENTATION);
}

std::string library_path;
try {
library_path = rcpputils::find_library_path(env_var);
} catch (const std::exception & e) {
RMW_SET_ERROR_MSG_WITH_FORMAT_STRING(
"failed to find shared library due to %s", e.what());
return nullptr;
}

if (library_path.empty()) {
RMW_SET_ERROR_MSG_WITH_FORMAT_STRING(
"failed to find shared library '%s'",
env_var.c_str());
return nullptr;
}

try {
return std::make_shared<rcpputils::SharedLibrary>(library_path.c_str());
} catch (const std::exception & e) {
RMW_SET_ERROR_MSG_WITH_FORMAT_STRING(
"failed to load shared library '%s' due to %s",
library_path.c_str(), e.what());
return nullptr;
}
}

std::shared_ptr<rcpputils::SharedLibrary>
get_library()
{
static std::shared_ptr<rcpputils::SharedLibrary> lib;

if (!lib) {
std::string env_var = rcpputils::get_env_var("RMW_IMPLEMENTATION");
if (env_var.empty()) {
env_var = STRINGIFY(DEFAULT_RMW_IMPLEMENTATION);
}
std::string library_path = rcpputils::find_library_path(env_var);
if (library_path.empty()) {
RMW_SET_ERROR_MSG(
("failed to find shared library of rmw implementation. Searched " + env_var).c_str());
return nullptr;
}

try {
lib = std::make_shared<rcpputils::SharedLibrary>(library_path.c_str());
} catch (const std::runtime_error & e) {
RMW_SET_ERROR_MSG(
("failed to load shared library of rmw implementation: " + library_path +
" Exception: " + std::string(e.what())).c_str());
return nullptr;
} catch (const std::bad_alloc & e) {
RMW_SET_ERROR_MSG(
("failed to load shared library of rmw implementation " + library_path + ": " +
std::string(e.what())).c_str());
return nullptr;
}
lib = load_library();
}
return lib;
}

void *
get_symbol(const char * symbol_name)
lookup_symbol(std::shared_ptr<rcpputils::SharedLibrary> lib, const char * symbol_name)
{
std::shared_ptr<rcpputils::SharedLibrary> lib = get_library();

if (!lib) {
// error message set by get_library()
if (!rmw_error_is_set()) {
RMW_SET_ERROR_MSG("no shared library to lookup");
} // else assume library loading failed
return nullptr;
}

if (!lib->has_symbol(symbol_name)) {
rcutils_allocator_t allocator = rcutils_get_default_allocator();
char * msg = rcutils_format_string(
allocator,
"failed to resolve symbol '%s' in shared library '%s'", symbol_name,
lib->get_library_path().c_str());
if (msg) {
RMW_SET_ERROR_MSG(msg);
allocator.deallocate(msg, allocator.state);
} else {
RMW_SET_ERROR_MSG("failed to allocate memory for error message");
try {
std::string library_path = lib->get_library_path();
RMW_SET_ERROR_MSG_WITH_FORMAT_STRING(
"failed to resolve symbol '%s' in shared library '%s'",
symbol_name, library_path.c_str());
} catch (const std::runtime_error &) {
RMW_SET_ERROR_MSG_WITH_FORMAT_STRING(
"failed to resolve symbol '%s' in shared library",
symbol_name);
}
return nullptr;
}

return lib->get_symbol(symbol_name);
}

void *
get_symbol(const char * symbol_name)
{
return lookup_symbol(get_library(), symbol_name);
}

#ifdef __cplusplus
extern "C"
{
Expand Down
57 changes: 57 additions & 0 deletions rmw_implementation/test/test_functions.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Copyright 2020 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 <gtest/gtest.h>

#include <memory>

#include "rcutils/env.h"
#include "rcutils/get_env.h"
#include "rcutils/testing/fault_injection.h"

#include "rmw/error_handling.h"

#include "rmw_implementation/functions.hpp"

TEST(Functions, bad_load) {
const char * rmw_implementation = nullptr;
ASSERT_EQ(nullptr, rcutils_get_env("RMW_IMPLEMENTATION", &rmw_implementation));
EXPECT_TRUE(rcutils_set_env("RMW_IMPLEMENTATION", "not_an_rmw_implementation"));

EXPECT_EQ(nullptr, load_library());
EXPECT_TRUE(rmw_error_is_set());
rmw_reset_error();

EXPECT_TRUE(rcutils_set_env("RMW_IMPLEMENTATION", rmw_implementation));
}

TEST(Functions, nominal_load_and_lookup) {
std::shared_ptr<rcpputils::SharedLibrary> lib = load_library();
ASSERT_NE(nullptr, lib) << rmw_get_error_string().str;
EXPECT_NE(nullptr, lookup_symbol(lib, "rmw_init")) << rmw_get_error_string().str;
EXPECT_EQ(nullptr, lookup_symbol(lib, "not_an_rmw_function"));
EXPECT_TRUE(rmw_error_is_set());
rmw_reset_error();
}

TEST(Functions, load_and_lookup_with_internal_errors) {
RCUTILS_FAULT_INJECTION_TEST(
{
void * symbol = lookup_symbol(load_library(), "rmw_init");
if (!symbol) {
EXPECT_TRUE(rmw_error_is_set());
rmw_reset_error();
}
});
}