Fix ROS message migration rule tool

[efernandez]

Description of the buggy corner case not handled on the rosbag migration tool

Consider the following scenario for a message G with the following versions:

G0 -- rule.bmr --> G1 -- auto-generated rule --> G3

where G3 is the current msg, and G2 is the bag message that needs migration.

We need to check for a migration rule like this first:

G2 --> G3

instead of the backwards ones that were generated before this bug fix:

G2 --> G1

Note that the lazy check to mark the rules as valid in the make_update_rule (see https://github.com/clearpathrobotics/ros_comm/blob/kinetic-devel-cpr/tools/rosbag/src/rosbag/migration.py#L1123) method makes that rule pass initially, but later it fails because there's no rule for the sub fields.

This has been adapted from the commit clearpathrobotics@ec8ec37 description.

Improvement to the validation check for the rules

The other commit clearpathrobotics@97fdf48 simply allows to improve the lazy check in make_update_rule and therefore allows to find a better placement/position for the message to be migrated inside of the chain rules. This helps to avoid marking rules as valid, that later would fail because there's no rule for the sub fields. The more precise check that fails later happens at the sub fields level, and there's already a comment on the code that suggest that the way this is handled should be improved (see https://github.com/clearpathrobotics/ros_comm/blob/kinetic-devel-cpr/tools/rosbag/src/rosbag/migration.py#L307).

This replaces clearpathrobotics#10

I recommend installing this chrome add-on to have a better diff: https://chrome.google.com/webstore/detail/github-diff-whitespace/lhbcdehjihmbiafeodkfnbndleijnnhp

FYI @mikepurvis @dirk-thomas @afakihcpr

[mikepurvis]

I've never created or even used bag migration rules, so I haven't a clue what's going on here, despite the effort at explaining the problem and this solution.

Is there some way that the bag migration tests could be modified to illustrate the problem, so that we have a failing test corrected by this patch?

[efernandez]

@mikepurvis I didn't know about those test, although I'm happy to see that something is exercising this code. Upstream (publicly) only two package use rules, and they're only for one generation/change.

I need to look into those tests and add one for the use case they didn't covered. It'd take me some time to get it done.
The good thing is that my change didn't break any of those tests 😃

[efernandez]

@dirk-thomas it seems you wrote most of those tests, could you please orient me on writing a new one that exercise the use case fixed on this PR?

[efernandez]

I've figured out myself what's needed to test this corner case.

Essentially, the MigratedMixed.msg almost covers this use case, except for a subtle thing: it's "version" on the gen3 already appears as the new message on the rule from gen2 to gen3, so the proper rule is created.

Therefore, I need to create a new generation gen4 with no explicit changes on MigratedMixed.msg. That should expose the bug and show how this PR fixes it.

However, I'd need access to upload new .bag files for the new gen4 messages into http://download.ros.org/data/test_rosbag/
@dirk-thomas @mikepurvis Does any of you have access there, or give me access?

For now I'd put the new .bag files in the repo, inside the test folder.

[dirk-thomas]

@efernandez Atm we don't have a way to provide access to other people. I can upload additional files to download.ros.org. Please either send me the bag files by email or post some links where I can download them.

[efernandez]

@dirk-thomas OK, thanks. I'd upload the .bag files somewhere and paste the links here. Once you upload the .bag files I'd update the links on the CMakeLists.txt.

I'm still working on creating the .bag files and setting up the new tests. I hope to finish either EOD today or tomorrow.

[efernandez]

Closing in favour of #1011 (which also depends on #1009 and #1010).