Skip to content

ament_vendor: Add 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
traversaro wants to merge 6 commits into
base: rolling
Choose a base branch
from
Open

ament_vendor: Add AMENT_VENDOR_POLICY CMake option #592

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b530c4b
ament_vendor: Add IS_VENDORED_OUTPUT_VARIABLE_NAME argument and AMENT…
traversaro Jul 3, 2025
d940d58
Improve documentation by using active voice
traversaro Jul 17, 2025
0f7fa8a
Remove IS_VENDORED_OUTPUT_VARIABLE_NAME output variable
traversaro Feb 28, 2026
e155504
Fix typo
traversaro Feb 28, 2026
57678fe
Do not mark_as_advanced AMENT_VENDOR_POLICY
traversaro Feb 28, 2026
f6a75b4
Fix wrong CMAKE_BUILD_TYPE set and remove variable pollution
traversaro Mar 7, 2026
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
63 changes: 59 additions & 4 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 @@ -49,6 +49,30 @@
# projects.
# :type GLOBAL_HOOK: option
#
# 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 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, check if the target name
# passed as TARGET_NAME exists with if(TARGET <target_name>).
#
# @public
#
macro(ament_vendor TARGET_NAME)
Expand Down Expand Up @@ -96,12 +120,45 @@ macro(ament_vendor TARGET_NAME)
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.")
Copy link
Copy Markdown
Contributor

@mjcarroll mjcarroll Mar 6, 2026

Choose a reason for hiding this comment

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

Since this is a macro, this leaks to the parent scope. Does this conflict with multiple invocations? Do we need to unset at the end of the scope here?

Copy link
Copy Markdown
Contributor Author

@traversaro traversaro Mar 7, 2026

Choose a reason for hiding this comment

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

Good point, fixed in b95318c .

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")
Copy link
Copy Markdown
Contributor

@mjcarroll mjcarroll Mar 6, 2026

Choose a reason for hiding this comment

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

What happens in the case of multiple vendor packages in the same package and the CACHE variable?

Copy link
Copy Markdown
Contributor Author

@traversaro traversaro Mar 7, 2026

Choose a reason for hiding this comment

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

If the cache entry already exists, a call to set(AMENT_VENDOR_POLICY "DEFAULT" CACHE STRING ${_AMENT_VENDOR_POLICY_DOCS}) will not change the existing value. In a nutshell, this means that the policy of both ament_vendor invocations will be controlled by a single AMENT_VENDOR_POLICY variable (this is for example how this is used for zenoh_cpp_vendor in robostack). One could think of having the possibility of setting per-ament_vendor policy with a AMENT_VENDOR_POLICY_${TARGET_NAME} variable, but as I am not aware of any use case for that, I am afraid it would be not useful complexity.


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)
Copy link
Copy Markdown
Contributor

@mjcarroll mjcarroll Mar 6, 2026

Choose a reason for hiding this comment

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

I don't think we want to set CMAKE_BUILD_TYPE here, right?

Copy link
Copy Markdown
Contributor Author

@traversaro traversaro Mar 7, 2026

Choose a reason for hiding this comment

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

Good point, fixed in b95318c .

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()
Comment thread
sloretz marked this conversation as resolved.
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 +142 to +152
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 Down Expand Up @@ -142,8 +199,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