-
Notifications
You must be signed in to change notification settings - Fork 143
feat: add option to export libraries as cmake targets #455
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
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: Vincent Richard <[email protected]>
|
Thank you for investigating this. To be precise, I don't think this PR should break anything dowstream itself. There are only 2 changes here:
If this PR was to break anything, I think it would show at build time, no during tests. |
|
Belated I realized that this will have almost no effect on the ROS 2 core, since we don't use |
|
With the deprecation of What is missing to get this merged? |
Signed-off-by: Vincent Richard <[email protected]>
|
If anyone is interested I have updated the branch. However I don't do much ROS recently so I don't have a test example at hand. |
| # export all targets for downstream packages | ||
| if(NOT "${${PROJECT_NAME}_TARGETS}" STREQUAL "") | ||
| install( | ||
| TARGETS ${without_interfaces} | ||
| TARGETS ${${PROJECT_NAME}_TARGETS} | ||
| EXPORT ${PROJECT_NAME}Targets | ||
| ARCHIVE DESTINATION lib | ||
| LIBRARY DESTINATION lib | ||
| RUNTIME DESTINATION bin | ||
| ) | ||
| ament_export_targets(${PROJECT_NAME}Targets) | ||
| endif() |
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.
@VRichardJP I know you said you don't have the ability to test this and that you've moved on, but if you have the time and context still, does this conditional install need to be outside of the above if-else?
As I read the code, one thing that definitely changed is that we try to install ${${PROJECT_NAME}_TARGETS} even if you don't use EXPORT_LIBRARY_TARGETS. If we were able to move this block up into the if (_ARG_AMENT_AUTO_PACKAGE_EXPORT_LIBRARY_TARGETS) clause, then this change becomes a very safe one (EXPORT_LIBRARY_TARGETS gives you a new behavior and without it you get the old behavior as the default).
But if you think it needs to be outside the clause then I'll have to study it closer. I don't think anything else is using ${${PROJECT_NAME}_TARGETS}, but I need to spend more time on it.
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 understand your point.
As I look back at the code, I think I have implemented it this way because it is simply more future proof. If you consider that more scripts may add targets to ${PROJECT_NAME}_TARGETS in the future (e.g. binaries as target too), and that targets should only be exported once (if any), then I think it makes more sense to keep the target exports separate.
Basically, this PR adds support for target export, and uses that new feature to implement a new export library as target option.
Add
EXPORT_LIBRARY_TARGETSflag toament_auto_package()macro.When the flag is provided, libraries are exported using their cmake target instead of going through ament home-made library export logic.
Although exporting libraries this way is stricter, it makes cmake invocations faster on downstream packages.
Example on Autoware's behavior path planner:
Before: 112s

After: 26s

Note: In my original experimentation, cmake time on this package went down to 17s. I have not checked exactly what is slowing things down yet.
ament_exmacros are still way faster (8s on this package).This feature has been experimented on this autoware branch: autowarefoundation/autoware_universe@f3b6226
Note that this experimental branch also leverages extra macros that are not included in this PR, found here: 450b6de
See #453 for more details