Skip to content

ament_vendor: Add IS_VENDORED_OUTPUT_VARIABLE_NAME argument and AMENT_VENDOR_POLICY CMake option #592

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from
Open

ament_vendor: Add IS_VENDORED_OUTPUT_VARIABLE_NAME argument and AMENT_VENDOR_POLICY CMake option #592

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e77b2f
ament_vendor: Add IS_VENDORED_OUTPUT_VARIABLE_NAME argument and AMENT…
traversaro Jul 3, 2025
6c5844f
Improve documentation by using active voice
traversaro Jul 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 80 additions & 5 deletions ament_cmake_vendor_package/cmake/ament_vendor.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,35 @@
# project, expose the external project globally to any downstream CMake
# projects.
# :type GLOBAL_HOOK: option
# :param IS_VENDORED_OUTPUT_VARIABLE_NAME: the name of the variable that
# will be set to ``TRUE`` if the package has been vendored, or ``FALSE``
# otherwise.
# :type IS_VENDORED_OUTPUT_VARIABLE_NAME: string
Comment on lines +51 to +54
Copy link
Contributor

@cottsay cottsay Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@traversaro traversaro Jul 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#
# The AMENT_VENDOR_POLICY cache entry and the SATISFIED argument
# control whether or not this function builds the vendor package.
# If you want something other than the default, set AMENT_VENDOR_POLICY via the
# command line:
#
# * If you are using cmake directly: cmake -DAMENT_VENDOR_POLICY:STRING=DEFAULT ...
# * If you are using colcon: colcon --cmake-args -DAMENT_VENDOR_POLICY:STRING=DEFAULT ...
#
# 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,
Copy link
Contributor

@cottsay cottsay Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# or false. This option is in unsupported by most packages,
# or false. This option is unsupported by most packages,

# so use at your own risk, as it could break the buid.
#
# To check if a package has been actually vendored, pass a variable name
# into the argument IS_VENDORED_OUTPUT_VARIABLE_NAME, and check
# if the variable is TRUE after you call `ament_vendor`.
#
# @public
#
Expand All @@ -60,7 +89,7 @@ macro(ament_vendor TARGET_NAME)
message(FATAL_ERROR "ament_vendor() must be called before ament_package()")
endif()

cmake_parse_arguments(_ARG "GLOBAL_HOOK;SKIP_INSTALL" "SOURCE_SUBDIR;VCS_TYPE;VCS_URL;VCS_VERSION;SATISFIED" "CMAKE_ARGS;PATCHES" ${ARGN})
cmake_parse_arguments(_ARG "GLOBAL_HOOK;SKIP_INSTALL" "SOURCE_SUBDIR;VCS_TYPE;VCS_URL;VCS_VERSION;SATISFIED;IS_VENDORED_OUTPUT_VARIABLE_NAME" "CMAKE_ARGS;PATCHES" ${ARGN})
if(_ARG_UNPARSED_ARGUMENTS)
message(FATAL_ERROR "ament_vendor() called with unused arguments: "
"${_ARG_UNPARSED_ARGUMENTS}")
Expand Down Expand Up @@ -93,15 +122,57 @@ macro(ament_vendor TARGET_NAME)
set(_ARG_SATISFIED FALSE)
endif()

# If defined, let's set ${_ARG_IS_VENDORED_OUTPUT_VARIABLE_NAME} to FALSE, it will be set
# to TRUE if the package is actually vendored
if(DEFINED _ARG_IS_VENDORED_OUTPUT_VARIABLE_NAME)
# There is no PARENT_SCOPE as this is a cmake macro,
# if it is converted to a function PARENT_SCOPE will need to be added
set(${_ARG_IS_VENDORED_OUTPUT_VARIABLE_NAME} FALSE)
endif()

option(FORCE_BUILD_VENDOR_PKG
"Build vendor packages from source, even if system-installed packages are available"
OFF)
mark_as_advanced(FORCE_BUILD_VENDOR_PKG)

set(_AMENT_VENDOR_POLICY_DOCS "Specify how ament_vendor behaves, allowed values are DEFAULT, FORCE_BUILD_VENDOR, NEVER_VENDOR and NEVER_VENDOR_IGNORE_SATISFIED_CHECK.")
set(AMENT_VENDOR_POLICY "DEFAULT" CACHE STRING ${_AMENT_VENDOR_POLICY_DOCS})
set_property(CACHE AMENT_VENDOR_POLICY PROPERTY STRINGS "DEFAULT" "FORCE_BUILD_VENDOR" "NEVER_VENDOR" "NEVER_VENDOR_IGNORE_SATISFIED_CHECK")
mark_as_advanced(AMENT_VENDOR_POLICY)
Copy link
Contributor

@cottsay cottsay Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to hide this?

Copy link
Contributor Author

