[feature] CI job to verify dependencies package.xml

[SteveMacenski]

Hi, feel free to close if you feel this is more on the developer (which is a totally fair assessment).

In my experience releasing packages, new packages typically fail the build farm the first time (or two), usually due to dependencies not being correctly called out in the package.xmls for the isolated builds. There are a couple of other common failure modes, but I have to say that this probably accounts for 75% of my re-releases.

The suggestion is to add a CI job to do some simple cross checking between the cmakelists and the package.xml making sure that any ROS package find_package() argument has a matching test_depend depend exec_depend or build_depend call. Obviously this doesn't hold for external libraries in rosdep where the rosdep keys may not match the find_package() name, but would still probably catch a number of issues before ever hitting the build farm stage. I think this would overall save alot of man-power on maintaining this repo if folks can resolve these issues on their own.

An alternative would be instead to add a new ament_XYZ test for this type of thing as well to be a more general solution, so that even local developer builds would fail if dependencies weren't correctly called out. I think there's a few places this could potentially live and I'd be open to discussing the right location.

Let me know what you think!

[nuclearsandwich]

First-time releases are definitely an area where dependency details can trip up initial package releases. catkin_make_isolated and colcon (which builds packages in isolation by default) will isolate packages from each other preventing unintended cross-dependencies and the deeper down your development workspace builds the greater that built-in isolation will be but neither tool can isolate dependencies installed system-wide, including installed ROS packages. Providing totally isolated environments for building each package on the ROS build farm is expensive from a build time perspective and creating environments per repository for dev and PR jobs has always been seen as an acceptable trade-off to get things close enough. Since binary packages are built one at a time rather than together with the rest of the repository that's the first time we do build each package in its own environment (although it's worth noting that we create those environments using platform-specific tooling after using bloom to translate package.xml metadata into platform dependency-specific metadata). With the size of the ROS ecosystem I don't think enough has changed that I would want to offer per-package isolation in CI jobs due to the increased compute time cost. However I think it may be feasible to add a colcon extension¹ which builds each package in a chroot or container image in order to get that full isolation tested locally.

^1. I also think an independent build tool could do it but starting from colcon will save reimplementation of lots of package handling and task creation effort.

---

Scanning and comparing CMakeLists.txt and package.xml wouldn't be a panacea since as you point out it would only work for packages where the find_package name matches the dependency key name. Further, it wouldn't work for packages that use build systems other than cmake or ament_cmake (of which only ament_python is currently supported by the build farm) but it would help catch undeclared dependencies on other ROS packages that are installed as system packages and that's no small thing. A static check like that would be implementable as a linter and integrated with the ament_lint workflow so that's definitely the approach I would lean towards.

[SteveMacenski]

I agree that doing per-package CI would be probably not practical. I primarily recommend just a CI job to scan the cmakelists / package.xml file to take care of the ros packages (since their keys will match find_package() calls). It's certainly not going to catch every error, but I think it would catch quite a few of them.

The 3 places I could see this having immediate value:

• Bloom-release having this be part of its step, for immediate feedback to the user without having to re-release or even bother rosdistro maintainers
• rosdistro to catch this before wasting cloud resources on spinning things up. Also pretty immediate since users can fix and re-open a PR within a few minutes with updates.
• colcon extension to test in their own environment as they wish

[130s]

+1 for adding more checking in rosdistro repo!

• @SteveMacenski Just curious, are you seeing cases where checking dependencies on each repository doesn't capture missing deps?

• A static check like that would be implementable as a linter and integrated with the ament_lint workflow so that's definitely the approach I would lean towards.

  Off top of my head, roscompile (discourse.ros.org) does seem to offer static checking. AFAIK as of now 2 modes it offers are to modify the package.xml and CMakeLists.txt files with/out asking human intervention, so it may not work out of the box for CI where we might only need boolean kind of output, but I imagine adding a feature to only return checking result can be simple.

Whatever the solution would be, I think the functionality will be useful in many different contexts, not just for rosdistro repo. So I +1 for adding such a functionality in a standalone tool like it's been discussed above already.

[tfoote]

If you'd like to implement the basic checks such as making sure that a found package which is a ROS package is at least in one of the dependencies of the package.xml I think that's a good idea. However that really should just be a dependency linter that if robust could be added to our default set of linters. If not it's just a linter that can be either manually added or run by the developer.

The only way to really check the dependencies and know that they're going to build which is what we would need to do to have a gating check is significantly complex. I wrote up a bit of an analysis on a similar discourse thread a few months ago.

[SteveMacenski]

This is more a smoke / sanity check since that is what has caused many of my personal issues with re-releasing because of dependencies missing. I can't speak on the maintainers' side about how often this is the root cause of issues in the build farm, but the impression I am under is it is fairly trial and error for many others.

The general strategy I would take on this:

• Pull in the rosdistro/<distro>/distribution.yaml
• Find all find_package() calls in CMakeLists.txt in the package, take the first argument
• If the call in find_package() matches an entry on distribution.yaml, then check that there is some form of dependency in package.xml for it of the same key name.

Since only ROS packages are in distribution.yaml, the keys for rosdistro and CMake are identical. Any case where you attempted a release where a given dependency does not already exist is already given to users as a warning in bloom-release so that part is covered already. Any non-ROS packages are excluded from the scope of this particular tool. You could still define the wrong depend_* tag, but I think this meets the 80/20 rule and would be simple to implement and integrate.

Would that be sensible?

[gavanderhoorn]

It doesn't have a ROS 2 compatible version right now afaik, but "check package.xml against CMakeLists.txt" is a large part of what catkin_lint does (to the point of complaining about packages appearing in find_package(..) but not have the required *_depend and vice-versa).