ros2 release

[malban]

I'd like to get csm and https://github.com/CCNYRoboticsLab/scan_tools released for ros2 (galactic, foxy, etc) based on the csm_eigen version.

I created a ros2 branch on my fork, branched off from csm_eigen: https://github.com/malban/csm/tree/ros2 that uses ament_cmake: https://docs.ros.org/en/foxy/How-To-Guides/Ament-CMake-Documentation.html.

Once a ros2 branch is made, then the repo can be bloomed to the ros2 releases: https://docs.ros.org/en/foxy/How-To-Guides/Releasing-a-ROS-2-package-with-bloom.html

[130s]

+1 for porting for ROS2 (even though CSM is agnostic to ROS). I can think of a few things to verify before taking an action.

Background of why CSM needs a work to be ported to ROS2

Just as a recap for the larger audience,

• scan_tools, a de-facto module in the robot operation system (ROS) that interfaces with CSM. Many users might have installed CSM via scan_tools using the installer that is built on the buildfarm on ros.org by using a package manager (e.g. apt on Ubuntu/Debian).
• New generation of ROS, called ROS2, is backward incompatible with older ROS (ROS1).
  • Most notable for CSM's context is that ROS1 and ROS2 use different build tools ("Catkin" for the former, "Ament" or "Colcon" for the latter).
    • AFAIUC, a package that is configured for Catkin CAN still be built by ament, but not the other way around.
• ROS2 buildfarm on ros.org doesn't use Catkin to build the installers. It probably uses ament or colcon.

Does CSM need to be ament-ified?

@malban Have you tried to build scan_tools (e.g. your ROS2 branch) along with CSM with ament/colcon withOUT CSM to be ament-ified?

