search

ros2 / ci

ROS 2 CI Infrastructure
http://ci.ros2.org/
Apache License 2.0
48 stars 30 forks source link

Make sure to ignore rosidl_dynamic_typesupport_fastrtps if needed. #785

Closed clalancette closed 1 month ago

clalancette commented 2 months ago

When we are not building Connext or Fast-RTPS, we have to make sure not to attempt to build this.

Draft for now, while I run CI on it over in https://github.com/ros2/rmw_cyclonedds/pull/504

clalancette commented 2 months ago

This looks like it does the trick (see the jobs in https://github.com/ros2/rmw_cyclonedds/pull/504 which are using it), so marking this as ready for review.

clalancette commented 2 months ago

I'm pretty stale on the typesupport dependency chains. Do we not need this package blocked on the other sans-fastrtps configurations like https://github.com/ros2/ci/pull/785/files#diff-54f7a5a1191410c54331d5e337e5760c7de5e219f01dfc732b9380dbf71b46b7R688 as well?

It is tricky.

rosidl_dynamic_typesupport_fastrtps depends on fastrtps_cmake_module, rcutils, rosidl_dynamic_typesupport, fastcdr, and fastrtps. There is no situation in which the CI scripts automatically ignore rcutils, rosidl_dynamic_typesupport, or fastcdr, so I won't consider them further.

For fastrtps and fastrtps_cmake_module, the only time we ignore them is when all of rmw_fastrtps_cpp, rmw_fastrtps_dynamic_cpp, and rmw_connextdds are ignored. In that case we have to ignore rosidl_dynamic_typesupport_fastrtps, since its dependencies are ignored. That's what this PR does.

In all of the other cases (where maybe rmw_fastrtps_cpp is ignored, but rmw_connextdds is not), rosidl_dynamic_typesupport_fastrtps can very well be compiled and succeed. That's because rmw_connextdds (for instance) depends on fastrtps and fastrtps_cmake_module.

So I believe the logic is correct as-is. That said, I'll be honest that I did not test anything here than the default (all RMWs enabled), and the CycloneDDS-only scenario. I can kick off some additional builds to see what happens.

All of that said, I don't really like this automatic blacklisting of packages. I guess it is kind of nice to encode this information here so users don't have to figure out the proper --packages-ignore build command when they want to use just one particular RMW, but it also sucks to have these packages hardcoded here and it makes the Jenkins UI for this very busy. But that is a problem for another day.

nuclearsandwich commented 2 months ago

All of that said, I don't really like this automatic blacklisting of packages.

Yeah, it's definitely a relic from when ROS 2 and CI were smaller and more developer-internal tools. I'm not even sure that moving to rosdep support for dependenices is a way around this since these are all in-workspace dependencies that need to be masked.

Maybe it's Fine™️ that CI is like this or maybe what we need is a way to push this information up to CI users by encoding it somewhere more discoverable even if the workflow remains unchanged.

clalancette commented 1 month ago

Here are some CI builds with this in place to see what happens:

Rolling, regular CI:

Rolling, Linux-only, ignore everything but CycloneDDS (the original problem):

Rolling, Linux-only, ignore everything but FastDDS:

Rolling, Linux-only, ignore everything but Connext:

clalancette commented 1 month ago

All right, all the CI I ran looks good (there are some failing tests on Connext, but that is likely because of the code itself, not this change). With that, and the approval, I'll go ahead and merge, thanks!