search

ament / uncrustify_vendor

CMake shim over the uncrustify library: https://github.com/uncrustify/uncrustify
Apache License 2.0
0 stars 9 forks source link

create an actual uncrustify_vendor package and build only is the vers… #1

Closed mikaelarguedas closed 6 years ago

mikaelarguedas commented 6 years ago

…ion on the path is lower than 0.66.1

As this builds the next version of uncrustify (that has the windows fix we want). I'll provide a list of changes necessary to comply with the new version so that we can make the decision of building a custom hash of 0.66.1 or using 0.67 (noting that homebrew ships 0.67).

CI with uncrustify 0.67 linter fixups:

CI on all rmw with 0.67 across the board:

Connects to ament/uncrustify#24

mikaelarguedas commented 6 years ago

Summary of the changes to support 0.67:

Code block closing indent fixup:

Closing parenthesis aligned with outer scope opening parenthesis

https://github.com/ros2/rviz/compare/ros2...ros2:uncrustify_0.67?expand=1#diff-01e3c347f9c0599fe3e931b968f6f042R146

Closing template marker matching indent of the opening template marker

https://github.com/ros2/rviz/compare/ros2...ros2:uncrustify_0.67?expand=1#diff-10cc67289440c9306d0985f5b12e6299R107

Space before parenthesis

Including within templates: https://github.com/ros/class_loader/commit/7f8cfbf35e3f0cfbd30d30d6f294ea95aa5b52cc#diff-46f87fe72b6ae80b3c6dfdf4057b0936R86

Though some exceptions: https://github.com/ros2/demos/commit/3d213fa3ad1222cfb5d6ab650ac826e9243a6d10#diff-c53d060e7c287a653e8fef96c7f7a4eeR89

Space around operators

No space around -> operator (was already the case it just cover more cases now) https://github.com/ros2/rviz/compare/ros2...ros2:uncrustify_0.67?expand=1#diff-921965224fd33c84693c4588e71cf92cR133

Space around & (was already the case it just cover more cases now): https://github.com/ros2/rviz/compare/ros2...ros2:uncrustify_0.67?expand=1#diff-fc0c95307e7270a94d454e7a6058de6eR75

No space between 2 *: https://github.com/ros2/rcl/compare/uncrustify_0.67#diff-8fda46206fc38bfdbcac0e5213bffddaR307

Space before {}: https://github.com/ros2/rosidl/commit/dc9d61789ca87bc2a0abed9c252736fa1f1e5e10

Space between * and variable name (Not sure how I feel about this one): https://github.com/ros2/rclcpp/compare/uncrustify_0.67#diff-5af14f62862bacbab35ea2de5215f716R119

Full list of changes on ros2.repos:

ros/class_loader: https://github.com/ros/class_loader/commit/7f8cfbf35e3f0cfbd30d30d6f294ea95aa5b52cc ros2/demos: https://github.com/ros2/demos/commit/3d213fa3ad1222cfb5d6ab650ac826e9243a6d10 ros2/rcl: https://github.com/ros2/rcl/compare/uncrustify_0.67 ros2/rclcpp: https://github.com/ros2/rclcpp/compare/uncrustify_0.67 ros2/realtime_support: https://github.com/ros2/realtime_support/commit/7880a1ceee134cf562a67a51b728b48a367e1b91 ros2/rmw_fastrtps: https://github.com/ros2/rmw_fastrtps/compare/uncrustify_0.67 ros2/rosidl: https://github.com/ros2/rosidl/compare/uncrustify_0.67 ros2/rosidl_typesupport_connext: https://github.com/ros2/rosidl_typesupport_connext/compare/uncrustify_0.67 ros2/rosidl_typesupport_opensplice: https://github.com/ros2/rosidl_typesupport_opensplice/compare/uncrustify_0.67 ros2/rviz: https://github.com/ros2/rviz/compare/ros2...ros2:uncrustify_0.67 ros2/system_tests: https://github.com/ros2/system_tests/commit/86c75e6c4998794f28f2667d20b82e09b0f086e8

@ament/team Can you please give feedback on the changes we do / do not want to comply with ? This will help use decide if we stick with 0.66.1 everywhere or upgrade everything to 0.67.

dirk-thomas commented 6 years ago

The changes to the existing sources seem almost all ok. Only the four spaces between the semicolon and the comment look weird to me: https://github.com/ros2/rviz/compare/ros2...ros2:uncrustify_0.67#diff-5fb4c5847e65295257a425c49e8b75b4R342 Are they really necessary?

Can you please update the uncrustify configuration file in ament_lint. That will also show the new options and what value are chosen for them by default.

I am just wondering if the updated code also passes with uncrustify 0.66.1? If not, that would be a problem since that is the version we want to use on Ubuntu Bionic, no?

mikaelarguedas commented 6 years ago

The changes to the existing sources seem almost all ok. Only the four spaces between the semicolon and the comment look weird to me: ros2/rviz@ros2...ros2:uncrustify_0.67diff-5fb4c5847e65295257a425c49e8b75b4R342 Are they really necessary?

No they are not this was only the behavios of --reformat but is not necessary to pass with the new version

Can you please update the uncrustify configuration file in ament_lint. That will also show the new options and what value are chosen for them by default.

:+1: Will do if we decide to go for the new version

I am just wondering if the updated code also passes with uncrustify 0.66.1? If not, that would be a problem since that is the version we want to use on Ubuntu Bionic, no?

Some change can be made compatible with both version, for example the template closing > will pass on both if we put them on the lsame line as the last argument https://github.com/ros2/rosidl_typesupport_opensplice/compare/uncrustify_0.67#diff-901828bcfbcbe1853b61915168183521R113 But if they are indented on the new line the expected indentation is different: https://github.com/ros2/rviz/compare/ros2...ros2:uncrustify_0.67#diff-10cc67289440c9306d0985f5b12e6299R107

All the other changes are not compatible with 0.66.1. The list of failures can be found on the linux jobs referenced above.

I personally find all the indentation changes better in 0.67 as we never have to "over-indent" anymore, but dont feel strongly about the other ones. Picking 0.67 mean that we ship our own uncrustify on all platforms but MacOS. Sticking with 0.66.1, mean that we can use upstream on bionic but will ship our own on all other platforms.

dirk-thomas commented 6 years ago

I am fine changing our plan and keep building the latest version instead of relying on the Debian package.