ROS2/ROS1 compatibility jinja discussion

[marinagmoreira]

Discuss issues in: https://github.com/nasa/astrobee/pull/582

Issues raised:

1. every time before colcon buildwe need to generate things - would be nice to automate this, not sure if we can specify project-based build rules that would automatically call the script whenever we do colcon build;
2. right now, files become jinja-, which is not good, because it's harder to look for them alphabetically. Maybe something like abc.jinja.msg is better;
3. a modification to a jinja file, even if not in the scope of jinja, still requires jinja to be updated (since the file will be replaced in-place under src/);
4. This one is easy to fix, but: git_src and src cannot be under the same workspace, otherwise colcon will complain of packages with repeated names, so git_src needs to a) be ignored by colcon, or b) moved to another folder (a hidden folder could probably work).

Trey's response:

/1. every time before colcon buildwe need to generate things - would be nice to automate this, not sure if we can specify project-based build rules that would automatically call the script whenever we do colcon build; /3. a modification to a jinja file, even if not in the scope of jinja, still requires jinja to be updated (since the file will be replaced in-place under src/);

I had the same thought but didn't yet investigate if there is an appropriate customizable pre-build hook to use so developers don't need to remember to run the script again.

2. right now, files become jinja-, which is not good, because it's harder to look for them alphabetically. Maybe something like abc.jinja.msg is better;

That sounds easy. Instead of f.startswith("jinja-") it could be f.startswith("jinja-") or ".jinja" in f. And likewise for ros1- and ros2-.

4. This one is easy to fix, but: git_src and src cannot be under the same workspace, otherwise colcon will complain of packages with repeated names, so git_src needs to a) be ignored by colcon, or b) moved to another folder (a hidden folder could probably work).

This might be a place where Astrobee's unusual folder conventions are causing confusion?

As I understand it:

1. A typical ROS workspace might have multiple source checkouts in a single workspace like:

   ~/ros_ws  # workspace
   ~/ros_ws/src  # where catkin/colcon does package discovery, checkouts in multiple subfolders often managed by vcs
   ~/ros_ws/src/std_msgs  # source checkout 1
   ~/ros_ws/src/trajectory_msgs  # source checkout 2

2. However, the Astrobee installation instructions recommend a simpler one-repo workspace structure which requires one less level of folders:

   ~/astrobee  # workspace
   ~/astrobee/src  # source checkout of astrobee repo, top level of checkout renamed to 'src' instead of 'astrobee'

   I think if you're in case 2 the script works fine as written, at least under ROS1, because the git_src folder is not within the src folder used for package discovery. But if your workspace is structured like case 1, its path conventions don't make sense. It would be easy to add a feature to the script to support alternative path conventions. (And perhaps we should also reconsider our unusual recommended workspace structure.)

[Pedro-Roque]

A few comments on this, a couple of times I was trying things out by moving packages or files in the src directory, but that breaks the symlink. Example: I was trying the colcon.pkg file and had it on root first. Then I moved it in src to another package to try to build, but it broke the symlink. So this comes back to issue (1).

The issue with jinja naming and colcon build failure have been addressed here: https://github.com/nasa/astrobee/pull/584

[bcoltin]

I would add to the questions:

1) Jinja2 will introduce the same issues "#define"s are generally frowned upon for and discouraged by the google style guide we use. But likely more confusing since we are doing a homegrown thing and changing file names. https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros 2) How many files will need a jinja2 double version? 3) Will this save work compared to having a ros1 and a ros2 branch and merging changes from ros1 to ros2, or introduce extra work and confusion?

• All developers will need to be develop some familiarity with ros2 immediately
• Every commit will need to be tested to work in both ros1 and ros2
• Extra logic to manage to work with both at same time vs. one ros1 version and transition it to ros2 in separate pull request 4) This will create extra work to get rid of it once we transition fully to ros2.

[trey0]

Pedro's comments 1-4 above should be addressed in #589.

His comment 4 about the naming convention git_src and src may need further discussion. My remaining questions are:

