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

tf2: Enable common linter tests #469

Merged
merged 6 commits into from
Dec 18, 2021
Merged

tf2: Enable common linter tests #469

merged 6 commits into from
Dec 18, 2021

Conversation

aprotyas
Copy link
Member

@aprotyas aprotyas commented Oct 17, 2021 •
edited
Loading

As discussed in #467, this PR:

  • Enables a subset of relevant linter tests from ament_lint_common in tf2.
  • Excludes LinearMath headers (include/tf2/LinearMath) from being linted.
  • Fixes line-length formatting issues in a few source files, so that there are no lint failures in the current state.

Note that individual linters had to be invoked rather than use ament_lint_auto because the LinearMath headers should not have linters run on them - see #258 (comment) for more information - and because ament_lint_auto does not provide a general way of providing file exclusion lists.

Signed-off-by: Abrar Rahman Protyasha [email protected]

@aprotyas
Copy link
Member Author

aprotyas commented Oct 17, 2021 •
edited
Loading

I'm marking this PR as draft because it is blocked by some PRs in ament/ament_lint, namely ament/ament_lint#327, ament/ament_lint#328, ament/ament_lint#329, ament/ament_lint#330, and a forthcoming PR to address file exclusion behavior in ament_uncrustify.

The Rpr job is failing because said PRs haven't landed.

@aprotyas aprotyas requested a review from clalancette October 17, 2021 23:35
clalancette
clalancette reviewed Oct 19, 2021
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

The changes here look generally fine to me, though in order to move forward here we need to:

  1. Rebase this on the latest
  2. Get the ament PRs in
  3. Get green CI here

@aprotyas
Reformat copyright header in test/test_time.cpp
c3f7b00 
The copyright header in test/test_time.cpp was failing
`ament_copyright` checks otherwise.

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
@aprotyas
Fix line-length formatting issues in a few files
6bcf949 
`ament_cpplint` reported a few formatting issues, all to do
with lines longer than 100 characters.

These issues have been addressed with `ament_uncrustify --reformat`.

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
@aprotyas
tf2: Enable common linter tests
e7e279c 
Each of the common linters were individually `find_package`-d
and invoked in the CMakeLists.txt file. This is because there
is no simple way to pass file exclusion information through
`ament_lint_auto`.

File exclusion is necessary because we do not want to run linters
on the include/tf2/LinearMath/ headers, since these are
external and periodically synced with upstream. See
#258 (comment)
for more information.

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
@aprotyas
Copy link
Member Author 

1. Rebase this on the latest

Will do. By the way, @clalancette as I was rebasing on the ros2 branch, I noticed that #466 and #467 were rebased onto that branch rather than squash-and-merged.

@aprotyas aprotyas force-pushed the aprotyas/tf2_enable_common_linters branch from 32a7ae1 to e7e279c Compare October 19, 2021 15:55
@clalancette
Copy link
Contributor

Will do. By the way, @clalancette as I was rebasing on the ros2 branch, I noticed that #466 and #467 were rebased onto that branch rather than squash-and-merged.

Yeah, that was on purpose. The changes were different enough in each commit that I didn't think squashing was appropriate.

@aprotyas
Address uncrustify warnings
68ba93b 
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
This was referenced Dec 1, 2021
Closed
Merged
@aprotyas
Address cpplint include ordering
8c576e6 
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
@aprotyas aprotyas marked this pull request as ready for review December 16, 2021 07:39
@aprotyas
Copy link
Member Author

Marking this PR as ready-for-review following release 0.11.3 of ament_lint, which introduced most of the lint infrastructure needed for this PR.

CI:
Repos file: https://gist.githubusercontent.com/aprotyas/3ff1c0006e565a416f4ea37cfe03970a/raw/ef23924c2fbabffb796176d74acb34b21b6f67e2/ros2.repos
Build args: --packages-above-and-dependencies tf2
Test args: --packages-above tf2
Job: https://ci.ros2.org/job/ci_launcher/9514/

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

@aprotyas
Fix indendation in CMakeLists.txt file
566b12a 
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
@aprotyas
Copy link
Member Author

Green CI + approval, so I'll merge this. Thanks for the review!

@aprotyas aprotyas merged commit 9ff7dad into ros2 Dec 18, 2021
@delete-merged-branch delete-merged-branch bot deleted the aprotyas/tf2_enable_common_linters branch December 18, 2021 05:50
@aprotyas aprotyas mentioned this pull request Apr 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aprotyas @clalancette