ros-tooling / setup-ros

Github Action to set up ROS 2 on hosts
Apache License 2.0
86 stars 40 forks source link

RHEL support #694

Closed christianrauch closed 3 months ago

christianrauch commented 4 months ago

This PR adds support for RHEL-derives distributions, such as AlmaLinux and Rocky Linux, to the setup action. The support should be feature-complete. However, I had issues actually applying the action for some of my packages, due to broken dependency definitions in ROS packages. E.g. ros-jazzy-ament-cmake-clang-format has no indirect dependency on clang-tools-extra, which provides the clang-format command; ros-jazzy-ament-clang-format only depends on clang but not clang-format. Hence, any package that has ament-cmake-clang-format as test dependency will fail unless those dependencies are installed manually.

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 12.50000% with 14 lines in your changes missing coverage. Please review.

Project coverage is 86.85%. Comparing base (8295aba) to head (8867912). Report is 2 commits behind head on master.

:exclamation: Current head 8867912 differs from pull request most recent head f79d216

Please upload reports for the commit f79d216 to get more accurate results.

Files Patch % Lines
src/utils.ts 12.50% 14 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #694 +/- ## ========================================== - Coverage 92.89% 86.85% -6.04% ========================================== Files 8 8 Lines 197 213 +16 Branches 23 23 ========================================== + Hits 183 185 +2 - Misses 14 28 +14 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

christophebedard commented 3 months ago

ros-jazzy-ament-clang-format only depends on clang but not clang-format.

Weird, ros-jazzy-ament-clang-format does depend on clang-format on Ubuntu. Looks like the rosdep entry for clang-format is clang-format on Ubuntu, but just clang on RHEL: https://github.com/ros/rosdistro/blob/2716870a27ef298e41fb48041e60c60c4aacc7a5/rosdep/base.yaml#L561. Is the clang-format package available? If so, we could switch it to clang-format.

christianrauch commented 3 months ago

Is the clang-format package available? If so, we could switch it to clang-format.

No. The package with the clang-format executable is called clang-tools-extra.

christophebedard commented 3 months ago

Then let's change it to that one! Do you want to do it, or should I do it?

christophebedard commented 3 months ago

Also, adding workflows to test this on RHEL would be great. You could just do the following workflows for RHEL:

  1. Check development tools
  2. Check ROS distribution (rolling)
christianrauch commented 3 months ago

Then let's change it to that one! Do you want to do it, or should I do it?

Well, if you give me the option, can you do it?

I am also not a regular RHEL or derivative user. I am only working on those actions so I can test-bloom my packages before releasing them. There may be a couple of more dependency issues that should be addressed by someone regularly using the RHEL platforms.

christophebedard commented 3 months ago

I've opened a PR: https://github.com/ros/rosdistro/pull/41591.

I don't really use RHEL either. Issues can always be fixed :grinning:

christianrauch commented 3 months ago

Also, adding workflows to test this on RHEL would be great. You could just do the following workflows for RHEL:

  1. Check development tools
  2. Check ROS distribution (rolling)

I added the AlmaLinux image to the list of Docker images. Apart from the Windows build, the CI passes (https://github.com/ros-tooling/setup-ros/actions/runs/9488155288?pr=694).

christophebedard commented 3 months ago

Thank you! I'm about to get on a plane, so I'll review it all tomorrow.

christianrauch commented 3 months ago

I seem to have problems with the message generation on AlmaLinux 8 but not 9. While ros-humble-rosidl-default-generators and ros-humble-rosidl-typesupport-c get installed, a call to find_package(rosidl_default_generators REQUIRED) fails with:

  -- Found ament_cmake: 1.3.9 (/opt/ros/humble/share/ament_cmake/cmake)
  -- Found Python3: /usr/bin/python3 (found version "3.6.8") found components: Interpreter 
  -- Override CMake install command with custom implementation using symlinks instead of copying resources
  -- Found rosidl_default_generators: 1.2.0 (/opt/ros/humble/share/rosidl_default_generators/cmake)
  CMake Error at /opt/ros/humble/share/rosidl_typesupport_c/cmake/get_used_typesupports.cmake:35 (message):
    No 'rosidl_typesupport_c' found
  Call Stack (most recent call first):
    /opt/ros/humble/share/rosidl_typesupport_c/cmake/rosidl_typesupport_c-extras.cmake:8 (get_used_typesupports)
    /opt/ros/humble/share/rosidl_typesupport_c/cmake/rosidl_typesupport_cConfig.cmake:41 (include)
    /opt/ros/humble/share/rosidl_default_generators/cmake/rosidl_default_generators-extras.cmake:21 (find_package)
    /opt/ros/humble/share/rosidl_default_generators/cmake/rosidl_default_generatorsConfig.cmake:41 (include)

However, the package in question (https://github.com/christianrauch/apriltag_msgs) builds fine on the build farm: https://build.ros2.org/view/Hbin_rhel_el864/job/Hbin_rhel_el864__apriltag_msgs__rhel_8_x86_64__binary/24/consoleFull.

I noticed that the official build farm additionally installs ros-humble-rosidl-typesupport-fastrtps-c and ros-humble-rosidl-typesupport-fastrtps-cpp. I don't see how these get installed as rosdep dependency for apriltag_msgs. There might be more dependency tracking issues with other packages.

christophebedard commented 3 months ago

I have to admit I'm not quite sure what's wrong there. You could start with only supporting RHEL 9 if you want.

christophebedard commented 3 months ago

Looks like the dist/index.js file needs to be re-generated: https://github.com/ros-tooling/setup-ros/actions/runs/9504837413/job/26198373578?pr=694

christianrauch commented 3 months ago

I also took the liberty to fix the Windows - jazzy URL. I am surprised nobody complained about this earlier.

christophebedard commented 3 months ago

Alright, this looks good now. Thank you very much for the PR and for iterating!

Since this definitely won't break existing workflows, we can start with this and then fix stuff further down the road.