ros2 / rcl

Library to support implementation of language specific ROS Client Libraries.
Apache License 2.0
128 stars 162 forks source link

Nested include directories complicates header inclusion logic #964

Open russkel opened 2 years ago

russkel commented 2 years ago

Bug report

As mentioned by @pablogs9 originally in https://github.com/ros2/rcl/pull/959#issuecomment-1021193235 the added PROJECT_NAME to include paths complicates other build logic that doesn't rely on cmake files.

Required Info:

Steps to reproduce issue

sudo apt install python3-rosinstall-generator
rosinstall_generator ros_core --rosdistro rolling --deps > rolling-ros-core.rosinstall
vcs import src < rolling-ros-core.rosinstall
sudo rosdep init
rosdep update
rosdep install --from-paths src --ignore-src -y --skip-keys "fastcdr rti-connext-dds-6.0.1 urdfdom_headers"
colcon build --merge-install
cd install

Expected behavior

Non-nested rcl directory, i.e. include/rcl/context.h etc

Actual behavior

$ russ @ xxx in ~/work/ros2_rolling/install [0:12:02]
↳ find include | grep rcl/
include/rcl/rcl
include/rcl/rcl/context.h
include/rcl/rcl/event_callback.h
include/rcl/rcl/service.h
include/rcl/rcl/domain_id.h
include/rcl/rcl/log_level.h
include/rcl/rcl/subscription.h
include/rcl/rcl/validate_enclave_name.h
include/rcl/rcl/allocator.h
include/rcl/rcl/arguments.h
include/rcl/rcl/client.h
include/rcl/rcl/rmw_implementation_identifier_check.h
include/rcl/rcl/publisher.h
include/rcl/rcl/remap.h
include/rcl/rcl/macros.h
include/rcl/rcl/logging_rosout.h
include/rcl/rcl/init_options.h
include/rcl/rcl/init.h
include/rcl/rcl/wait.h
include/rcl/rcl/expand_topic_name.h
include/rcl/rcl/rcl.h
include/rcl/rcl/types.h
include/rcl/rcl/guard_condition.h
include/rcl/rcl/network_flow_endpoints.h
include/rcl/rcl/lexer.h
include/rcl/rcl/visibility_control.h
include/rcl/rcl/timer.h
include/rcl/rcl/lexer_lookahead.h
include/rcl/rcl/validate_topic_name.h
include/rcl/rcl/security.h
include/rcl/rcl/event.h
include/rcl/rcl/graph.h
include/rcl/rcl/logging.h
include/rcl/rcl/node_options.h
include/rcl/rcl/time.h
include/rcl/rcl/localhost.h
include/rcl/rcl/node.h
include/rcl/rcl/error_handling.h

The above shows a nested rcl directory.

Additional information


Feature request

Feature description

Implementation considerations

clalancette commented 2 years ago