@traversaro traversaro Jul 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if(FORCE_BUILD_VENDOR_PKG AND NOT AMENT_VENDOR_POLICY STREQUAL "FORCE_BUILD_VENDOR")
message(DEPRECATION "FORCE_BUILD_VENDOR_PKG set to ON detected, FORCE_BUILD_VENDOR_PKG variable is deprecated, please set AMENT_VENDOR_POLICY to FORCE_BUILD_VENDOR instead.")
set(CMAKE_BUILD_TYPE "FORCE_BUILD_VENDOR" CACHE STRING ${_AMENT_VENDOR_POLICY_DOCS} FORCE)
endif()

# AMENT_VENDOR_POLICY

if(NOT _ARG_SATISFIED OR FORCE_BUILD_VENDOR_PKG)
if(AMENT_VENDOR_POLICY STREQUAL "FORCE_BUILD_VENDOR")
set(_call_ament_vendor TRUE)
if(_ARG_SATISFIED)
message(STATUS "Forcing vendor package build for '${TARGET_NAME}', which is already satisfied")
message(STATUS "Forcing vendor package build for '${TARGET_NAME}', which is already satisfied as AMENT_VENDOR_POLICY is set to FORCE_BUILD_VENDOR")
endif()
elseif(AMENT_VENDOR_POLICY STREQUAL "NEVER_VENDOR")
if(NOT _ARG_SATISFIED)
message(FATAL_ERROR "Error as SATISFIED argument is not TRUE, but AMENT_VENDOR_POLICY is set to NEVER_VENDOR")
endif()
set(_call_ament_vendor FALSE)
elseif(AMENT_VENDOR_POLICY STREQUAL "NEVER_VENDOR_IGNORE_SATISFIED_CHECK")
if(NOT _ARG_SATISFIED)
message(STATUS "Not vendoring even if SATISFIED is not TRUE as AMENT_VENDOR_POLICY is set to NEVER_VENDOR_IGNORE_SATISFIED_CHECK")
endif()
set(_call_ament_vendor FALSE)
else()
Comment on lines +155 to +165
Copy link
Contributor

@sloretz sloretz Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cottsay this seems to differ from your suggestion of using the presence of the SATISFIED argument to enable the check in #552 (comment) , but I think it accomplishes the same thing as FORCE_BUILD_VENDOR by building the package regardless of SATISFIED being set.

It seems like we'd need CMake 3.31 to handle the case where SATISFIED is given but empty string, so I think if we used if(DEFINED... then the warning would be skipped if SATISFIED was provided with a variable containing the empty string.

Copy link
Contributor

@cottsay cottsay Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really depends on how @traversaro thinks that an unconditionally vendored package should behave in the presence of NEVER_VENDOR_IGNORE_SATISFIED_CHECK. The naming makes me think that a vendored package which doesn't even check for satisfaction would NOT raise an error, in which case, I agree that using DEFINED to check if _ARG_SATISFIED was even supplied would be a better behavior than raising an error 100% of the time.

Copy link
Contributor Author

@traversaro traversaro Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I follow here. The value of _ARG_SATISFIED is normalized earlier:

  if(NOT _ARG_SATISFIED)
    set(_ARG_SATISFIED FALSE)
  endif()

after that check, it is not possible for _ARG_SATISFIED to be undefined or to be defined as an empty string. The only possible values after that check are FALSE or all values that are considered true by CMake, i.e. 1, ON, YES, TRUE, Y, or a non-zero number (including floating point numbers), see https://cmake.org/cmake/help/latest/command/if.html#basic-expressions .

# This is the default case
if(_ARG_SATISFIED)
message(STATUS "Skipping vendor package build for '${TARGET_NAME}', as SATISFIED is TRUE and AMENT_VENDOR_POLICY is set to DEFAULT")
set(_call_ament_vendor FALSE)
else()
set(_call_ament_vendor TRUE)
endif()
endif()

if(_call_ament_vendor)
list_append_unique(_AMENT_CMAKE_VENDOR_PACKAGE_PREFIX_PATH "${CMAKE_CURRENT_BINARY_DIR}/${TARGET_NAME}-prefix/install")

_ament_vendor(
Expand All @@ -115,6 +186,12 @@ macro(ament_vendor TARGET_NAME)
"${_ARG_SKIP_INSTALL}"
)

if(DEFINED _ARG_IS_VENDORED_OUTPUT_VARIABLE_NAME)
# There is no PARENT_SCOPE as this is a cmake macro,
# if it is converted to a function PARENT_SCOPE will need to be added
set(${_ARG_IS_VENDORED_OUTPUT_VARIABLE_NAME} TRUE)
endif()

if(NOT _ament_vendor_called AND NOT _ARG_SKIP_INSTALL)
# Hooks for CMAKE_PREFIX_PATH
if(_ARG_GLOBAL_HOOK)
Expand Down Expand Up @@ -142,8 +219,6 @@ macro(ament_vendor TARGET_NAME)

set(_ament_vendor_called TRUE)
endif()
else()
message(STATUS "Skipping vendor package build for '${TARGET_NAME}', which is already satisfied")
endif()
endmacro()

Expand Down