-
Notifications
You must be signed in to change notification settings - Fork 44
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
RHEL support #694
Conversation
Codecov ReportAttention: Patch coverage is
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. |
const distribVer = await utils.determineDistribVer(); | ||
const distribVerMaj = distribVer.split(".")[0]; |
There was a problem hiding this comment.
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
Weird, |
No. The package with the |
Then let's change it to that one! Do you want to do it, or should I do it? |
Also, adding workflows to test this on RHEL would be great. You could just do the following workflows for RHEL:
|
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. |
I've opened a PR: ros/rosdistro#41591. I don't really use RHEL either. Issues can always be fixed 😀 |
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). |
Thank you! I'm about to get on a plane, so I'll review it all tomorrow. |
I seem to have problems with the message generation on AlmaLinux 8 but not 9. While
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 |
I have to admit I'm not quite sure what's wrong there. You could start with only supporting RHEL 9 if you want. |
Looks like the |
I also took the liberty to fix the Windows - jazzy URL. I am surprised nobody complained about this earlier. |
Signed-off-by: Christian Rauch <[email protected]>
Signed-off-by: Christian Rauch <[email protected]>
Signed-off-by: Christian Rauch <[email protected]>
Signed-off-by: Christian Rauch <[email protected]>
Signed-off-by: Christian Rauch <[email protected]>
Signed-off-by: Christian Rauch <[email protected]>
Signed-off-by: Christian Rauch <[email protected]>
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. |
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 onclang-tools-extra
, which provides theclang-format
command;ros-jazzy-ament-clang-format
only depends onclang
but notclang-format
. Hence, any package that hasament-cmake-clang-format
as test dependency will fail unless those dependencies are installed manually.