If my theory in the background section above is correct, then I expect CSM to be still built w/o issues by ament/colcon. If so, we may not need a change in CSM in order to port scan_tools to ROS2 (benefit of not making a new branch is obviously that we won't have to maintain).

My guess is that it may build on your local host (if your bash terminal has somehow been configured for ROS1 along with ROS2 so that ament can reference to those CMake macros catkin provides e.g. answers.ros.org#318969). But it will unlikely build on the ROS2 buildfarm, as buildfarm shouldn't be configured for both ROS2 and ROS1.

So in a nutshell I'm leaning toward branching for ament, but I just want to see what happens.

[malban]

That's a good question. I don't think the CSM branch used to for ros1 even uses catkin. It claims to have a runtime dependency on catkin: https://github.com/AndreaCensi/csm/blob/1.0.2/package.xml#L26, but that's the only reference I can find.

I think pkg-config is used instead of cmake_catkin to handle discovery of csm in cmake in the catkin workspace: https://github.com/CCNYRoboticsLab/scan_tools/blob/indigo/laser_scan_matcher/CMakeLists.txt#L21

The pkg-config file is installed from csm at /opt/ros/noetic/lib/pkgconfig/csm.pc, which appears to be good enough for it to be found by the scan_tools packages.

Though, it doesn't look like the package.xml file is installed in the share directory, where I would it expect it, so I'm not quite sure how catkin is completely happy.

The same general approach should presumably work fine for the ros2/ament/colcon case as well.

The one issue is that I would like the version built for ros2 to be based on the csm_eigen branch, which replaces the dependency on the GNU Scientific Library (GSL) with eigen. The cmake files are a bit different on that branch and it doesn't have the pkg-config setup, but I'll see if I can replicate what the main branch is doing.

Installing a pkg-config file, like on the main branch, would avoid making ament a build dep and then the csm_eigen branch could be used directly instead of needing a new ament branch.

[130s]

Sorry I misunderstood but looks like CSM is a pure cmake package (nice!), not a Catkin pkg #34 (comment)

With that, is the reason why you want to add changes to CSM because of eigen situation? If so, as in my review comment #34 (review), we need to figure out what to do for csm_eigen branch. If possible, we may just cherry-pick the necessary eigen changes into the main branch?

Though, it doesn't look like the package.xml file is installed in the share directory, where I would it expect it, so I'm not quite sure how catkin is completely happy.

That might be because csm_eigen branch is missing the install rule that was added in #28 into the main branch?

Anyways, I now do not see a necessity to make changes in CSM to port it to ROS2, but I might be wrong.

[malban]

That might be because csm_eigen branch is missing the install rule that was added in #28 into the main branch?

Is that what this does?

  <export>
    <build_type>cmake</build_type>
  </export>

Otherwise I don't see anything that might cause package.xml to be installed

[malban]

With that, is the reason why you want to add changes to CSM because of eigen situation? If so, as in my review comment #34 (review), we need to figure out what to do for csm_eigen branch. If possible, we may just cherry-pick the necessary eigen changes into the main branch?

Right. I would like the ROS package to use eigen instead of GSL. For the time being my use case is ROS2, so no need to disturb the existing ROS1 configuration unless it's necessary.

The primary reason is that GSL is GPLv3, which is copyleft for anything that links against it, at minimum. Eigen, on the other hand uses, MPL2, which is only copyleft at the file level, so doesn't propagate outside source file boundaries.

Most of the ROS ecosystem is BSD licensed and full GPL is usually avoided since it's often not compatible with commercial use cases.

Eigen is also a much easier library to work with and is common in the ROS ecosystem.

It's not clear that the fact that the ros1 package for csm, and therefore scan_tools, link against a GPLv3 library is well known or indicated anywhere. It's documented that csm is LGPLv3, but I don't think that changes the fact that GSL is also linked against.

It may not be a big issue because there is some indication the ROS style message passing breaks the link so to speak, to prevent GPL from propagating, but I don't know if that's an actual legal fact:

• http://wiki.ros.org/DevelopersGuide#Licensing
• https://answers.ros.org/question/12313/using-gpl-licensed-packages-in-a-commercial-product/

Regardless, since there is an existing eigen branch that has removed linking to GSL, I think that is most appropriate for ROS.

I could be wrong, but I don't think there is much if anything in the csm_eigen branch that would need to get merged into main. I think the eigen branch just builds with the gsl_eigen shim and doesn't link to GSL, while the main branch links to GSL instead of using the shim.

[130s]

• Branching: I see, I'm convinced to vote for creating a new branch for newer versions of this library (CSM) -- Switching from GSL to Eigen sounds like a (almost?) breaking change.
  • Whether we keep using csm_eigen branch as a (temporary) bleeding edge branch for eigen version of CSM, or not is up to maintainers. I'm good for now (I can't remember why I'm appointed as a maintainer of this repo..).
  • I'd avoid changing anything regarding GPL unless you're certain..
  • There's a few PRs that were only merged into main. Maybe it's worth cherry-picking those too.
• Installation rule: I can't remember where I looked when I said the following in ros2 release #33 (comment). There IS already an install rule for package.xml ONLY in csm_eigen branch AFAICT, which is weird to me. I don't know then why package.xml isn't installing:

  That might be because csm_eigen branch is missing the install rule that was added in Fix ROS builds #28 into the main branch?

[malban]

Regarding the package.xml file. I kept that in the updated CMakeLists.txt file of my PR on the csm_eigen branch, which seems to work fine for me.

Installing the noetic debian version of csm indeed does not seem install a package.xml file, but that seems like a different issue on it's own. 🤷

I'll take a closer look at the differences between the main branch and csm_eigen. Ideally the only difference would be the build scripts from a maintainability point of view.

[malban]

I reviewed the differences between csm_eigen and master. At a high level csm_eigen has the following differences:

• reorganized dir structure to have top level include and src dirs
• converted of code from C to C++ (relatively minor changes)
• added gls_eigen code to replace GSL
• missing sm_icp_xy function: 17768c9 (alternate scan matching method?)
• removed
  • cairo dependency and related draw functions
  • GSL dependency (replaced with eigen)
  • misc, website, scripts, local_config, manual top level dirs
  • TODO.txt, install_quickstart.sh, Doxyfile, csm_manual top level files
  • sm/apps folder
  • sm/lib/json_c folder and related JSON output functions and calls
  • sm/csm/gpm (alternate scan matching method?)
  • sm/csm/hsm (alternate scan matching method?)
  • sm/csm/mbicp (alternate scan matching method?)

So, basically, the two branches are pretty diverged even though the core algorithm is the same. I'll look at making a MR to pull in the missing sm_icp_xy function, which was probably added after csm_eigen was created.

The ROS2 branch I'm working on for scan_tools: https://github.com/malban/scan_tools/tree/galactic works fine with the csm_eigen branch once addressing the build/install issue: #34

[130s]

@malban Thanks for the detailed look, and offering to sync Eigen branch. That'll be great as I continue receiving users' inquiries, which once again demonstrate the usefulness of CSM and scan tools. Once the work is done, I think it's worth making an announcement at a larger community (e.g. discourse.ros.org).

[malban]

@130s I made a PR to port the sm_icp_xy method to the csm_eigen branch: #35

[130s]

Hi @AndreaCensi and the maintainers, I put together some suggestions. Please review and give approvals/opinions. I'm no longer an active user but happy to help as much as I can.

Suggested direction

long story short (i.e. no need to read previous conversations unless you're curious), I have a couple of suggestions summarized as following, after communicating with @malban who's looking into the detail:

• Suggestion1. Make the branch that uses Eigen instead of GSL the default.
  • Rename the branch from csm_eigen to something more generic e.g. develop.
    • If we do this rename, I suggest renaming master to something like develop_gsl (following branching model like ref. the famous git-model (nvie.com))
• Suggestion-2. Start merging the changes that are needed to adjust to "ROS2", a new version of ROS that requires the packages some different configuration.
  • Changes for ROS2 can be only merged into the branch for Eigen.

For suggest-1, as outlined in #33 (comment), GSL makes using CSM harder due to software license, esp. in the industry. The less license issues, the more user contributions we can get I believe. @malban is trying hard to re-use csm_eigen branch, which seems to allow CSM to work with Eigen, but is very outdated (See the review #33 (comment)).

Adding to suggest-2, I don't see a need to make a release regarding Eigen-related changes into ROS1, unless users request, since there's no plan in the world of ROS to make a new release of ROS1 any more (except for bug fixes) and CSM is already released into all ROS1 distributions.

Actions

Based on assumption that the suggested direction above is ok,

• Maintainers agreed with a direction
• Rename csm_eigen.
• Update readme, explain branching. --> doc: Clarify branching after Eigen discussion  #37
• ROS side of things
  • Make a new release into ROS2.
  • Update rosdistro about the branching for ROS1. Similar to Update imu_tools and scan_tools URLs ros/rosdistro#32728, if rosdistro is not aware of the change in dev repo info (repo name, branching etc.), building installer would start failing.

[cst0]

Regarding GPLv3/copylefting/etc: the "application service provider" loophole in GPLv3 is something I've wondered about for a while. My reading is that it means that ROS users won't be impacted by copylefting requirements, since DDS (driving ROS2) provides a network-based service. But, I'm no lawyer. MPL is closer to the intended license than GPLv3: if the intent was for copylefting to spread beyond this repository, it would have been made GPLv3 to begin with.

As a bonus, it's also widely used in ROS (as already mentioned), and I found this blog post benchmarking GSL vs. Eigen for eigenvalue calculations: Eigen is the winner, speed-wise.

So overall, I'm in agreement: the proposed shift to Eigen (and follow-up requirements of updating repositories, etc) makes sense to me, and the ROS2 port is of course valuable. I think it would be nice (in theory) to backport some of these changes to Noetic... but it's hard to justify that time cost, so leaving the ROS1 code mostly as-is makes sense to me too.

[malban]

My reading is that it means that ROS users won't be impacted by copylefting requirements, since DDS (driving ROS2) provides a network-based service.

I agree that this seems like the common sense reading and is described on the ROS wiki pages as such.

But, I'm no lawyer.

Same.

I think it would be nice (in theory) to backport some of these changes to Noetic

As far as I can tell, the csm_eigen and master branches have identical algorithms and interfaces, at least when it comes to the core scan matcher, which is what is being used by the laser_scan_matcher in ros.

I think the cmake build config here: #34, should be agnostic to ros1/ros2 catkin/ament, so it's probably more of a decision to test/release for noetic from the csm_eigen branch instead of the master branch.

[RainerKuemmerle]

As far as I can tell, the csm_eigen and master branches have identical algorithms and interfaces, at least when it comes to the core scan matcher, which is what is being used by the laser_scan_matcher in ros.

At the time of creation of the csm_eigen branch, we used Eigen instead of GSL (easier portable, less restrictive license). There was no change in the core of the algorithms, just a small glue code was added to use Eigen underneath a GSL syntax. We also dropped a lot of stuff which we did not need (or simply could not compile easily on our target OS at the time).

[130s]

No objection heard AFAICT since the suggestion #33 (comment) last month. I'm moving forward.

• I pushed a new branch master_eigen, which I made based off of csm_eigen.
• I made doc: Clarify branching after Eigen discussion  #37 to clarify the new branching direction discussed in this ticket.