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

RHEL support #694

Merged
merged 7 commits into from
Jun 13, 2024
Merged

RHEL support #694

merged 7 commits into from
Jun 13, 2024

Conversation

christianrauch
Copy link
Contributor

@christianrauch christianrauch commented Jun 4, 2024

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.

Copy link

codecov bot commented Jun 4, 2024

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.

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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@christophebedard christophebedard self-requested a review June 6, 2024 18:37
@christianrauch christianrauch marked this pull request as ready for review June 6, 2024 20:35
@christianrauch christianrauch requested a review from a team as a code owner June 6, 2024 20:35
@christianrauch christianrauch requested review from gbiggs and removed request for a team June 6, 2024 20:35
Comment on lines +76 to +77
const distribVer = await utils.determineDistribVer();
const distribVerMaj = distribVer.split(".")[0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor suggestion, but you could make this a function, since you extract the major version number in addDnfRepo() too

@christophebedard
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

@christophebedard
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

I've opened a PR: ros/rosdistro#41591.

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

@christianrauch
Copy link
Contributor Author

christianrauch commented Jun 12, 2024

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
Copy link
Member

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

@christianrauch
Copy link
Contributor Author

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
Copy link
Member

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

src/utils.ts Outdated Show resolved Hide resolved
@christophebedard
Copy link
Member

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
Copy link
Contributor Author

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

Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
@christophebedard
Copy link
Member

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.

@christophebedard christophebedard merged commit 4e5f33e into ros-tooling:master Jun 13, 2024
26 checks passed
@christianrauch christianrauch deleted the alma branch June 13, 2024 21:04
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

Successfully merging this pull request may close these issues.

2 participants