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.

Sign up for GitHub

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

Jump to bottom

Fix ament_lint_cmake line length expression #236

Merged
merged 4 commits into from
Feb 12, 2021
Merged

Fix ament_lint_cmake line length expression #236

merged 4 commits into from
Feb 12, 2021

Conversation

cottsay
Copy link
Contributor

@cottsay cottsay commented May 6, 2020

This regular expression is using the re.VERBOSE flag, meaning that characters after an un-escaped '#' character are interpreted as a comment and are not part of the expression. From what I can tell, this has been broken ever since the code was introduced. We've essentially only been getting proper line length detection for strings in a message() call.

Unfortunately, there are currently 792 line length violations in ROS 2 that have been unnoticed because of this bug: Build Status

Personally, I believe that 80 characters may be a bit aggressive for CMake. Maybe we should try changing this limit to a larger number, fix the violations and merge this fix to prevent more regressions, and then maybe bring the limit down once the enforcement is working right. I'm open to suggestions.

@cottsay
Fix ament_lint_cmake line length expression
ca73967 
This regular expression is using the re.VERBOSE flag, meaning that
characters after an un-escaped '#' character are interpreted as a
comment and are not part of the expression.

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay cottsay added the bug Something isn't working label May 6, 2020
@cottsay cottsay self-assigned this May 6, 2020
@cottsay
Merge branch 'master' into cmake_linelength
25b8018
@cottsay
Set the default max line length in CMake to 120
68bd7c2 
I'm setting this in the CMake instead of the cmakelint source code to
avoid further diverging from the upstream cmakelint code.

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay
Copy link
Contributor Author

cottsay commented Jan 28, 2021 •
edited
Loading

Trying again with the maximum set to 120 columns:
Build Status

EDIT: 120 columns yields 60 violations across ~22 packages

ivanpauno
ivanpauno approved these changes Jan 28, 2021
View reviewed changes
Copy link

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM! (when CI is green)
If the max line length is changed again to make more packages pass, that still looks good to me.

@cottsay
Try a maximum line length of 140 in CMake
8d06e14 
Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay
Copy link
Contributor Author

cottsay commented Jan 28, 2021

Trying again at 140 - maybe after we get this merged, we can ratchet it back to 120 someday.
Build Status

cottsay added a commit to ros2/spdlog_vendor that referenced this pull request Feb 11, 2021
@cottsay
Shorten some excessively long lines of CMake
21ceab2 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay added a commit to ros2/rmw that referenced this pull request Feb 11, 2021
@cottsay
Shorten some excessively long lines of CMake
0d61858 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay added a commit to ros2/rmw_implementation that referenced this pull request Feb 11, 2021
@cottsay
Shorten some excessively long lines of CMake
3cf4fe9 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay added a commit to ros2/rosbag2 that referenced this pull request Feb 11, 2021
@cottsay
Shorten some excessively long lines of CMake
668bae7 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay added a commit to ros2/rosidl that referenced this pull request Feb 11, 2021
@cottsay
Shorten some excessively long lines of CMake
515a42d 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay added a commit to ros2/rmw_dds_common that referenced this pull request Feb 11, 2021
@cottsay
Shorten some excessively long lines of CMake
0f400f6 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay added a commit to ros2/rcutils that referenced this pull request Feb 11, 2021
@cottsay
Shorten some excessively long lines of CMake
9dbf51b 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
This was referenced Feb 11, 2021
Merged
Merged
Merged
Closed
Merged
Merged
Merged
Merged
Merged
@cottsay
Copy link
Contributor Author

cottsay commented Feb 12, 2021

Here's the CI build targeting the 9 packages with 140 column violation, on branches with the violations resolved. Once the linked PRs are in, I'll merge this one.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

nuclearsandwich pushed a commit to eProsima/foonathan_memory_vendor that referenced this pull request Feb 12, 2021
@cottsay
Shorten some excessively long lines of CMake (#44)
ac8e182 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay added a commit to ros2/rcutils that referenced this pull request Feb 12, 2021
@cottsay
Shorten some excessively long lines of CMake (#328)
d413e13 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay added a commit to ros2/rmw_dds_common that referenced this pull request Feb 12, 2021
@cottsay
Shorten some excessively long lines of CMake (#44)
37d16b8 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay added a commit to ros2/rmw_implementation that referenced this pull request Feb 12, 2021
@cottsay
Shorten some excessively long lines of CMake (#179)
dadb0ec 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay added a commit to ros2/rosbag2 that referenced this pull request Feb 12, 2021
@cottsay
Shorten some excessively long lines of CMake (#648)
10fff0d 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
clalancette pushed a commit to ros-visualization/python_qt_binding that referenced this pull request Feb 12, 2021
@cottsay
Shorten some excessively long lines of CMake (#99)
0af15ba 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay added a commit to ros2/rmw that referenced this pull request Feb 12, 2021
@cottsay
Shorten some excessively long lines of CMake (#300)
ae0553a 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay added a commit to ros2/rosidl that referenced this pull request Feb 12, 2021
@cottsay
Shorten some excessively long lines of CMake (#571)
57befb8 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay cottsay merged commit b889bd8 into master Feb 12, 2021
@delete-merged-branch delete-merged-branch bot deleted the cmake_linelength branch February 12, 2021 18:09
@clalancette
Copy link
Contributor

This is probably the cause of some failing tests in CI, like: https://ci.ros2.org/view/nightly/job/nightly_osx_release/1964/ . I'm guessing that we just need to fix up the cmake files in rmw_connext_cpp, which should fix this particular issue.

cottsay added a commit to ros2/rmw_connext that referenced this pull request Feb 14, 2021
@cottsay
Shorten some excessively long lines of CMake
01caa0b 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay added a commit to ros2/rosidl_typesupport_connext that referenced this pull request Feb 14, 2021
@cottsay
Shorten some excessively long lines of CMake
6eb3c3c 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
This was referenced Feb 14, 2021
Merged
Merged
cottsay added a commit to ros2/rosidl_typesupport_connext that referenced this pull request Feb 14, 2021
@cottsay
Shorten some excessively long lines of CMake (#66)
7ae141c 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay added a commit to ros2/rmw_connext that referenced this pull request Feb 14, 2021
@cottsay
Shorten some excessively long lines of CMake (#486)
fd3e6a7 
The line length enforcement in ament_lint_cmake has been broken for some
time, but will be fixed by ament/ament_lint#236. This change brings this
package into compliance with a 120 column limit.

Signed-off-by: Scott K Logan <logans@cottsay.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivanpauno ivanpauno ivanpauno approved these changes

Assignees

@cottsay cottsay

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cottsay @clalancette @ivanpauno