Make library attributes visible

[nnmm]

This change was requested in ros2/rclcpp#1452 (comment): have a central place for these attributes instead of duplicating them in rclcpp.

Signed-off-by: Nikolai Morin nikolai.morin@apex.ai

[nnmm]

@wjwwood Could you review?

[wjwwood]

I think it would be better if this were exposed via a function(s), but I don't have a strong reason why.

@clalancette @mjeronimo any feedback (as maintainers)?

[emersonknapp]

This seems fine - though my personal preference would be to expose a new function find_library_in_path - and keep these implementation details hidden

[clalancette]

This seems fine - though my personal preference would be to expose a new function find_library_in_path - and keep these implementation details hidden

Exposing these details seems pretty ugly. I have to agree with @wjwwood and @emersonknapp that a function would be much nicer here.

[nnmm]

@clalancette @emersonknapp find_library_in_path() does not do what I'm interested in. What rclcpp_generic does is

auto library_path = package_prefix + dynamic_library_folder + rcpputils::kSolibPrefix +
    package_name + "__" + typesupport_identifier + rcpputils::kSolibExtension;

(dynamic_library_folder is also platform-specific, but I assume it's a ROS convention that doesn't generalize)

So the only function that we could extract into here is something like affix_library_name(), which adds the prefix and the extension. Would you prefer that?

[mjeronimo]

It does seem that there is a function or two that wants to come out rather than exposing the platform details. I'm for encapsulating the platform-specifics in the implementation file and providing platform independent functions in the header.

[nnmm]

I changed it to expose only a function.

[clalancette]

Oh, sorry. Can you just add one test to cover this? Should be a simple one.

[nnmm]

@clalancette It is covered by the existing test since I modified find_library_path to use filename_for_library, does that count? Otherwise it's a bit tricky – for an exact test, I'd need to know which platforms this test runs on to construct an expected output, and the test would just be a reimplementation of the function.

[clalancette]

@clalancette It is covered by the existing test since I modified find_library_path to use filename_for_library, does that count?

Oh yeah, that counts. OK, fine by me. I'll add my approval, but you should take care of @emersonknapp 's comments as well.

[emersonknapp]

Gist: https://gist.githubusercontent.com/emersonknapp/d8fce7350957a70f43a565b4f7f62c21/raw/b486b71a66a8779f7512fbdf779c11b71c4c5544/ros2.repos
BUILD args: --packages-up-to rcpputils
TEST args: --packages-select rcpputils
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/7987

• Linux
• Linux-aarch64
• macOS
• Windows

[nnmm]

@emersonknapp I assume that means it's good to merge?

[wjwwood]

@emersonknapp I'm gonna merge this so we can move the other pr forward. 👍