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

ament_vendor: Add AMENT_VENDOR_NEVER_VENDOR #552

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

Conversation

traversaro
Copy link
Contributor

When building ros package for a rolling release distribution, if you want a ROS ***_vendor to use a distro package instead of building its own, typically you want the CMake configuration to fail with a clear message if it is not able to find the package at the required version, not silently start vendoring the package.

To do so, I added the CMake option AMENT_VENDOR_NEVER_VENDOR that by default is OFF, and if ON, raise a FATAL_ERROR if the SATISFIED option of ament_vendor is OFF, effectively ensuring that ament_vendor never vendors anything. I also added documentation in the macro header of the FORCE_BUILD_VENDOR_PKG CMake option that was missing.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me; I've left a couple of comments.

@cottsay Can you take a look and comment on whether this feature makes sense to you?

ament_cmake_vendor_package/cmake/ament_vendor.cmake Outdated Show resolved Hide resolved
ament_cmake_vendor_package/cmake/ament_vendor.cmake Outdated Show resolved Hide resolved
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Silvio Traversaro <[email protected]>
@cottsay
Copy link
Contributor

cottsay commented Oct 11, 2024

I'm 100% behind the idea here. Some thoughts:

For better or worse, there are vendor packages which we never allow to be satisfied by system packages, and this change will break builds of those packages. Can we check that _ARG_SATISFIED is DEFINED and bypass the error if it wasn't even supplied to the ament_vendor invocation?

I never really liked the name FORCE_BUILD_VENDOR_PKG for this functionality, and just inherited it from sporadic usage among the legacy vendor packaging pattern before ament_vendor came to be. Since these flags are mutually exclusive anyway, can we specify a sort of "tristate" variable instead, and deprecate FORCE_BUILD_VENDOR_PKG in favor of that?

Demo: https://gist.github.com/cottsay/daf9962561ca00c4a78f2bc9b390064b

@traversaro
Copy link
Contributor Author

Cool idea, I will modify the PR to follow @cottsay (I will be traveling next week, so not sure when I will do it).

@cottsay
Copy link
Contributor

cottsay commented Oct 11, 2024

No rush, thanks for considering the changes. I've used this tri-state pattern in other projects before with success, and it seem applicable here.

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

Successfully merging this pull request may close these issues.

3 participants