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

Support propagation of version compatibility to consuming libraries #506

Open
Ryanf55 opened this issue Feb 9, 2024 · 2 comments
Open

Comments

@Ryanf55
Copy link
Contributor

Ryanf55 commented Feb 9, 2024

Use Case

I am a developer that works in a code base with multiple repositories. These are shared by
multiple projects. Some projects use ROS, some do not. Some projects use ament, others do not.

Currently, it is difficult to enforce ABI breaking checks at configure time in our ament packages.

Motivating Example

I have three packages and two teams.

  • foo_core: Version 3, shared by both teams
  • bar_app: Version 6, owned by bar team
  • baz_app: Version 2, owned by baz team

Foo has a horrible C++ function signature with no error handling.

void foo_core::do_velocity_cmd(
    double linear_x,
    double linear_y,
    double linear_z,
    double angular_x,
    double angular_y,
    double angular_z);

Both bar and baz teams despise it, and agree to make an ABI breaking change when we switch to ROS.

#include <geometry_msgs/msg/TwistStamped>
enum class VelCmdErr {
    SUCCESS,
    UNRESPONSIVE,
    OUT_OF_RANGE
};
[[nodiscard]] VelCmdErr foo_core::do_velocity_cmd(geometry_msgs::msg::TwistStamped& twist);

Ok great, we broke ABI, we don't have time to do a nice migration over the next 6 years, so how do
the baz and bar team's build system know about this?

Normally, foo_core uses Modern CMake, SemVer, and expresses its version dependencies in CMake.

Because rosdep doesn't support versioning, we all build stuff from source and rely on find_package
with a version requirement.

Here's the FOO CmakeLists

foo_repo/foo_core/CMakeLists.txt

cmake_minimum_required(VERSION 3.23)
project(foo_core VERSION 4) # Used to be 3
find_package(ament_cmake CONFIG 1 REQUIRED)
find_package(geometry_msgs CONFIG 1 REQUIRED)

add_library(velocity)
target_sources(velocity
    PRIVATE src/velocity.cpp
    PUBLIC include/${PROJECT_NAME}/velocity.cpp
)
target_link_libraries(velocity PUBLIC ${geometry_msgs_TARGETS})

include(GNUInstallDirs)
install(
   TARGETS velocity
   EXPORT export_${PROJECT_NAME}
   RUNTIME DESTINATION ${CMAKE_INSTALL_LIBDIR}/${PROJECT_NAME}
   FILE_SET HEADERS
   INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
)
ament_export_targets(export_${PROJECT_NAME} HAS_LIBRARY_TARGET)
ament_export_dependencies(geometry_msgs)
ament_package()

bar_repo/bar_app/CMakeLists.txt

cmake_minimum_required(VERSION 3.23)
project(bar_app VERSION 6)

find_package(ament_cmake CONFIG 1 REQUIRED)
find_package(foo_core CONFIG REQUIRED)

add_library(app)
target_sources(app
    PRIVATE src/app.cpp
    PUBLIC include/${PROJECT_NAME}/app.hpp
)
target_link_libraries(velocity PRIVATE foo_core::velocity)

include(GNUInstallDirs)
install(
   TARGETS velocity
   EXPORT export_${PROJECT_NAME}
   RUNTIME DESTINATION ${CMAKE_INSTALL_LIBDIR}/${PROJECT_NAME}
   FILE_SET HEADERS
   INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
)
ament_export_targets(export_${PROJECT_NAME} HAS_LIBRARY_TARGET)
ament_export_dependencies(foo_core)
ament_package()

bar.repos

foo/foo_core:
    version: main
    url: https://server/foo/core.git
    type: git

If a bar developer pulls sources and builds, they get a link error which is hard to troubleshoot, especially if it was the baz team that broke ABI.

Proposal

  1. Allow ROS packages to express their version compatibility in the installation
  2. Allow ROS packages to propogate requirements for the versions of their dependencies

Express Version Compatibility in the Installation

Add a COMPATIBILITY argument to ament_package. Default to AnyNewerVersion if not supplied which matches current behavior.

User call:

ament_package(COMPATIBILITY SameMajorVersion)

The compatibility would be forwarded to write_basic_package_version_file.

Then, you can use it in bar

find_package(foo_core 4 CONFIG REQUIRED)

If you forgot to update foo_core from 3 to 4 and rebuild your workspace, then colcon
will fail at the configure stage of bar with a nice error message.

Express Version Compatibility in Dependencies

ament_export_dependencies shall propagate the version requested of dependencies to a call to find_dependency <version>.

For example:
find_package(foo_core CONFIG 4) results in
add_dependency(foo_core CONFIG 4).

find_package(foo_core CONFIG 4.8.3-dev2) results in
add_dependency(foo_core CONFIG 4.8.3-dev2).

find_package(foo_core MODULE) results in
add_dependency(foo_core MODULE).

I think this can be done automatically with the use of foo_core_FIND_VERSION.

Adding in the find method of MODULE/CONFIG may be out of scope for this discussion.

I want to avoid a user manually maintaining versions in multiple places.
It's hard enough as it is to remember when you add a find_package call to also add it to ament_export_dependencies.

References

Big picture

ament_cmake is highly optimized for the ROS way of building packages, which prohibit ABI breaks in LTS releases. Let's give users of ament a bit more flexibility and nicer behavior if they break from that paradigm and provide intuitive ways that expose the standard CMake ways of handing versioning and ABI breaks.

@audrow
Copy link
Contributor

audrow commented Feb 22, 2024

Hey @Ryanf55, this seems like a good thing to put in a design doc in this repository to get some discussion and feedback.

Can you take the text of this issue and move it into the package you think is most relevant in a directory called doc and open a PR? From there it will be easier to discuss this.

From there, feel free to close this issue or link it to the PR.

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Feb 23, 2024

Done! See the linked PR.

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

2 participants