Remove MANUAL_BY_NODE liveliness API

[ivanpauno]

It was agreed in the discussion here, that we were going to remove MANUAL_BY_NODE related liveliness API.

It would be great to actually do it before Foxy release, as it doesn't have more sense after the Participant/Context change.

[ivanpauno]

@wjwwood @jacobperron can you review this and the connected PRs?

I will then add a note to Foxy release notes, and update tutorials/design docs.

[rotu]

This is not yet deprecated. Shouldn't we be deprecating instead?

https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#deprecation-strategy
https://semver.org/#how-should-i-handle-deprecating-functionality

[ivanpauno]

This is not yet deprecated. Shouldn't we be deprecating instead?

https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#deprecation-strategy
https://semver.org/#how-should-i-handle-deprecating-functionality

No, I think it's not going to be deprecated, but directly removed.

The reasons are two:

• It isn't working as it was intended to work.
• It's rarely used, so it won't affect much users.

[wjwwood]

I'll defer to @ivanpauno on this, as he has the most context, but my understanding is that the move to one participant per context makes this feature impossible to have working in a way that makes sense. So, it would not satisfy the first conditional of the linked policy, "where possible". But maybe my interpretation is wrong.

[wjwwood]

@ivanpauno this is way past the API freeze date though, I think we need approval from several people to make this change at this point...

[wjwwood]

lgtm, though changing the API at this point should be reviewed and acknowledged by several maintainers and should be announced separately, imo.

[rotu]

I'll defer to @ivanpauno on this, as he has the most context, but my understanding is that the move to one participant per context makes this feature impossible to have working in a way that makes sense. So, it would not satisfy the first conditional of the linked policy, "where possible". But maybe my interpretation is wrong.

That seems to be a reason for deprecating it! Deprecate it and stub out the implementations if necessary. But don't introduce binary incompatibility just for the heck of it.

This is not yet deprecated. Shouldn't we be deprecating instead?
https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#deprecation-strategy
https://semver.org/#how-should-i-handle-deprecating-functionality

No, I think it's not going to be deprecated, but directly removed.

The reasons are two:

• It isn't working as it was intended to work.

That's a great reason to deprecate and gently redirect users.

• It's rarely used, so it won't affect much users.

That's not even a consideration of either our developer guidelines nor SemVer

.I looked into this because I got pushback from @clalancette and @sloretz on a completely unused API function ros2/kdl_parser#7 (comment), who both felt it was incorrect to remove a function without first deprecating. We should enforce the policy or update the documented expectations.

[ivanpauno]

@rotu The difference with ros2/kdl_parser#7 (comment), is that you can still use the deprecated API, and it will do what's supposed to do.
That's not the case here.

There will be a note in the release notes, so people will have enough information to understand the issue.

@ros2/team for approval

[wjwwood]

It isn't working as it was intended to work.
That's a great reason to deprecate and gently redirect users.

Well I can't speak for everyone on our team, that's part of why I said we should include more reviewers, but I see no value in providing an API where the most reliable thing to do is throw "intentionally not implemented". I think it's better to remove it. It's not a "this other version is better" or "we wanted to change the name", it's a "this no longer even works and we can't replicate the old behavior".

[jacobperron]

I agree that removing the API is better than people using a deprecated API that has broken behavior. I think many will assume that a deprecated API will still work.

[rotu]

I think many will assume that a deprecated API will still work.

That's what the message is for - informing the user of why an API is removed and how code should evolve.

C++: [[deprecated("Broken. Please use XXX instead.")]].
C: RCUTILS_DEPRECATED_WITH_MSG("Broken. Please use XXX instead.")

Removing a function is disruptive. It breaks the build and blocks direct testing of different versions of the same package. e.g. we couldn't test yesterday's CycloneDDS with tomorrow's rmw_implementation to compare whether a certain commit on a master branch broke the nightly.

Also, this redefines the existing numeric values of enum rmw_qos_liveliness_policy_t, another breaking change.

[dirk-thomas]

👍 to remove now.

[rotu]

Please deprecate now and remove in a future version.

[wjwwood]

If the main concern is a message for the user, I'd be fine with having a stub in rmw which is deprecated, but continue with removing the implementations. The result will be a deprecation warning when using it, and a linker error later since the symbol is not provided.

