-
Notifications
You must be signed in to change notification settings - Fork 974
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
[feature] Add AmentDeps generator for ROS2 support in colcon builds #17055
base: develop2
Are you sure you want to change the base?
Conversation
conan/tools/ros/ament.py
Outdated
|
||
import os | ||
|
||
local_setup_bash = """\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these files coming from?
It is a bit surprising having to embed all this stuff for this to integrate with ros
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The templates can be found at https://github.com/ament/ament_package
Probably some files are not completely necessary, but I'd rather play safe for the moment and mimic the Ament behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to include this by default if we don't really understand what they are doing and we can take ownership of them.
For example other ament package like this one https://github.com/ament/ament_cobra, doesn't seem to include these scripts. What are they doing? are they really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these scripts are automatically generated when building a package with the colcon
and ament
tools, so you won't find them in the source code.
The package.xml
is the only exception (this is needed for building and it's then copied into the install
folder)
Co-authored-by: James <[email protected]>
Co-authored-by: James <[email protected]>
Co-authored-by: James <[email protected]>
Co-authored-by: James <[email protected]>
@danimtb what's the purpose of this generator? When building a ROS 2 package, all this is not necessary (the ROS package will already contain a |
Thanks for your feedback @alsora The goal of this integration is to allow colcon builds to use any existing Conan packages, as automatically as possible. The idea is that a |
Thanks for the explanation, but I don't think you need any of this. Colcon (or ament) won't check for the existence of any of those files that you are creating with the generator while building other packages. For example:
When building package If anything, the The |
Hi @alsora and thank you very much for your comments above. Indeed, many of the files included in the generator are not needed as those are generated by Ament itself. They were added in an early stage of the development of the generator, so I have removed them in the latest commits. Let me explain a little bit the purpose and how this generator is working at the moment. The goeal of the generator is to make the Conan installed dependencies available to a ROS2 package that has external dependencies, like boost for example. So, the way this works is by collecting the required conan dependencies from a conanfile.txt and creating "fake" packages in the colon workspace for only the direct dependencies and then placing the CMake find package files generated by Conan into the install folder in the appropriate path. Let's take the example of a ROS2 package called "consumer" that depends on Conan boost package. This AmentDeps generator will create a "conan_boost" package in the workspace and place the CMake find package files of boost and its transitive dependencies generated by conan into the path I have prepared an examples repo with this case at https://github.com/danimtb/ros2-examples?tab=readme-ov-file#conan_install_ament_example for better understanding. Why do we have this At this point, we have some questions so we can move this forward:
Thanks again for the feedback, this very appreciated for us as we are not ROS experts 😅 |
Thank you for the explanation. It looks to me that you are trying to achieve two different goals here:
For this PR, my main concerns are with respect to the first goal. Calling Note that the dependency built via conan doesn't need a On the other hand, we have the second goal. My main concern with this approach is how to make it coexist with the standard, non-conan, solution for ROS 2 packages that are released.
Lastly, I don't know how the |
Thanks again for the feedback. Yes, this PR focuses on the first goal you mention but also considers the second goal. My initial tests were using CMAKE_PREFIX_PATH to set the Conan installation folder and then invoke colcon build normally and this works with ROS2 and Conan. However, we wanted to achieve a transparent integration and reduce any additional steps for the user when using Conan. That is the reason for converting direct Conan dependencies to "fake" Colcon packages. That way the user does not have to set that CMAKE_PREFIX_PATH. Would ROS users prefer to set the CMAKE_PREFIX_PATH themselves, or better have fewer steps to use Conan packages possible? However, the main drawback of just using CMAKE_PREFIX_PATH is that ament does not interpret correctly this conan dependencies and we have trouble when propagating the transitive dependencies to dowsntream in a ros package that depends from another ros package that has conan dependencies. In that case, if the consumer ros package is a dependency of another ros package called "app", the
Regarding the rosdep integration, I think it makes total sense to have conan packages named as "boost-conan" or similar and that would be opt-in for users having to change that (if they want to use conan) in their package.xml, I think that is reasonable and we don't want to confuse users resolving "system" package manager dependencies to conan dependencies all of a sudden. |
Changelog: Feature: Add Ament generator for ROS2 support in colcon builds
Docs: missing
develop
branch, documenting this one.