search

moveit / moveit_resources

URDFs, meshes, and config packages for MoveIt testing
65 stars 118 forks source link

Pin ubuntu version and update clang-format #159

Closed JafarAbdi closed 1 year ago

JafarAbdi commented 1 year ago

Our ros2 branch target humble/rolling and ubuntu 22.04 is the only tier 1 platform. this PR avoid using ubuntu-latest and pin the version + update clang-format to 12

JafarAbdi commented 1 year ago

Regarding the switch to clang-tidy-12: Did you run pre-commit run -a locally to check all files for formatting changes due to the version change? CI doesn't do this: It only checks .cpp files modified in a PR, which are none here.

Yes, I did before pushing the commits!

rhaschke commented 1 year ago

I just re-checked the ros2 branch of this repo: It doesn't contain any C or C++ files anymore (you removed the Pilz robot). Thus, indeed here, one could remove clang-format checking altogether.

Is there any specific reason you decided on clang-format-12 instead of clang-format-14, which is the newest one available in 22.04? As I pointed out in https://github.com/ros-planning/moveit_resources/pull/158#pullrequestreview-1217620188, this is a decision that should be discussed in the maintainer meeting.
I think we should use the same clang-format version across all MoveIt repositories. So, I'm curious which version is preferred on the main repo.

JafarAbdi commented 1 year ago

Is there any specific reason you decided on clang-format-12 instead of clang-format-14, which is the newest one available in 22.04? As I pointed out in https://github.com/ros-planning/moveit_resources/pull/158#pullrequestreview-1217620188, this is a decision that should be discussed in the maintainer meeting. I think we should use the same clang-format version across all MoveIt repositories. So, I'm curious which version is preferred on the main repo.

I just picked the version we used in the main repo https://github.com/ros-planning/moveit2/blob/main/.github/workflows/format.yaml#L22-L23. I added it to the agent for the next maintainers meeting

rhaschke commented 1 year ago

clang-format-12 was introduced in https://github.com/ros-planning/moveit2/pull/1095 without deeper discussion. Howerver, Vatan complained about the bad formatting results of clang-format-12: https://github.com/ros-planning/moveit2/pull/1095#discussion_r817233046. Maybe you discussed that in March somewhere else?

vatanaksoytezer commented 1 year ago

clang-format-12 was introduced in ros-planning/moveit2#1095 without deeper discussion. Howerver, Vatan complained about the bad formatting results of clang-format-12: ros-planning/moveit2#1095 (comment). Maybe you discussed that in March somewhere else?

Sorry for chiming in late. So the reason I used 12 at the time was to maintain compatibility between 20.04 and 22.04. clang-format-12 is the only version that exists in both. Since ubuntu-latest and all of our development is now fully on 22.04 I don't see a reason to not use clang-format-14