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

fix RHEL CI warning for rosbag2_storage_mcap. #1883

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

fujitatomoya
Copy link
Contributor

@fujitatomoya fujitatomoya requested a review from a team as a code owner December 14, 2024 23:58
@fujitatomoya fujitatomoya requested review from MichaelOrlov and hidmic and removed request for a team December 14, 2024 23:58
Copy link
Contributor Author

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@MichaelOrlov can you take a look?

@fujitatomoya fujitatomoya force-pushed the fujitatomoya/humble-rhel-warning-fix branch 4 times, most recently from 9d1e034 to ee2da8d Compare December 15, 2024 00:34
Copy link
Contributor Author

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@MichaelOrlov
Copy link
Contributor

@fujitatomoya I am bit confused. Do you see this error on Rolling or Humble? And only on RHEL or other distros too?
If this is on Humble, it is a bit unexpected and suspicious that ament_calng_format was taking from rolling. From your log.

Setting up ros-rolling-ament-clang-format (0.11.4-1focal.20220120.172447) ...
/usr/bin/bash -c source /opt/ros/rolling/setup.sh && ament_clang_format $(colcon list --packages-select rosbag2_storage_mcap -p) --config rosbag2_storage_mcap/.clang-format
Code style divergence in file 'rosbag2_storage_mcap/test/rosbag2_storage_mcap/test_message_definition_cache.cpp':

@MichaelOrlov
Copy link
Contributor

@fujitatomoya I have a suspicious that $(colcon list --packages-select rosbag2_storage_mcap -p) fails to find rosbag2_storage_macp package for some reasons and ament_clang_fromat ignores local config from the rosbag2_storage_mcap/.clang-format and trying to check against default clang_format configuration.

@MichaelOrlov
Copy link
Contributor

See in the log https://ci.ros2.org/job/ci_linux-rhel/1801/consoleText

rhel/ws/build/rosbag2_storage_mcap/ament_clang_format/clang_format.txt" "--command" "/home/jenkins-agent/workspace/ci_linux-rhel/ws/install/ament_clang_format/bin/ament_clang_format" "--xunit-file" "/home/jenkins-agent/workspace/ci_linux-rhel/ws/build/rosbag2_storage_mcap/test_results/rosbag2_storage_mcap/clang_format.xunit.xml" "--config" ".clang-format"

As far as I understand the clang format trying to refer to the the default .clang-format file. which is not going to be present in the build folder at least. And therefore clang-format silently fallback to the default .clang-format configuration.

Need to compare how the same clang format check running on Rolling CI or how it was running before it starts failig.

@fujitatomoya
Copy link
Contributor Author

Do you see this error on Rolling or Humble?

Humble only.

And only on RHEL or other distros too?

Yeah, RHEL only.

it is a bit unexpected and suspicious that ament_calng_format was taking from rolling. From your log.

So this is really weird... why does it not use humble version for that... this does not look like a problem in rosbag2.

@clalancette
Copy link
Contributor

@fujitatomoya I am bit confused. Do you see this error on Rolling or Humble? And only on RHEL or other distros too?
If this is on Humble, it is a bit unexpected and suspicious that ament_calng_format was taking from rolling. From your log.

This is a bug in the GitHub workflow on Humble: https://github.com/ros2/rosbag2/blob/humble/.github/workflows/lint.yml#L21 . I'll submit a PR to fix it.

@fujitatomoya
Copy link
Contributor Author

@clalancette thanks! i was taking a look at the same line! happy to review PR!

@clalancette
Copy link
Contributor

See #1884, which should fix this.

@fujitatomoya fujitatomoya force-pushed the fujitatomoya/humble-rhel-warning-fix branch from ad13f87 to 6e81385 Compare December 17, 2024 18:25
@fujitatomoya
Copy link
Contributor Author

Test CI with rosbag2_storage_mcap to see if clang_format still sees the failure. (current code is the same with humble head)

@fujitatomoya
Copy link
Contributor Author

Pulls: #1883
Gist: https://gist.githubusercontent.com/fujitatomoya/22ddf66f8e25e92464b14c5cfa549390/raw/95fa7054ffaefd956248eb5a80a8bc0f9ac23798/ros2.repos
BUILD args: --packages-up-to rosbag2_storage_mcap
TEST args: --packages-select rosbag2_storage_mcap
ROS Distro: humble
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14973

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya fujitatomoya force-pushed the fujitatomoya/humble-rhel-warning-fix branch from 6e81385 to 4fa321e Compare December 17, 2024 20:02
@fujitatomoya fujitatomoya force-pushed the fujitatomoya/humble-rhel-warning-fix branch from 4fa321e to b66eb2d Compare December 18, 2024 00:58
@fujitatomoya
Copy link
Contributor Author

the other thing is that github workflow uses released package, and CI builds the package for ament-clang-format.
i tried using both ros-humble-ament-clang-format and source code build, it does not change the outcome.

after all, github workflow seems to be fine to me by local verification.

something could be wrong with RHEL CI but the following CI commands are the same between rolling and humble.

Test command: /usr/bin/python3 "-u" "/home/jenkins-agent/workspace/ci_linux-rhel/ws/install/ament_cmake_test/share/ament_cmake_test/cmake/run_test.py" "/home/jenkins-agent/workspace/ci_linux-rhel/ws/build/rosbag2_storage_mcap/test_results/rosbag2_storage_mcap/clang_format.xunit.xml" "--package-name" "rosbag2_storage_mcap" "--output-file" "/home/jenkins-agent/workspace/ci_linux-rhel/ws/build/rosbag2_storage_mcap/ament_clang_format/clang_format.txt" "--command" "/home/jenkins-agent/workspace/ci_linux-rhel/ws/install/ament_clang_format/bin/ament_clang_format" "--xunit-file" "/home/jenkins-agent/workspace/ci_linux-rhel/ws/build/rosbag2_storage_mcap/test_results/rosbag2_storage_mcap/clang_format.xunit.xml" "--config" ".clang-format"

i cannot think of anything else... any thoughts?

@clalancette
Copy link
Contributor

i cannot think of anything else... any thoughts?

My best guess of what is happening here is that the different versions of clang on Ubuntu 22.04 (1.14.0) and RHEL-8 (18.1.8) have different formatting. We deal with this a lot when updating e.g. uncrustify.

The good news is that there is a solution which I believe will satisfy both. I'm going to push a commit to this branch in a minute here to see if that is the case.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor

All right, the only failure here was on the Hpr job, but that didn't have to do with formatting. So I'll say that this is fixed. I'm going to run CI on it. @MichaelOrlov since I did some work here, I'd like your review on this.

@clalancette
Copy link
Contributor

Re-run of CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Contributor Author

@clalancette thanks for checking this! hopefully CI goes green 🤞🤞🤞

@fujitatomoya
Copy link
Contributor Author

@MichaelOrlov all green!

@clalancette thanks again 🚀 this took my time longer than expected.... small things take time 😅

@MichaelOrlov MichaelOrlov merged commit a13af8e into humble Dec 19, 2024
11 of 12 checks passed
@MichaelOrlov MichaelOrlov deleted the fujitatomoya/humble-rhel-warning-fix branch December 19, 2024 00:27
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.

3 participants