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

Nested include directories complicates header inclusion logic #964

Open
russkel opened this issue Mar 7, 2022 · 9 comments
Open

Nested include directories complicates header inclusion logic #964

russkel opened this issue Mar 7, 2022 · 9 comments
Labels

Comments

@russkel
Copy link

russkel commented Mar 7, 2022

Bug report

As mentioned by @pablogs9 originally in #959 (comment) the added PROJECT_NAME to include paths complicates other build logic that doesn't rely on cmake files.

Required Info:

  • Operating System:
    • Ubuntu 21.10
  • Installation type:
    • From source using rosinstall
  • Version or commit hash:
    • version: release/rolling/rcl/5.1.0-1

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
Copy link
Contributor

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 ros2/ros2#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
Copy link
Author

russkel commented Mar 7, 2022

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
Copy link
Contributor

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
Copy link
Author

russkel commented Mar 7, 2022

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
Copy link
Contributor

sloretz commented Mar 7, 2022

@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
Copy link
Author

russkel commented Mar 9, 2022

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
Copy link
Contributor

sloretz commented Mar 9, 2022

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
Copy link
Author

russkel commented Mar 9, 2022

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
Copy link
Contributor

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.

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

No branches or pull requests

3 participants