Adding ament_lint_common

[CursedRock17]

This pull request is meant to fix #29 because ament_lint_common is currently not in the package.xml but it should be.

[CursedRock17]

So it seems like we have two problems still

• Explicit constructors for the synchronizers. Maybe its the nature of the type system, but if we want a regular Synchronizer we would have to pass an entire Policy yet that policy is only going to want us to pass some sort of queue_size or equivalent. If our type of Synchronizer is being declared we aren't going to want to pass that same whole Policy it seems like is would be just an annoyance for users.
• Copyright issues. I have no clue what would different message_filters from something like tf2_py which is also licensed by BSD

[clalancette]

• Explicit constructors for the synchronizers. Maybe its the nature of the type system, but if we want a regular Synchronizer we would have to pass an entire Policy yet that policy is only going to want us to pass some sort of queue_size or equivalent. If our type of Synchronizer is being declared we aren't going to want to pass that same whole Policy it seems like is would be just an annoyance for users.

I guess I don't understand the problem here; can't we just add the explicit keyword to these constructors? That will make it so that the constructor doesn't coerce arguments, but I think that is generally a good thing. Or is your concern that we already depend on coercion, and this would break existing users?

• Copyright issues. I have no clue what would different message_filters from something like tf2_py which is also licensed by BSD

This is almost certainly the whitespace and other formatting concerns. Our copyright linter is very dumb, and if the copyright statement doesn't exactly match what it expects, it fails. I have a branch somewhere where I update the copyrights in message_filters; I can add those changes in here.

[clalancette]

All right, beff090 fixes the copyright problems.

But there is another problem here, and that is that message_filters isn't a "proper" C++ package. In particular, the header files still have the .h extension, and so our linters consider them to be C headers, not C++ headers. That is another problem that we should fix, but we have to do that through a deprecation cycle. Since this PR is already large enough as it is, I'm going to suggest we do that separately.

Where that leaves us is that we have to pull in the linters by hand, so we can properly tell uncrustify that this is a C++ package. I've done that in f921062, but that causes a whole host of new issues.

This PR is getting out of hand, and because it is so large, it is going to be harder later to figure out whether it caused regressions. My suggestion here is that we step back, and actually add in the linters one-by-one in separate PRs. For each linter we add in, we only fix the issues corresponding to that linter. So, for example, we could open up one PR just enabling the copyright linter, and the fixes for that. And another for uncrustify. And another for cpplint. And one for lint_cmake. And finally, one that deprecates the .h files in favor of .hpp files, which will (someday) allow us to just use auto here.

@CursedRock17 What do you think?

[CursedRock17]

I guess I don't understand the problem here; can't we just add the explicit keyword to these constructors? That will make it so that the constructor doesn't coerce arguments, but I think that is generally a good thing. Or is your concern that we already depend on coercion, and this would break existing users?

Yeah this would be the latter, I've tried plugging different things into constructors which have the explicit keyword and they don't work quite well, I believe we would need coercion which would break builds, though the other constructors not dealing with Synchronizer can have the explicit keyword and work quite well.

My suggestion here is that we step back, and actually add in the linters one-by-one in separate PRs

@clalancette This works well for me, it'll be a lot easier to break down these issues. I can submit a PR for the copyright if that's the direction we want to head and work from there, is there a certain order we need to progress in, or can I just go in the order that you listed in that last comment? Unless you already have a branch for the copyright you want me to work off of, I can create brand new PRs for these and just link them back to this PR and the main issue.

[clalancette]

I can submit a PR for the copyright if that's the direction we want to head and work from there, is there a certain order we need to progress in, or can I just go in the order that you listed in that last comment?

I don't think we need to go in any particular order, so whatever you think is best. There is no separate branch with copyright, so I'll suggest you split those changes out of here and submit them as one of those PRs.

[ahcorde]

I think we should cleanup this PR. There are still one things that I would like to change:

• Add <test_depend>ament_lint_auto</test_depend> in the package.xml
• Add the CMakeLists.txt the following lines under BUILD_TESTING

find_package(ament_lint_auto REQUIRED)
ament_lint_auto_find_test_dependencies() 

And fix the pep257 and flake8 linters issues.

[ahcorde]

remove ament_cmake_lint_cmake, ament_cmake_cpplint, ament_cmake_copyright, ament_cmake_uncrustify`

[CursedRock17]

As per the second section here: https://github.com/ament/ament_lint/blob/rolling/ament_lint_auto/doc/index.rst

Is it not correct to keep them in the test_dependencies or am I reading the documentation wrong?

[clalancette]

Is it not correct to keep them in the test_dependencies or am I reading the documentation wrong?

They are unnecessary in this case because ament_lint_common brings them all in indirectly.

[CursedRock17]

Makes sense now, I'll have it updated in about 4 hours.

[ahcorde]

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[ahcorde]

Some more linters issues https://ci.ros2.org/job/ci_linux/21546/

[ahcorde]

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[ahcorde]

unrelated warnning on windows

[clalancette]

unrelated warnning on windows

That warning is not unrelated; it is directly related to what was merged here. The reason we don't see it on Linux is because cppcheck is disabled there, because it is too slow.

What we'll need here is a follow-up PR to force cppcheck to consider this package C++ instead of C. @ahcorde can you look into that?

[ahcorde]

unrelated warnning on windows

That warning is not unrelated; it is directly related to what was merged here. The reason we don't see it on Linux is because cppcheck is disabled there, because it is too slow.

What we'll need here is a follow-up PR to force cppcheck to consider this package C++ instead of C. @ahcorde can you look into that?

Sure, #138