1. Does this naming convention cause any problem when you follow the directory structure assumptions from the astrobee build instructions? (Hopefully no.)
2. If Pedro strongly desires to use a different directory structure, does the flexibility of the new command-line arguments satisfy his needs?

[trey0]

1. Jinja2 will introduce the same issues "#define"s are generally frowned upon for and discouraged by the google style guide we use. But likely more confusing since we are doing a homegrown thing and changing file names. https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros

I think you may have two issues in mind here: (A) Jinja2 template expansion can make it harder to determine what the actual source code is and where it came from. (B) Using #define macros in a ros_compat.h like file (for example to enable different field names between ROS1 and ROS2) could cause further confusion.

I see both of these issues as valid but manageable. For (A), we should both avoid using Jinja2 when there are other more native solutions for a particular file format, and keep the Jinja2 conditionals simple and easy to understand. For (B), we can look for ideas in that Google style guide. One of their suggestions is to prefix macros with a string based on the header name so they are easier to trace and guaranteed unique--which matches my earlier suggestion of a consistent FF_ROS_ prefix. Another of their suggestions is to prefer inline functions in a namespace (e.g., ff_ros::) to macros, which I also think is a good idea when it's workable.

2. How many files will need a jinja2 double version?

So far I count 14 *.msg and *.srv files which use Jinja2 conditionals in a very simple way. It seems hard to avoid using Jinja2 here because *.msg files don't have a native conditional syntax and the ROS filename conventions really push us to use the same *.msg filename for both ROS1 and ROS2.

We may discover more places that need Jinja2 later, of course. But I'm happy we've been able to discover native conditional syntax in places like package.xml, CMakeLists.txt, etc. and the COLCON_IGNORE files are an easy way to skip building packages that haven't yet been migrated to ROS2.

3. Will this save work compared to having a ros1 and a ros2 branch and merging changes from ros1 to ros2, or introduce extra work and confusion?

Of course, it will both save work in some places and cause extra work in others.

• All developers will need to be develop some familiarity with ros2 immediately

Yes, but it may be minimal. If you are editing a package that hasn't been migrated to ROS2 yet (marked with COLCON_IGNORE), you don't need to do anything different. And we can use this to our advantage by ordering which packages we migrate first to delay the ones that are actively being edited.

• Every commit will need to be tested to work in both ros1 and ros2

Dual testing would apply only to packages that have already been ported to ROS2. I assume we'll develop CI testing workflows for ROS2 and run them side-by-side with ROS1.

In terms of what other testing we feel is needed beyond the CI workflows, just like now, it will depend on the nature of the change. In practice, more rigorous testing under ROS2 might only be needed when we are about to deploy the ROS2 version to ISS.

• Extra logic to manage to work with both at same time vs. one ros1 version and transition it to ros2 in separate pull request

Here is actually where I expect a universal ROS1/ROS2 package to save effort.

If a change affects code that is shared between the ROS versions (i.e., not within a ROS1 conditional branch), you can make the change once and don't need to transition it to another branch at all.

I notice many of our PRs affect lots of files (10-20 or more). I think these kinds of PRs would be quite painstaking to migrate to another branch, with the likelihood of say missing one modified file. And comparing ROS1 vs. ROS2 PRs in code review would be a huge pain.

I'm also concerned about having separate PRs for the same change across ROS1 and ROS2 branches. I feel like our accountability mechanisms are likely to be leaky. I predict a common case is that the ROS1 PR will be prioritized for development and review as a rush job (for an ISS activity) and the ROS2 PR will be delayed then forgotten.

4. This will create extra work to get rid of it once we transition fully to ros2.

Yes, but we can structure the universal package so that this is almost a global search-and-replace operation. (Strip off FF_ROS_ and ff_ros:: prefixes, replace the Jinja2 template file with the ROS2 version of the Jinja2 output file, then some manual editing to remove the ROS1 branch of a bunch of conditionals.)

[marinagmoreira]

Closing this as we've reached a useable jinja setup and a plan to phase it out in the future