-
Notifications
You must be signed in to change notification settings - Fork 117
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
feat: support components in ament_export_dependencies #457
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: Vincent Richard <richard-v@macnica.co.jp>
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'm sorry for the long silence here.
Overall, I think the idea of exporting COMPONENTS downstream is a good one. It has the benefits you describe in the initial comment.
I think this implementation could be improved. In particular:
- Whether you add in the COMPONENTS keyword or not to
ament_export_dependencies
changes how it processes things, which I think is a strange interface. I think we should either add in a new macro (ament_export_component_dependencies
), or possibly expose the:
syntax to the user (so they could doament_export_dependencies(pkg1 pkg2:component1:component2 pkg3)
. The downside to these suggestions is that they are "different" from what plain CMake does, but I'm not sure we can fix that now. - The fact that we are using the
:
as the component separator betweenament_export_dependencies
andament_cmake_export_dependencies-extras.cmake.in
will make it more difficult to debug in the future. Someone looking at this will somehow have to know that the colcon is special and means "component". I don't currently have a better proposal here, but I do think it bears thinking about more.
@clalancette Thank you for looking into this.
I would like to argue this is the case of pretty much every single cmake function :-). Personnaly, I think implementing the behavior in find_package(libA)
find_package(libB)
find_package(libC COMPONENTS compX compY)
# ...
ament_export_dependencies(libA)
ament_export_dependencies(libB)
ament_export_dependencies(libC COMPONENTS compX compY)
# ?
# ament_export_component_dependencies(libC COMPONENTS compX compY) Hence the I know the current I agree, using In the library deduplicate functions, the variable |
It is used, pretty heavily. And that's the basis of my argument. If it only ever allowed a single argument (like |
I understand. In that case, I guess the find_package(libA)
find_package(libB)
find_package(libC COMPONENTS compX compY)
ament_export_dependencies(libA) # OK
ament_export_component_dependencies(libB) # Not OK
ament_export_component_dependencies(libC COMPONENTS compX compY) # OK |
We talked about this a bit more in the ROS core developers meeting, and we came up with a different proposal. In particular, we are going to suggest adding in a new macro which is This has the benefit of acting much more like What do you think? |
Consider making sure the new function supports PkgConfig. See #504. This would be a departure because |
I'm not sure that is something we really want to do, to be honest. Regardless, I think we should do it separately to keep the scope here smaller. |
@clalancette Totally agree Regarding the |
If we are going to introduce
And that way we don't need to use any special tricks here. Does that make sense? |
I am not sure how this could work, without introducing something like the For example, what should be state of these variables with this:
With the PR implementation we get With only a single variable to store the components, I don't see how we can track which component belongs to which package The only alternative I see to a flat list (i.e. with the
|
Oh, I see. I misunderstood how this worked. Thanks for the explanation. I agree with you that we need to do something else then. In that case, I'm OK with the |
I'm personally in favor of separate variables for each package instead of the |
This matches find_package interface variable It also matches the module component behavior of As long as the variables follow a predictable naming schema, seems like a reasonable approach. |
As explained here: #456
Test
For example, I have replaced all
find_package()
call in the autoware.universe project by the following:This macro simply make sure that all fetched packages are correctly exported downstream.
Without this feature, the macro does not care about components. For example
![image](https://private-user-images.githubusercontent.com/18645627/241841217-28f9f0fe-d3ee-487a-9a02-2ecf99cab8b7.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTgwNTAwNzIsIm5iZiI6MTcxODA0OTc3MiwicGF0aCI6Ii8xODY0NTYyNy8yNDE4NDEyMTctMjhmOWYwZmUtZDNlZS00ODdhLTlhMDItMmVjZjk5Y2FiOGI3LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA2MTAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNjEwVDIwMDI1MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTcxM2QxOWUxZmEzODhlMmM5YjkzNjUxOTY5ZjRiMjdjZmQxOGRmMjA5MmE2MTU2YWYxNTU1ZDRiNjNhZTVmZDYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.Y6LkhMBdioVLP9JwEp8sy0Ufi_C3rGZpJcFySdLIqLA)
ament_auto_find_package(PCL REQUIRED COMPONENTS common)
would eventually callament_export_dependencies(PCL)
, which means downstream packages will fetch all PCL components instead of justcommon
. For example, on the packagebehavior_path_planner
, cmake execution flame graph looks like this:With the feature (e.g. ``ament_export_dependencies(PCL COMPONENTS common)`), it looks like this:
![image](https://private-user-images.githubusercontent.com/18645627/241841457-3f86b0b4-29ba-487b-ae5c-e47405bdb0c7.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTgwNTAwNzIsIm5iZiI6MTcxODA0OTc3MiwicGF0aCI6Ii8xODY0NTYyNy8yNDE4NDE0NTctM2Y4NmIwYjQtMjliYS00ODdiLWFlNWMtZTQ3NDA1YmRiMGM3LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA2MTAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNjEwVDIwMDI1MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTJjOGRjNzAwYzk1YTlkNzk1ZmIxZWYwZDIzNGY5ZTJiNGEzMDJmODE0N2YzMGU4ZWVjMzk1OTlhMWI5NjVhMmImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.QWVjSycGo95K08Dtnv2HRhLycAtmNUcac3VP41mJAqw)
As you can see in the first graph, VTK library is being fetched, although it is never used in the whole project. This is because if you can only do
ament_export_dependencies(PCL)
, then all downstream package will always fetch all PCL components. On the second graph, the VTK library does not appear, because all upstream packages only ever used limited components (common
,io
, etc).In this toy example, cmake configuration is just faster. But it would also allow people to write things such as
ament_export_dependencies(Boost COMPONENTS thread)
.