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

redundancy: De-duplicate find_library code from RMW libs, move here? #3

Closed
EricCousineau-TRI opened this issue Mar 25, 2019 · 4 comments
Labels
enhancement New feature or request

Comments

@EricCousineau-TRI
Copy link

If I follow the build-from-source instructions here:
https://index.ros.org/doc/ros2/Installation/Linux-Development-Setup/#get-ros-2-0-code

I notice that the find_library code is duplicated 3 times:

$ find src/ -name '*.h' -o -name '*.c' -o -name '*.hpp' -o -name '*.cpp' | xargs grep 'find_library'
src/ros2/rosidl_typesupport/rosidl_typesupport_cpp/src/type_support_dispatch.hpp:std::string find_library_path(const std::string & library_name);
src/ros2/rosidl_typesupport/rosidl_typesupport_cpp/src/type_support_dispatch.hpp:        std::string library_path = find_library_path(library_name);
src/ros2/rosidl_typesupport/rosidl_typesupport_cpp/src/type_support_dispatch.cpp:std::string find_library_path(const std::string & library_name)
src/ros2/rosidl_typesupport/rosidl_typesupport_c/src/type_support_dispatch.hpp:std::string find_library_path(const std::string & library_name);
src/ros2/rosidl_typesupport/rosidl_typesupport_c/src/type_support_dispatch.hpp:        std::string library_path = find_library_path(library_name);
src/ros2/rosidl_typesupport/rosidl_typesupport_c/src/type_support_dispatch.cpp:std::string find_library_path(const std::string & library_name)
src/ros2/rmw_implementation/rmw_implementation/src/functions.cpp:std::string find_library_path(const std::string & library_name)
src/ros2/rmw_implementation/rmw_implementation/src/functions.cpp:    std::string library_path = find_library_path(env_var);

It would be really nice to de-duplicate this code. If possible, could this be hoisted into this library? (I'm guessing re-written in C?)

From perusing this code, it looks there are current conditionals that depend on _WIN32 and __APPLE__, so it seems appropriate?

This effectively comes from: https://raw.githubusercontent.com/ros2/ros2/release-crystal-20190314/ros2.repos
Source: https://github.com/ros2/ros2/blob/8d34d6550a77dcfe4828466e53f0cc5401be77fe/ros2.repos

(Came about while tinkering with consuming libs from Bazel (https://index.ros.org/doc/ros2/Installation/Linux-Development-Setup/#get-ros-2-0-code).)

@dirk-thomas
Copy link
Member

The code could stay C++ and be moved into rcpputils.

@EricCousineau-TRI
Copy link
Author

EricCousineau-TRI commented Mar 25, 2019

Sounds good! Do you want me to migrate this issue there, or keep this one?

I'd be happy to do the legwork and submit some prototype PRs.

@dirk-thomas dirk-thomas transferred this issue from ros2/rcutils Mar 25, 2019
@dirk-thomas
Copy link
Member

Do you want me to migrate this issue there, or keep this one?

Moved 😉

I'd be happy to do the legwork and submit some prototype PRs.

Great!

@ivanpauno
Copy link
Member

I see that #25 was merged, which I think addressed the issue.
Closing! Please re-open if #25 didn't solve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants