ament_vendor: Add AMENT_VENDOR_POLICY CMake option

[traversaro]

Evolution of #552 .

This PR aim is based on our experience at RoboStack with the ament_vendor, and aims to improve the experience of downstream packagers of ROS on non-officially supported platform (being them conda/robostack, nix or similar), reducing the need of patching the vendor package to obtain the required behaviour.

In particular, a common pattern that we are experiencing is that compare to stable linux distributions, rolling releases as conda-forge have much more recent packages, so in many case we want to disable completely the vendoring in _vendor packages.

A few examples:

• zenoh_cpp_vendor: In conda-forge, we have a build of zenoh_cpp that includes all the features required by rmw_zenoh (see https://github.com/ros2/rmw_zenoh/blob/56f2688994600a176b9b26f7c2f23c00487e220c/zenoh_cpp_vendor/CMakeLists.txt#L18) and also all the default features enabled, so we want to force the use of the conda-forge version, instead of having zenoh_cpp_vendor builds its own limited variant of zenoh, that could conflict with the conda-forge' zenoh.
• gz-*-vendor packages: In conda-forge, we have a build of the gz libraries that support Python bindings (differently from vendor packages, see Enable Python Bindings gazebo-tooling/gz_vendor#2) so we want to use them. However, the satisfied check in gz-math-vendor and other packages is EXACT by default (see https://github.com/gazebo-release/gz_math_vendor/blob/51d2e68e872d2f071bdf40fe7017b9a00ddd8a19/CMakeLists.txt#L31-L37), and the check is not easily disabled by CMake arguments (just from environment variables, but that is tricky for example to set via colcon.pkg files).

At the moment, we handle that with a lot of patches (see for example https://github.com/RoboStack/ros-kilted/blob/main/patch/ros-kilted-zenoh-cpp-vendor.patch, https://github.com/RoboStack/ros-kilted/blob/main/patch/ros-kilted-gz-cmake-vendor.patch, https://github.com/RoboStack/ros-kilted/blob/main/patch/ros-kilted-gz-rendering-vendor.patch and all similar patch in that directory). However, patches are typically cumbersome to maintain, as you need to frequently update them if patched project changes.

To deal with this, this PR adds the AMENT_VENDOR_POLICY CMake option, whose documentation is included in the code, and is reported in the following:

#   AMENT_VENDOR_POLICY: String option that specifies how ament_vendor behaves,
#                        the allowed values are listed in the following.
#     DEFAULT: Vendor if ``SATISFIED`` argument is not supplied or false,
#              do not vendor otherwise.
#     FORCE_BUILD_VENDOR: Always vendor, independently of the value of the
#                   ``SATISFIED`` argument.
#     NEVER_VENDOR: Never vendor, and raise an error if ``SATISFIED`` argument
#                   is not supplied or false.
#     NEVER_VENDOR_IGNORE_SATISFIED_CHECK: Never vendor, and do not raise
#                   an error even if ``SATISFIED`` argument is not supplied
#                   or false. This option is in unsupported by most packages,
#                   so use at your own risk, as it could break the buid.

One downside of having this option, is that now it is not anymore trivial to know if ament_vendor actually vendored something or not, while before it was sufficient to check if the variable passed to SATISFIED was TRUE or not (see for example https://github.com/gazebo-release/gz_sim_vendor/blob/423f5eaf34d1c69735c99704c610e434e902886e/CMakeLists.txt#L90). For this reason, the PR also adds a IS_VENDORED_OUTPUT_VARIABLE_NAME argument that pass the variable name that is populated by ament_vendor reporting if the package is actually vendored or not.

To check if a package was actually vendored, you can check if the target name passed to ament_vendor exists. For example, the gz-vendor packages can check that with:

ament_vendor(${LIB_NAME_UNDERSCORE}_vendor
  SATISFIED ${${LIB_NAME_FULL}_FOUND}
  VCS_URL https://github.com/gazebosim/${GITHUB_NAME}.git
  VCS_VERSION ${GITHUB_NAME}${LIB_VER_MAJOR}_${LIB_VER}${LIB_VER_SUFFIX}
  CMAKE_ARGS
    -DSKIP_PYBIND11:BOOL=ON
  GLOBAL_HOOK
)

# [...]

# Code that only should done if the package is actually vendored
if(TARGET ${LIB_NAME_UNDERSCORE}_vendor)
  ament_environment_hooks("${CMAKE_CURRENT_SOURCE_DIR}/${PROJECT_NAME}.dsv.in")
  # Create a dummy .sh file needed for ament_package to source the .dsv file.
  # See https://github.com/ament/ament_package/issues/145
  file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}.sh "# Dummy .sh file needed for .dsv file to be sourced.")
  ament_environment_hooks("${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}.sh")
endif()

[YuanYuYuan]

As one of the ros-nix-overlay users, I'm excited about this solution 😄

[traversaro]

As one of the ros-nix-overlay users, I'm excited about this solution 😄

Great, feel free to comment if there is anything that could be improved from the nix point of view, as I a not so familiar with nix.

[sloretz]

Thank you for the PR!

I have a couple nits and a question, but this code seems to do what it says.

[cottsay]

Elsewhere in ROS 2 we're already using if(TARGET target_name) for this purpose. Is there some reason that approach isn't sufficient?

For example: https://github.com/ros2/libyaml_vendor/blob/c69b99721471d47f88042b44e510cfefafb385b9/CMakeLists.txt#L27

[traversaro]

Good point! I did not think about this approach, and apparently I am not the only one (all gz vendor packages duplicate the satifisfied condition to check it the package was vendored or not, see https://github.com/gazebo-release/gz_math_vendor/blob/51d2e68e872d2f071bdf40fe7017b9a00ddd8a19/CMakeLists.txt#L67). But using the target defined by the ExternalProject make total sense, I will revise the PR to suggest doing that instead.

[traversaro]

This was fixed in a935bb3 .

[cottsay]

Is there a reason to hide this?

[traversaro]

Probably not, I typically hide options defined in functions to avoid littering the downstream projects option space (for example, last time I checked calling find_package(rclcpp) basically hides all project specific options if they start with a letter later in the alphabet), but in this case if ament_vendor is used in a project, then the main purpouse of a project is to be a vendor package, and then it make sense for the option to be visible.

[traversaro]

Thanks a lot for the review @sloretz and @cottsay, I should have addressed all your comments, and sorry for the delay in the reply.

Downstream projects are adopting local workarounds due to this option not being available (see for example ros2/rmw_zenoh#908) so it would be great to have this option in ament_cmake_vendor_package itself.

[traversaro]

@mjcarroll I should have addressed your feedback. On an unrelated note, could it make sense to enable signoff on web commits for this repo or org? See for example https://docs.github.com/en/organizations/managing-organization-settings/managing-the-commit-signoff-policy-for-your-organization#managing-compulsory-commit-signoffs-for-your-organization, this is already done in gazebosim and ros orgs, and simplifies the process of making simple web-based commits without the DCO bot complaining.

[traversaro]

@mjcarroll @sloretz @cottsay if there is anything I can do to simplify the review of this PR, feel free to ask! It would be great to reduce the amount of patches we have downstream in robostack, and this modification is quite extensively used, so it would be great to have it merged upstram at some point, thanks a lot!

[mjcarroll]

Pulls: #592
Gist: https://gist.githubusercontent.com/mjcarroll/e9ee0e0a05d6c5565388631ab7adb7a2/raw/bf4498cec61724cfce2e59f0774a5c201714db3a/ros2.repos
BUILD args: --packages-above-and-dependencies ament_cmake_vendor_package
TEST args: --packages-above ament_cmake_vendor_package
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/19396

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[Tobias-Fischer]

Quick reminder @mjcarroll @sloretz @cottsay

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[traversaro]

@mjcarroll @sloretz @cottsay if there is anything I can do to simplify the review of this PR, feel free to ask! It would be great to reduce the amount of patches we have downstream in robostack, and this modification is quite extensively used, so it would be great to have it merged upstram at some point, thanks a lot!

Hello @mjcarroll @sloretz @cottsay , friendly remainder on this, having this upstream would be really useful to drop a bunch of robostack-specific patches, and remove the need for all downstream package introduce custom specific-options, as done for example in ros2/rmw_zenoh#908 .