Incorporating ABI compatibility checks in CI pipelines of core ROS 2 packages.
Why?
REP-0009 “ABI compatibility” requires that, on a “best effort” basis, ABI compatibility be maintained for even-cycle patch releases of the public facing API of a package.
While the PR review process, developer guide, and manual ABI compatibility checking cover most bases, I believe being able to automate this process provides ease-of-mind value to authors, maintainers, and reviewers of the C/C++ libraries within the ROS 2 core. Having ABI compatibility results between a suggested patch and its targeted base available in the CI pipeline could even help in discussing designs that are (more) easily backportable - an issue I’ve seen quite a few non-trivial PRs in rclcpp facing.
Also, why do the check yourself when a computer can do it instead!
How?
Essentially, the package needs to be built twice, once with the changes in the current HEAD reference, and once with the base said PR is targeting. The two build artifacts representing the respective library ELF binaries can then be supplied to an ABI compatibility checker. One tool to facilitate this proposal would be RedHat’s abidiff.
Input/Thoughts?
I recognize that this is not particularly high priority given the frequency of ABI compatibility issues within core ROS 2 packages, but I’d still like to hear others’ thoughts on augmenting the ROS 2 developer experience like this.
Are there any latent pros/cons of this proposal which weren’t considered here?
Is this something anyone has tried to do with their own ROS packages/libraries?
Is it worth the churn given that it isn’t a particularly low-hanging fruit?
(If this isn’t the right place to have this discussion, feel free to redirect!)
So it turns out that we already have some builtin buildfarm support for this, in the form of a test_abi flag that can be added to a package in rosdistro (e.g. rosdistro/distribution.yaml at 506ea82e36f8847407fd55a2f86933d9a890329e · ros/rosdistro · GitHub). That said, we don’t enable this on very many packages, and I honestly don’t know what the result of doing that is. I believe @jrivero did most of the implementation of that, so may be able to provide more details on exactly what that option does.
@aprotyas thanks for bringing this up, i believe this really helps for review and process. probably automatically labeling api-review / abi-review would be nice to see for each PR.
I would love a tool like this for Nav2 so that I can be confident about what I can and cannot backport on an immediate glance without having to re-read code every time I do a sync to older distributions.
I’d love this even more if it were an ament_abi tool that could be run as part of the default automatic profile of static analysis tools – but even just some standard tool and profile across the ROS community would have value.
I wanted to have something like this since ROS Hydro but never got around to implement it!
I think an ament_ wrapper may not be the right venue for this as it’d flag it up inappropriately as a test failure whether you like it or not.
While something you can simply slap onto your gh workflows and configure which branch to apply it to would be awesome, in most cases maintainers would only need this to flag up a warning for branches where ABI is meant to stay stable.
How I was imagining using it was to create a compiler flag for whether or not its valid for there to be ABI breaking changes and set the docker container for each branch to set whether its valid to run the ament_ wrapper such that PRs on stable distribution branches would fail the test if it changed ABI.
But agreed, that may not be the preferable way of doing it. I’m open to other workflows - I’m sure there must be some conventions around this from other communities.
That certainly sounds close to the topic here - not sure why I never noticed this in action. Maybe the PR reviewers are doing a great job!
I concur that this is the use-case that’d see most of the gains if ABI compatibility checks were enabled.
Yes, this is really what I was envisioning with this as well. Ideally, there will be a Github Action (think ros-tooling/action-ros-ci) that can be tacked on as an additional step to (existing) Github CI workflows - at best, maybe a 3 line addition to CI workflows! I haven’t fully thought about the mechanics but this might be complicated for repositories that either (a) generate multiple targets or (b) house multiple packages.
On the other hand, an ament_ wrapper might be a bit awkward to work around because the ABI checking portion shouldn’t creep into the package’s build system information, like its CMakeLists.txt file. Also, if anyone is so inclined to clone/build/test a PR locally, running an ABI compatibility checker on top would not be a big deal. I think the real utility is for backport reviewss and initial phase review discussions on Github’s online PR interface.
My cursory search online didn’t lead to much… but if anyone can find examples of similar workflows in the wild, feel free to drop it in the comments.
I already opened issue about this on ros-controls/ros2_control repository. I am planing to support community contribution for creation of a first viable pipeline. I have a clear idea how this should work, but I, don’t have the capacity to tackle it. If you want to keep updated about this check our issue I hope we can motivate someone to tackle this in the next weeks.
P.S. We will probably go with industrial_ci first (I know it better) and then we can check how to add this into a tooling action.