So I guess we need more to know about your use-case. We intentionally moved these down a directory level (see the long discussion at https://github.com/ros2/ros2/issues/1150), and this solves a real problem that users are having. We've modified the CMake logic in the core to handle all of this, so the change should be transparent to users. The question is why this is causing trouble for you and your project, and what steps we can take to mitigate that.

russkel commented 2 years ago

Okay, I am using ros2 libraries within Unreal Engine so we can tx/rx ROS2 msgs from within Unreal projects. Currently this (a fork of rclUE) is how I am including the headers https://github.com/Greenroom-Robotics/rclUE/blob/whiskey/Source/rclUE/rclUE.Build.cs#L50

These changes complicates things slightly, and short of writing a Cmake file parser I'm not sure what the elegant solution would be.

clalancette commented 2 years ago

OK, thanks, that helps. We'll take a look and see if there is anything we can do to help your use-case. @sloretz when you get a chance can you take a look and provide any thoughts on how to find the include path from non-CMake systems?

russkel commented 2 years ago

Thanks @clalancette!

This is my current work around, it works for the subset of packages this uses, but I imagine it's quite brittle.

                PublicIncludePaths.Add(Path.Combine(ROS2InstallPath, "include"));

                // hack to get around the change in include paths in some packages
                foreach (var pkg in rosPackages)
                {
                    if (Directory.Exists(Path.Combine(ROS2InstallPath, "include", pkg, pkg)))
                    {
                        PublicIncludePaths.Add(Path.Combine(ROS2InstallPath, "include", pkg));
                    }
                }   
sloretz commented 2 years ago

@sloretz when you get a chance can you take a look and provide any thoughts on how to find the include path from non-CMake systems?

Sure, but I think this is a question that needs to be turned around. @russkel How does the build system normally get include directories from software that was built using a different build system? This has to be a use case it deals with.

In CMake all software is looked up using find_package(foobar). That exposes targets or variables that have the information needed to build with that software (include directories, compiler flags, libraries etc). Software that is built with CMake usually ships a foobarConfig.cmake file which gets run when the package is find_package()'d. For software that isn't built with CMake, someone has to write a "Find Module" called `Findfoobar.cmake".

Find Modules try to find the software on the system, usually by making educated guesses and checking. For example, a Find Module would call call find_path() to look for a file called foobar/foobar.hpp. The directory containing that file is the include directory. What is Unreal Engine build system's equivalent? Does it have a function to find the directory containing rcl/rcl.h, maybe starting at /opt/ros/<distro>?

russkel commented 2 years ago

How does the build system normally get include directories from software that was built using a different build system? This has to be a use case it deals with.

The way it was done before CMake et al: providing include directories and lib paths.

What is Unreal Engine build system's equivalent? Does it have a function to find the directory containing rcl/rcl.h, maybe starting at /opt/ros/<distro>?

Unreal Build Tool scripts are written in C# (don't ask) so one can do anything like this if they write the code by hand. Ultimately my hack works and lets me carry on with the job, but I wouldn't be opposed to some kind of yaml file that lists include dirs and libs for each package. Reading such a thing would be trivial. Hell, if it's easy enough one could parse the cmake files directly (but from my quick look at that it didn't seem that straight forward).

sloretz commented 2 years ago

The way it was done before CMake et al: providing include directories and lib paths.

Does Unreal Build Tool have helper functions for figuring out where include directories and libraries are?

if it's easy enough one could parse the cmake files directly (but from my quick look at that it didn't seem that straight forward).

Definitely not straightforward. Maybe rclUE could create a temporary CMake package that calls find_package() on all packages depended upon, and have that package print needed target properties to stdout. That doesn't sound fun to write or maintain though.

Unreal Build Tool scripts are written in C# (don't ask) so one can do anything like this if they write the code by hand. Ultimately my hack works and lets me carry on with the job, but I wouldn't be opposed to some kind of yaml file that lists include dirs and libs for each package. Reading such a thing would be trivial.

Do you mean a yaml file provided by each ROS 2 package, or a file hard-coded into rclUE.build.cs? If the former, that seems unlikely to be accepted. However, I bet PRs outputting something standard like a pkg-config file would be well received. Assuming making ROS 2 packages like rcl output them is straightforward, does Unreal Build Tool have the ability to read pkg-config files?

russkel commented 2 years ago

Does Unreal Build Tool have helper functions for figuring out where include directories and libraries are?

A cursory search didn't find anything. Here is a brief overview if you're interested: https://docs.unrealengine.com/4.27/en-US/ProductionPipelines/BuildTools/UnrealBuildTool/ThirdPartyLibraries/

However, I bet PRs outputting something standard like a pkg-config file would be well received. Assuming making ROS 2 packages like rcl output them is straightforward, does Unreal Build Tool have the ability to read pkg-config files?

pkg-config format would work fine and could be trivially parsed. I couldn't find any built in readers but it'd only be a small C# file.

As for cmake generation, this seems to be how it's done in cmake: https://www.scivision.dev/cmake-generate-pkg-config/

clalancette commented 2 years ago

One other thing that I might suggest is using the ament_index for storing this data. We still need to define a file format (so it might be a good idea to reuse an existing one), but then we can store it and look it up via the ament_index. I'm not sure it is a better option than just using pkg-config, but I thought I'd point it out as another possible way to do this.