ros2_message_filters: 2.0.0-1 in 'bouncy/distribution.yaml' [bloom]

[gaoethan]

Increasing version of package(s) in repository ros2_message_filters to 2.0.0-1:

• upstream repository: https://github.com/intel/ros2_message_filters.git
• release repository: https://github.com/ros2-gbp/ros2_message_filters-release.git
• distro file: bouncy/distribution.yaml
• bloom version: 0.6.5
• previous version for package: null

message_filters

* Change build tool from catkin to ament in CMakeLists.txt
* ROS APIs move to ROS2 APIs
* Drop boost dependency, using C++14 instead
* Add message_event.h message_trait.h parameter_adapter.h which are missing in ROS2 core
* Update existing test cases and add a fuzz test(default disable)
* Drop unused setup.py
* Add a README
* Bug fixes
* Contributor: Jing Wang, Ethan Gao

[dirk-thomas]

I would suggest to rename this line to just message_filters simply because all repositories enumerated here are for ROS 2.

As a consequence the following two lines need to be removed since the new name matches the only package then:

packages:
- message_filters

[dirk-thomas]

We haven't tested this yet for build.ros2.org. Therefore I would recommend to remove this line for now.

[gaoethan]

@dirk-thomas update with your feedback, thanks !

[mikaelarguedas]

@gaoethan this is currently failing CI because the new entry is out of alphabetical order.
You can find the diff to apply in the log of the travis job of this PR

[gaoethan]

@mikaelarguedas fixed the alphabetical order, thanks for your info.

[mikaelarguedas]

lgtm thanks @gaoethan for iterating.

Note that the doc entry is currently being ignored as there is no documentation jobs on the ROS 2 farm at the moment.

[mikaelarguedas]

@gaoethan I know this is very similar to the discussion we had in ros-perception/vision_opencv#216, but as there seem to be some overlap, I'm listing some discussion points below.

As the code on the source repository https://github.com/intel/ros2_message_filters.git appears to be a copy of the code existing in ros_comm that has then been modified to be ROS 2 compatible, there are several concerns that come up:

• The code has been copied without retaining the original commit history making it very hard to trace modifications as well as give credit to the multiple authors of that code base over the years. ROS repositories should be forked to retain history rather than only copied into a new repository.
• The package has been re-licensed Apache 2.0 while the original code base was BSD. The individual files still list the BSD license header but the repository license file as well as the license in the package.xml are Apache 2.0. They should be BSD to reflect the license of the code in the repository / package.
• The co-author and maintainer for many years of message_filters @dirk-thomas does not appear either in the author list of the package or the commit history.

I know there is currently little documentation regarding the process of porting ROS 1 code to ROS 2. We will try to summarize these points in a document on the ROS 2 wiki in the future. In the meantime, could you please update the copied/forked repositories to maintains history, license and copyright of the original code?
Thanks!

cc @testkit @jwang11 @ros2/team

[testkit]

@mikaelarguedas, thanks for bringing this to the table. We selected Apache 2.0 license for ROS2 components because we'd like to align with ROS2 Core, like https://github.com/ros2/rcl, etc. And for BSD licensed project, it's ok to convert to Apache 2.0 for redistribution per my understand. But you are right, the copyright should keep as it was and we should keep the co-author and maintainer considering most of code are contributed from them and ROS1. Let me get proper approval to change from Apache 2.0 to BSD license firstly, and then we can do the code change accordingly.

[testkit]

@mikaelarguedas, I didn't get license change approval yet since recently there is U.S. holiday. Is it ok to go with current version and update later on with new PRs?

[mikaelarguedas]

I didn't get license change approval yet since recently there is U.S. holiday.

Yes this is fine.

Is there anything blocking addressing the 2 other points ? (especially restoring the commit history of the original repository). As restoring the original history will overwrite the history of the repository, it will make the release tag 2.0.0 invalid as it won't share any history tree with the rest of the repository. So I strongly encourage to address that part before making the release.

Is it ok to go with current version

Unfortunately this package doesn't compile at the moment, the dependencies in the package.xml need to be updated to match the new set of dependencies introduced by the conversion to ROS 2.

