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

Backporting features from ament_cmake_extension to ament_cmake_auto #453

Open
VRichardJP opened this issue May 23, 2023 · 0 comments
Open

Comments

@VRichardJP
Copy link
Contributor

VRichardJP commented May 23, 2023

After I discovered the performance issue with ament_auto macros, I got interested in Ament and CMake targets and started to write my own "auto" macros in a package named ament_cmake_extension. To put it simply, it is a stripped down version of ament_cmake_auto, to which I have added a couple features that I found made sense, in particular exporting CMake targets to improve build times. I used Autoware as toy project to test and implement new features, and over the last few weeks I think I have managed to build something rather solid and reliable. As much I like reinventing the wheel and playing with new ideas on my side, ament_cmake_extension serves the same purpose than ament_cmake_auto and I would be happy to see some/most of the features I came up to added back into ament_cmake_auto (and keep using the auto macros instead).

In this ticket, I want to list the similarities and differences between the 2 packages, in particular the reason I came up with this or that change, so that we could eventually discuss what feature could make sense in ament_cmake_auto.

TL;DR

The features I think would make sense in ament_cmake_auto:

CMake target export support:

  • add option (e.g. EXPORT_LIBRARY_TARGETS) in ament_auto_package() to export all libraries as CMake targets. Must be optional because many ament_auto packages create targets that are ill-formed for export (based on my experience with autoware packages)

Fixes:

  • install ./include directory properly by default (for exported targets).

QoL improvements for CMake target export:

  • add documentation or ament_ex_find_package() like function to automatically export imported packages downstream.
  • add ament_ex_install_targets() and ament_ex_target_add_package_dependencies() like functions do deal with targets that cannot use the ament_auto wrappers directly.
  • add ament_ex_target_include_directories() like function to easily include and install extra directories (for exported targets)

ament_ex Macros

Most ament_ex macros are based on existing ament_auto macros, so they share many similarities. The only "auto" macros that have no equivalent in ament_cmake_extension are ament_auto_add_gtest() (because there are millions of test wrappers, not just ament_add_gtest_executable) and ament_auto_generate_code() (because I have no idea what it is used for).

From user PoV, these macros are equivalent:

  • ament_ex_add_executable() ~= ament_auto_add_executable()
  • ament_ex_add_library() ~= ament_auto_add_library()
  • ament_ex_find_all_package_dependencies() ~= ament_auto_find_build_dependencies() + ament_auto_find_test_dependencies() if BUILD_TESTING is set.

Other ament_ex macros differ significantly from their ament_auto counterpart (if there is any). Full detail below.

ament_ex_find_package()

  • ament_ex_find_package() = find_package() + ament_export_dependencies()

Although the macro is extremely simple, I have realized many people forget to call ament_export_dependencies() when they import and link system packages (e.g. Eigen, PCL) in their shared libraries. This cause issue in downstream packages.

Just a foolproof macro. Some documentation could also do.

ament_ex_install_targets()

  • used by ament_ex_add_executable() and ament_ex_add_library() to install their targets
  • can also be used by the user to install targets that cannot use the ament_ex wrapper: for example tests or cuda libraries.

I find this macro very convenient when you need it for the several use cases above. There is no equivalent in the ament_auto macros, besides hacking ament_auto internal variables. For example: list(APPEND ${PROJECT_NAME}_LIBRARIES "${target}"). One thing I find problematic with ament_auto macros is the lack of flexibility: it's often all or nothing. This macro helps to break down the logic a bit.

ament_ex_package()

  • ament_ex_package() ~= ament_auto_package()
  • ament_ex version exports targets instead of libraries
  • ament_ex does not install ./include directories nor targets (this is done earlier by other macros)

This macro introduces 2 breaking changes:

  • ament_ex packages do not install ./include directory if no target is built. Header only packages must create an INTERFACE library to trigger the install/export. This is purely because I wanted to have "clean" CMake target everywhere, but I am not sure it brings anything from the user PoV.
  • ament_ex exports targets, for which extra care is required. In particular, include directories must be installed properly and dependencies must be exported, otherwise building the package will fail. Most of the other changes I have introduced to ament_cmake_extension are just make this step simpler.

ament_ex_target_add_package_dependencies()

  • used by ament_ex_add_executable() and ament_ex_add_library() to install include/link build dependencies
  • can also be used by the user to include/link build and/or test dependencies to targets that cannot use the ament_ex wrapper: for example tests or cuda libraries.

Often used along with ament_ex_install_targets(), I find this macro quite convenient as well. There is no equivalent among the ament_auto macros. Again, it gives more flexibility.

ament_ex_target_include_directories()

  • used by ament_ex_add_library() to properly include and install the ./include directory
  • can also be used by the user to easily include other directories

Although quite rare, I have seen cases where people need to include other directories than ./include. CMake being CMake, it is very easy not to install the include properly for exported targets. Again it just gives more flexibility.

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

No branches or pull requests

1 participant