Deprecate impl without options

[ijnek]

Deprecate advertiseImpl and subscribeImpl without options.

These changes will warn any method that tries to call

• advertiseImpl or subscribeImpl with four arguments
• advertiseImplWithOptionsor subscribeImplWithOptions

The only issue with this deprecation, is that we can't raise a warning for child classes that override advertiseImplor subscribeImplwith four arguments. Maybe we can get away with adding the changes to the release notes.

There seems to be no image transports currently released outside those in image_transport_plugins, I can raise another PR for image_transport_plugins if this PR gets merged.

[gbiggs]

Since there's no way to warn child classes that override the deprecated functions via the compiler, I think the best solution is to just document that the no-options advertiseImpl/subscribeImpl functions and the advertiseImplWithOptions/subscribeImplWithOptions functions are deprecated and leave it at that, rather than changing the API. We can make sure it goes in the changelog for Iron.

[ijnek]

With your suggestion, the following method would be deprecated but still stay as a pure virtual requiring to be overridden (which inhibits the transition process).

  virtual void advertiseImpl(
    rclcpp::Node * nh, const std::string & base_topic,
    rmw_qos_profile_t custom_qos) = 0;

I believe we should add a default implementation like done in this PR to prevent compilation errors.

  [[deprecated("Use advertiseImpl with four parameters instead by providing "
               "rclcpp::PublisherOptions as fourth argument.")]]
  virtual void advertiseImpl(
    rclcpp::Node * nh, const std::string & base_topic,
    rmw_qos_profile_t custom_qos)
  {
    advertiseImpl(nh, base_topic, custom_qos, rclcpp::PublisherOptions());
  }

Conversely, making the following method pure virtual would cause compilation errors in child classes, but at least we should be able to catch them early in rolling and force the child classes to upgrade before releasing Iron:

  virtual void advertiseImpl(
    rclcpp::Node * node,
    const std::string & base_topic,
    rmw_qos_profile_t custom_qos,
    rclcpp::PublisherOptions options) = 0;

What are your thoughts? @gbiggs

[gbiggs]

I think we should go with the default implementation option. Deprecation shouldn't destroy the API, just warn against using it (either via the compiler or in documentation, or both). Then we can completely remove it in J Turtle.

[ijnek]

Just to clarify, we both agree on adding a default implementation, but what are you suggesting with the following code that exists currently:

  virtual void advertiseImpl(
    rclcpp::Node * node,
    const std::string & base_topic,
    rmw_qos_profile_t custom_qos,
    rclcpp::PublisherOptions options)
  {
    (void) options;
    RCLCPP_ERROR(
      node->get_logger(),
      "PublisherPlugin::advertiseImpl with four arguments has not been overridden");
    this->advertiseImpl(node, base_topic, custom_qos);
  }

All options I can think of have advantages (and disadvantages):

1. converting it to pure virtual (breaks child classes if new method isn't implemented) - done in this PR
2. having an empty implementation (child classes will compile, but may experience unexpected problems)
3. keeping the current code (throws warnings because it calls a deprecated method. More seriously, the problem is the two methods will be indirectly recursive if one of the methods isn't overriden).

Option 1 seems like it ensures the safest transition for child classes.

[gbiggs]

Thanks for clarifying. I agree that options 2 and 3 lead to hard-to-debug and probably broken behaviour, so despite the lack of a deprecation cycle let's do option 1. Let me know when you've made any changes to this PR and I'll review it.

[ijnek]

@gbiggs I don't think any changes are needed, so should be ready for a review.

[gbiggs]

• Linux
• Linux-aarch64
• Windows

[gbiggs]

Thanks for the contribution!

[ivanpauno]

This is breaking behavior downstream (see e.g. https://github.com/ros-perception/image_transport_plugins/pull/106) We should maybe consider breaking API instead of causing only a warning.

[ijnek]

Maybe the name of the PR is incorrect, but we had to intentionally break API for downstream plugins that currently override the method without options. The deprecation notices will show up if you call the method without options. The discussion can be found above.

[ivanpauno]

The deprecation notices will show up if you call the method without options.

The thing is that currently downstream packages only show a warning. But they compile, can be run and they behave incorrectly. If you're going to break behavior, it's better to actually make downstream packages not compile anymore, rather than changing behavior with only a warning. IMO, the worst kind of API break are the ones that don't generate a "fatal error", because it's hard to undestand what's going on.

edit:

My preferred way of "break API" in order:

• With a tick-tock cycle. When the deprecation warning is introduced, the "old way of doing stuff" should not be broken.
• Generate a compile error downstream.
• (never a good option IMO) Break behavior downstream with only a warning.

[ijnek]

But they compile, can be run and they behave incorrectly.

That's not intended, and if that's happening then that'll defnitely be a bug. It should be showing a warning and still behaving correctly.

From https://github.com/ros-perception/image_transport_plugins/pull/106, I don't see why these changes would cause incorrect behaviour...?

[ivanpauno]

That's not intended, and if that's happening then that'll defnitely be a bug. It should be showing a warning and still behaving correctly.

CompressedPublisher::advertiseImpl() (with options) is not being called anymore if you're holding a pointer to base. i.e.:

PublisherPlugin * plugin_;
... // plugin is a CompressedPublisher instance
plugin_->advertiseImpl(node, topic, qos, options);

is calling this

https://github.com/ijnek/image_common/blob/74510a9ba6825dd0ddcbdd5a04ff9d34bad83da7/image_transport/include/image_transport/simple_publisher_plugin.hpp#L101

instead.

That's because the new method hasn't been overriden (only the old one without options), and SimplePublisherPlugin provides an implementation. Because of that, the parameters are not being declared and the plugin is being configured incorrectly.

[ijnek]

Ahhhh... I see the issue, I thought those downstream plugins would fail to compile, but simple_publisher_plugin and simple_subscriber_plugin overrides the methods with the options so they still compile but nothing works... Thanks for explaining @ivanpauno

So then, I'd delete the method without the options completely. Is that what you suggest too?

[ivanpauno]

So then, I'd delete the method without the options completely. Is that what you suggest too?

Yeah, I don't see how this can be deprecated in a nicer way, but if you find an alternative that also would be fine.