You can follow the steps detailed in #227 (comment) to test the compilation of the package (modifying the git clone step to clone https://github.com/intel/ros2_message_filters.git instead of the vision_opencv repository)

There are also multiple declared dependencies that are not needed by the package, some examples below.

• Needed dependencies not listed in the package.xml
  • builtin_interfaces
  • sensor_msgs
• Declared dependencies not needed by the package
  • boost
  • rosidl_default_generators
  • rcutils is not used, rmw_implementation, rmw_implementation_cmake...
• Test dependencies are missing from the package.xml
  • ament_cmake_gtest
  • ament_cmke_pytest
  • ament_lint_auto

Once these issues have been addressed, a new release should be made and the newly geenrated PR will replace this one that will be closed

[testkit]

@mikaelarguedas, at to restoring the commit history, the easiest way to avoid cleaning the project (which need involve github organization maintainer) is to create a new ros2/message_filters project in github, fork the code from ros_comm, then apply all the ROS2 patches to that new project. The license can be updated to BSD naturally in this way as well. Do you think it's a better approach for long term project management?
As to dependency issue, @gaoethan is working on the fix.

[jwang11]

@testkit fork all code and commit history from ros_comm doesn't work as it not just include message filters but a bunch of other components. Message filter is just a subdirectory of ros_comm.

[testkit]

@jwang11, yes, I understand, but I'm afraid there is no good way to pick out message_filters only commits. An alternative way is to add contributors' name in package.xml but not fork all ros_comm, @mikaelarguedas, @dirk-thomas, do you have better idea?

[jwang11]

Maybe just cherrypick message_filter related commits from ros_comm if applicable

[mikaelarguedas]

I usually use git filter-branch with the -subdirectory-filter flag to filter the history of a specific directory. There are multiple tutorials online explaining how to do this, e.g. https://s-opensource.org/2016/04/26/retain-history-moving-files-git-repositories/

It is preferred to keep the commit history rather than simply adding contributors' names to the package.xml.
It is not only valuable for crediting contributors but also very usful when it comes to ebugging code and bissecting changes.

[tfoote]

There's a good tutorial here exactly splitting out a directory: https://help.github.com/articles/splitting-a-subfolder-out-into-a-new-repository/

[gaoethan]

@mikaelarguedas Thanks for your feedback, I found one issue that the build system can't find the g++ for the C++ code with and it provides the following errors while running colcon build:

CMake Error at CMakeLists.txt:2 (project):
  No CMAKE_CXX_COMPILER could be found.

  Tell CMake where to find the compiler by setting either the environment
  variable "CXX" or the CMake cache entry CMAKE_CXX_COMPILER to the full path
  to the compiler, or to the compiler name if it is in the PATH.

But it works after install g++ or build-essential
apt-get install -y g++
or
apt-get install -y build-essential

I cloned the ros2 examples of minimal_client to have a try and it has the same issues, so my question is that which package should be added as build dependency to involve g++ in ros2 ? my current package.xml is as follows:

<buildtool_depend>ament_cmake</buildtool_depend>

  <build_depend>builtin_interfaces</build_depend>
  <build_depend>rclcpp</build_depend>
  <build_depend>rclpy</build_depend>
  <build_depend>std_msgs</build_depend>
  <build_depend>sensor_msgs</build_depend>

  <test_depend>ament_lint_auto</test_depend>
  <test_depend>ament_cmake_gtest</test_depend>
  <test_depend>ament_cmake_pytest</test_depend>

  <export>
    <build_type>ament_cmake</build_type>
  </export>

[testkit]

@mikaelarguedas and @tfoote , thanks for the sharing about how to split a directory! As to the proposal of managing message_filters under ros2 github organization, do you have any comments?

[mikaelarguedas]

As to the proposal of managing message_filters under ros2 github organization, do you have any comments?

There was a discussion to that effect on the ros_comm repository ros/ros_comm#1301. I recommend asking there so that the ros_comm maintainer can give feedback on where / how they recommend the ROS 2 version to be hosted and maintained

[dirk-thomas]

I created the new repo. The ticket ros2/message_filters#2 describes what needs to happen to integrate the numerous changes from the "fork".