Maint/roslint socketcan bridge

[JWhitleyWork]

Fixes linter errors in socketcan_bridge and adds the roslint hook to run_tests. This means that any roslint errors will cause a failed build in CI. This is not required but is strongly encouraged in ROS2. It's also just generally good practice.

[JWhitleyWork]

FYI - I would like to apply this to all of the packages in here and rebase my crystal-devel branch on them. Linting errors are the biggest divisors between the ROS1 and ROS2 versions of a package and if we get them close enough, we may be able to port many changes back and forth. Thoughts @ipa-mdl?

[mathias-luedtke]

FYI - I would like to apply this to all of the packages in here and rebase my crystal-devel branch on them.

I am okay with using roslint for socketcan_bridge, it was meant to follow the ROS(1) style guide. However, I don't like the ROS1 C++ style and I don't agree with the choices implemented in roslint.

I am okay with the new ROS2 style (like https://github.com/JWhitleyAStuff/ros_canopen/commit/4b2a39954b973962588bc03888438116d1d81d7a), so we can share this between ROS1 and ROS2.

Perhaps we can find some way to use the ROS2 linter (in parts) for ROS1 as well.

[JWhitleyWork]

@ipa-mdl - Actually... I have found a way to make a package ROS1 and ROS2 compatible. See https://github.com/astuff/ml_classifiers/pull/2 for an example. If I instead do this to the melodic branch, we can use the ROS2 linting tools as the standard. Thoughts on this?

[gavanderhoorn]

@JWhitleyAStuff: there are 29 commits in https://github.com/astuff/ml_classifiers/pull/2. Could you describe which specifically make the package ROS 1 and ROS 2 compatible?

[JWhitleyWork]

@gavanderhoorn - Sure. So, the basics are:

• In package.xml, use Package Format 3 which includes the condition attribute on nearly all tags. This allows you to specify which ROS_VERSION the tag applies to.
• In CMakeLists.txt, grab the environmental variable $ROS_VERSION and use an if() to only apply sections of the file to each ROS version.
• For the majority of files which do not rely on ROS (pure C++ objects), no major modification is necessary. The only caveat might be removing references to boost (not strictly required, but no longer needed since C++11 is supported in Kinetic and beyond).
• Rename all header files to .hpp.
• Apply ROS2 linting style and remove roscpp_lint from ROS1 checks (only if ROS2 style is preferred over ROS1). The coding styles of ROS1 and ROS2 conflict so you must choose your preferred style and use the linter in that ROS version. Both will compile with either style but only one linter will pass without error.

[gavanderhoorn]

• In package.xml, use Package Format 3 which includes the condition attribute on nearly all tags. This allows you to specify which ROS_VERSION the tag applies to.

• In CMakeLists.txt, grab the environmental variable $ROS_VERSION and use an if() to only apply sections of the file to each ROS version.

Ah, yes, ROS_VERSION predicates.

That has got to be the ugliest solution to this that could've been implemented in v3 manifests.

Doesn't matter for your PR of course. As long as it works for you.

[JWhitleyWork]

@gavanderhoorn - I agree that it isn't pretty but it does work and the package builds and runs in both environments. Maybe they'll come up with something nicer in the future (maybe an additional enclosing tag with a ros_version attribute?) but, for now, it does the job. What are your and @ipa-mdl's thoughts on implementing this here on crystal-devel, which would enforce the ROS2 lint style and maintain backwards-compatibility?

[JWhitleyWork]

@ipa-mdl - thoughts on my comment above?

[mathias-luedtke]

thoughts on my comment above?

I did not have much time until now.. I think it is a good idea, to use the ROS2 style for both version and we can migrate to *.hpp headers as well (*.h backwards support for ROS1).

However, I don't want to create "hybrid" catkin/ament packages for now. Instead we should shape the CMakeLists.txt files in a similar way. This will make a later migration to hybrid packages much easier :) I could imagine to create some helper packages that offer some macro to streamline this. We should try to find a way to support the ROS2 style in ROS1 as well.

The only caveat might be removing references to boost (not strictly required, but no longer needed since C++11 is supported in Kinetic and beyond).

Boost will stick at least for threads, asio and ptree Pointers, chrono etc. might get migrated. And C++11 (actually C++14) is only an option for melodic. Kinetic will stay unchanged, with API compatibility if possible.

[JWhitleyWork]

@ipa-mdl - So, the easiest way to validate the ROS2 style is using ament_lint. However, this obviously requires the ament build system. The next easiest way would be to fork ament_lint and make it compatible with catkin. The last and most cumbersome option is to reproduce the CMake files from ament_lint inside ros_canopen. The major components of ament_lint that are used to validate the format are ament_cmake_cpplint and ament_cmake_uncrustify which both call existing binaries, passing a handful of options specific to ROS2's format. Thoughts on how to handle this would be helpful.

I agree with your points on Boost. I guess there's no need to change anything prior to Melodic in this regard since Lunar and before didn't "technically" support C++11, even though the versions of Ubuntu that they were targeted at did.

[mathias-luedtke]

For now we could omit the validation in ROS1. It should not be too hard to port the linters..

[mathias-luedtke]

This could be added as an industrial_ci test, or as a bare travis script:

docker run --rm -v /home/mathias/melodic/src/ros_canopen/socketcan_interface/:/src ros:crystal ament_uncrustify /src

[JWhitleyWork]

I'll switch to using the crystal docker for validation. Might even be able to get it to output the JUnit XML files and have Catkin parse them. Thanks for the suggestion.