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

Use available clang-format version for Ubuntu 22 #45

Merged
merged 3 commits into from
Jan 20, 2024

Conversation

Ryanf55
Copy link
Collaborator

@Ryanf55 Ryanf55 commented Jan 17, 2024

Purpose

ROS2 (on humble) comes with clang-format 14, but the scripts hard code clang-format 6. You can't install it in Ubuntu 22. Thus, it's time to migrate up a few versions.

$ sudo apt install clang-format-6.0
[sudo] password for ryan: 
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
E: Unable to locate package clang-format-6.0
E: Couldn't find any package by glob 'clang-format-6.0'

Risk

Note - running version 14 results in the following diff (small):

ryan@B650-970:~/Dev/ros2_ws/src/grid_map_geo/Tools$ ./check_code_format.sh 
diff --git a/include/grid_map_geo/grid_map_geo.hpp b/include/grid_map_geo/grid_map_geo.hpp
index 7d293b6..6e98f78 100644
--- a/include/grid_map_geo/grid_map_geo.hpp
+++ b/include/grid_map_geo/grid_map_geo.hpp
@@ -37,2 +36,0 @@
-#include "grid_map_geo/transform.hpp"
-
@@ -41,0 +40,2 @@
+#include "grid_map_geo/transform.hpp"
+
diff --git a/src/test_tif_loader.cpp b/src/test_tif_loader.cpp
index a1dddc4..4951269 100644
--- a/src/test_tif_loader.cpp
+++ b/src/test_tif_loader.cpp
@@ -41,4 +40,0 @@
-
-#include "rclcpp/rclcpp.hpp"
-#include "std_msgs/msg/string.hpp"
-
@@ -46,0 +43,2 @@
+#include "rclcpp/rclcpp.hpp"
+#include "std_msgs/msg/string.hpp"
diff --git a/test/test_grid_map_geo.cpp b/test/test_grid_map_geo.cpp
index cb1c005..738be37 100644
--- a/test/test_grid_map_geo.cpp
+++ b/test/test_grid_map_geo.cpp
@@ -1,2 +0,0 @@
-#include "grid_map_geo/transform.hpp"
-
@@ -3,0 +2 @@
+
@@ -5,0 +5,2 @@
+#include "grid_map_geo/transform.hpp"
+
Code style check failed, please run clang-format (e.g. with scripts/fix_code_style.sh)

I've applied the diff, and we now enfoce the same version of clang that is installed on Ubuntu 22.

@Ryanf55 Ryanf55 changed the title Use available clang-format version Use available clang-format version for Ubuntu 22 Jan 17, 2024
@Ryanf55 Ryanf55 force-pushed the use-any-clang-format-version branch from 0d2de2a to 4858a4a Compare January 17, 2024 04:19
Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

Was meaning to do this for a while now.

Would be great if you could rebase?

@Ryanf55 Ryanf55 force-pushed the use-any-clang-format-version branch from bb2d426 to 96aa82b Compare January 17, 2024 14:42
@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Jan 17, 2024

Thanks for fixing this!

Was meaning to do this for a while now.

Would be great if you could rebase?

Done! Conflicts are resolved.

@Ryanf55 Ryanf55 force-pushed the use-any-clang-format-version branch from 93746b4 to 1dc22c0 Compare January 17, 2024 17:03
@Jaeyoung-Lim
Copy link
Member

Jaeyoung-Lim commented Jan 17, 2024

@Ryanf55 Any ideas for the build failures?

@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Jan 17, 2024

@Ryanf55 Any ideas for the build failures

Oops, yep. I'll put it in draft and re-request review when it's fixed.

@Ryanf55 Ryanf55 marked this pull request as draft January 17, 2024 20:44
@Ryanf55 Ryanf55 force-pushed the use-any-clang-format-version branch from 52ab184 to 1ddf87a Compare January 18, 2024 00:34
@Ryanf55 Ryanf55 force-pushed the use-any-clang-format-version branch 3 times, most recently from 1d81acc to a642a1b Compare January 20, 2024 06:21
@Ryanf55 Ryanf55 marked this pull request as ready for review January 20, 2024 06:24
* Use rosdep for all deps
* Name things better
* Stop hard-coding the ros distro

Signed-off-by: Ryan Friedman <[email protected]>
* And add missing dependency on clang-format

Signed-off-by: Ryan Friedman <[email protected]>
* This commit should be hidden in git blame

Signed-off-by: Ryan Friedman <[email protected]>
@Ryanf55 Ryanf55 force-pushed the use-any-clang-format-version branch from 2e49e2b to 2946784 Compare January 20, 2024 06:35
@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Jan 20, 2024

Ready for final review. Let's try to get this in soon to fix all the style problems and enforce clean diffs after this.

Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@Jaeyoung-Lim Jaeyoung-Lim merged commit 7155cb4 into ethz-asl:ros2 Jan 20, 2024
3 checks passed
@Ryanf55 Ryanf55 deleted the use-any-clang-format-version branch January 20, 2024 07:42
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