I do not agree with providing a stub that just asserts or throws at runtime. I think that's just not useful for users.

[wjwwood]

Removing a function is disruptive. It breaks the build and blocks direct testing of different versions of the same package. e.g. we couldn't test yesterday's CycloneDDS with tomorrow's rmw_implementation to compare whether a certain commit on a master branch broke the nightly.

You could still do this test, you would just have to include the commit that removes these functions. Btw, if cyclone still implements these functions, but they are removed from rmw there will be no build failure...

The point about the enum's is a good one though, I agree with the suggestion to explicitly set them to avoid changes in the number for now.

[ivanpauno]

Is that different than the deprecation warning with no implementation? I guess it's always a compiler error and not a warning? I'm fine with either.

Ah, yeah you're right. I will do that.

[ivanpauno]

• Linux
• Linux-aarch64
• macOS
• Windows

[ivanpauno]

@wjwwood I think this is ready.
I've made changes in:

• Remove MANUAL_BY_NODE liveliness API #227
• Remove MANUAL_BY_NODE liveliness API rmw_cyclonedds#178
• Remove MANUAL_BY_NODE liveliness API rclpy#556

[rotu]

I think it's bad practice

Why?

are not backwards compatible and would necessitate a MAJOR version bump, not a minor one.

Why is that an issue?

In the end, it just comes down to a personal judgement. I think API compatibility and stability are attractive software features; and that major releases of a package should happen when necessary due to package needs, and not just because an upstream package wants to tie up loose ends. Contrary to a long history of the "ROS way" of big releases spanning many repositories with long periods of instability between releases, I think it's important to decouple development of different, individually versioned and repositoried codebases.

I think you have it backwards - you want to delete the declaration before the implementation.

If we were deprecating functionality we wanted to remove in the future, I'd agree. However, the difference here is that the functionality is gone now, no other option. There is no way to preserve the existing behavior, and that point is the reason why we cannot do what you laid out, not because we just don't want the take the steps. There's no opportunity to delete the declaration after the definition, we can either delete both now (original proposal for this pr) or delete the (no longer working) definition and leave the declaration only as a way to inform users why it is no longer there and then remove it later.
If you want a version with the function available, use the current version, if you want to move forward you have to stop using the function immediately, there is no in between where you get to keep using the function and just have a warning.

Sorry. I mean the function declaration before the function definition. You're right that the functionality is already gone. I think API stability and build compatibility are still worth preserving, even if that means gutting a few function implementations to no-ops for a release.

At any rate, I'm personally done discussing this in circles. My preference is to remove the definition now, leave something behind for users to run into (deprecated declaration or error macro), and do one major version bump release, and I don't think I will be persuaded otherwise. Others can weigh in if they want. Also, we can take what ever steps possible to avoid unnecessary breaks downstream, like the enum fix.

I agree. I think everything has been said and we've both laid out our cases fully. I'm sympathetic to your take. It's very sensible and well-thought out, and nobody want to release broken or ugly code.

[jacobperron]

On the topic of bumping the version, I don't think it necessarily has to be a major bump as we have not release a 1.0.0 yet. I think a minor bump is also acceptable.

From https://semver.org/:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

[wjwwood]

@jacobperron sure, but the plan is to move this to 1.0.0 for foxy anyways, since as semver recommends:

How do I know when to release 1.0.0?

If your software is being used in production, it should probably already be 1.0.0. If you have a stable API on which users have come to depend, you should be 1.0.0. If you’re worrying a lot about backwards compatibility, you should probably already be 1.0.0.

I think some or all of those things are true for this package now.

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-foxy-fitzroy-call-for-testing-and-package-releases/13998/1

[ivanpauno]

New CI round:

• Linux
• Linux-aarch64
• macOS
• Windows

[ivanpauno]

Build up to rmw_implementation, test rmw (to test that linters now pass, and enum value deprecation is fine):

• Linux
• Linux-aarch64
• Windows

MacOS failed randomly last few times, another try:

• macOS

---

Retriggered Windows after failures:

• Windows

[ivanpauno]

Build up to rmw_implementation test rmw, after updated style in deprecation code:

• Linux
• Linux-aarch64
• Windows

Another full macos, because IDK what happened in last one:

• macOS (unrelated failures)