Fix Time and Duration raising exception when compared to another type

[suurjaak]

Fixes #952.

Ought to be back-ported to Galactic and Foxy as well.

[fujitatomoya]

Note, original issue detected on Foxy, we need to backport all supported ROS 2 distribution (humble, galactic and foxy)

[suurjaak]

@fujitatomoya @sloretz ping on this?

[clalancette]

Note, original issue detected on Foxy, we need to backport all supported ROS 2 distribution (humble, galactic and foxy)

I don't think we should backport this. This is a behavior change, and I don't think we should do that in the already released distributions. For rolling, this is fine.

[clalancette]

CI:

• Linux
• Linux-aarch64
• Windows

[suurjaak]

This is a behavior change, and I don't think we should do that in the already released distributions.

I would say that this is a bugfix, and as such ought to be ported to distros being supported.

[clalancette]

I would say that this is a bugfix, and as such ought to be ported to distros being supported.

But it is a potentially major behavior change, and has the ability to break things that already work. A good indication that this is the case is the fact that we needed to change tests to deal with this.

This is OK for Rolling (and distros going forward), it is not acceptable for already released distributions. So I am still not in favor of backporting this.

[fujitatomoya]

@suurjaak sorry i was not thinking clearly before, but i am with @clalancette on this. this behavior change might break the ROS 2 user space. I consider that would be much bigger problem.

[suurjaak]

But it is a potentially major behavior change, and has the ability to break things that already work.

You're not wrong in that it may break somebody's code. Very unlikely, though, and would not call it a major change. I'd assume nobody has stumbled over this before, evidenced by the fact that the code has been like this from the very beginning, 4 years and counting, without anybody making noise about the wrongful behavior.

But I think bugfixes are backported, in general? That a bugfix involves behavioral changes, is its very reason for existence :)

I do see your point, though, in that this changes the contract of the Time/Duration API. Although that contract was never declared in documentation.

[fujitatomoya]

IMO, i totally understand your point. this kind of policy should be simple as much as possible, once we have exception that will be hard to keep afterwards. So i am still not positive to backport this.

[suurjaak]

Well, regardless of which version this gets ported to, how about getting this merged?

[fujitatomoya]

@suurjaak thanks for the contribution, the issue has been also closed accordingly.

[clalancette]

@suurjaak @fujitatomoya Would one of you be willing to open a PR to https://github.com/ros2/ros2_documentation/blob/rolling/source/Releases/Release-Iron-Irwini.rst mentioning this change in behavior?

[suurjaak]

@clalancette I can do the release note PR as well.

[clalancette]

@clalancette I can do the release note PR as well.

Thank you, it is much appreciated!