-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove MANUAL_BY_NODE liveliness API #227
Conversation
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@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. |
This is not yet deprecated. Shouldn't we be deprecating instead? https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#deprecation-strategy |
No, I think it's not going to be deprecated, but directly removed. The reasons are two:
|
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. |
@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... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, though changing the API at this point should be reviewed and acknowledged by several maintainers and should be announced separately, imo.
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.
That's a great reason to deprecate and gently redirect 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. |
@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. There will be a note in the release notes, so people will have enough information to understand the issue. @ros2/team for approval |
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". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
That's what the message is for - informing the user of why an API is removed and how code should evolve. C++: 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 Also, this redefines the existing numeric values of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to remove now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please deprecate now and remove in a future version.
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. |
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. |
Ah, yeah you're right. I will do that. |
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@wjwwood I think this is ready. |
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.
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.
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. |
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/:
|
@jacobperron sure, but the plan is to move this to 1.0.0 for foxy anyways, since as semver recommends:
I think some or all of those things are true for this package now. |
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 |
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
…g it Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
(upstream api break from ros2/rmw#227) Signed-off-by: Ted Kern <ted.kern@canonical.com>
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.