rmw_get_topic_endpoint_info doesn't exist on Dashing #91
Conversation
Signed-off-by: Erik Boasson <eb@ilities.com>
|
I was also facing the same issue while building . Now its resolved, thanks @eboasson . |
| @@ -12,6 +12,10 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // limitations under the License. | |||
|
|
|||
| #include "rmw_cyclonedds_cpp/rmw_version_test.hpp" | |||
|
|
|||
| #if RMW_VERSION_GTE(0, 8, 1) | |||
There was a problem hiding this comment.
The missing functions weren't part of release 0.8.1, so 0.8.1 shouldn't be included. The problem is that we usually bump the version when releasing and not when developing.
I see that Dashing is using master branch (1), while eloquent is using it's own branch (2). Why the incosistence?
If there weren't such inconsistency, including the new functions in 0.8.1 would be a problem.
There was a problem hiding this comment.
Ok, I saw this comment about development versions ros2/rmw#188 (comment), and also this:
https://github.com/ros2/rmw/blob/a3774531e9b7289574501275aafbc3f93120360e/rmw/CMakeLists.txt#L66.
I'm not pretty sure about how it should be used, but I think that should be considered in this PR.
There was a problem hiding this comment.
@ivanpauno, thanks for looking into it. The 0.8.1 comes straight from the Foxy build settings. I guess that one should have been bumped then.
Unless I am mistaken, I'm the one who asked for the rmw versioning stuff to work nicely so that I could have a single source for multiple versions without having to deal with backporting things. That used to be perfectly viable, but I have a feeling ros2/design#250 is going to happen, and that will be too big a change to handle with a bit of conditional compilation. Therefore, it seems multiple branches are likely to become unavoidable.
Since it turns out a branch had already been created for eloquent, I guess we now should retroactively give dashing a branch as well. Then this PR can simply be closed.
There was a problem hiding this comment.
@ivanpauno, thanks for looking into it. The 0.8.1 comes straight from the Foxy build settings. I guess that one should have been bumped then.
mmm, it wasn't bumped. The eloquent version of rmw is indicating 0.8.1: https://github.com/ros2/rmw/blob/eloquent/rmw/package.xml.
I'm unsure how the versioning was supposed to be used.
@wjwwood @dirk-thomas can you clarify?
Unless I am mistaken, I'm the one who asked for the rmw versioning stuff to work nicely so that I could have a single source for multiple versions without having to deal with backporting things
Yes, that's true. I agree that mantaining similar versions from the same source is desired.
Since it turns out a branch had already been created for eloquent, I guess we now should retroactively give dashing a branch as well. Then this PR can simply be closed.
I guess that probably was an error, if it's easier to mantain eloquent and dashing from the same branch it would be reasonable to merge them again.
That used to be perfectly viable, but I have a feeling ros2/design#250 is going to happen, and that will be too big a change to handle with a bit of conditional compilation. Therefore, it seems multiple branches are likely to become unavoidable.
Yeah, I agree that after that change it will be easier to branch that to mantain all the versions from the same branch.
There was a problem hiding this comment.
I'm unsure how the versioning was supposed to be used.
Whenever the API is broken in rmw it should declare that by bumping the dev version to the next upcoming version number: e.g. ros2/rmw@688e53a
There was a problem hiding this comment.
Yeah, we should have updated it when merging that. I forgot to do so. It's tricky to remember to do that when pull requests span releases, but we'll just have to be more diligent about it.
There was a problem hiding this comment.
I'm working on a pull request to fix it.
|
@eboasson What is the status on this? It would be nice to get the Dashing builds passing again. |
|
In my understanding it requires me (or someone) to change the |
|
While I understand the problem of having little time for many tasks I just want to point out that building Dashing from source using CycloneDDS has been broken since the other PR was merged 16 days ago. That does look bad for ADLINK as well as ROS 2 as a whole. So I would think it is in ADLINKs interest to address such a major regressions earlier than later. (More than two weeks sounds already way to long to me.) |
Signed-off-by: Erik Boasson <eb@ilities.com>
* rmw_get_topic_endpoint_info doesn't exist on Dashing Signed-off-by: Erik Boasson <eb@ilities.com> * get_topic_endpoint_info got added in RMW 0.8.2 Signed-off-by: Erik Boasson <eb@ilities.com>
... and this PR avoids trying to compile them on Dashing, similarly to how other RMW interface changes have been dealt with in the Cyclone DDS RMW layer so far.
Signed-off-by: Erik Boasson eb@ilities.com