-
Notifications
You must be signed in to change notification settings - Fork 90
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
API changes to sync with one Participant per Context change in rmw_fastrtps #106
Conversation
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.
Looks reasonable to me, but I'd like @eboasson to chime in too. Particularly, when we can expect them to implement recent node-to-participant mapping changes.
It certainly looks perfectly reasonable to me. The answer to @hidmic's question is a bit more difficult: this involves a few rather large pull requests and we need to first figure out what changes are actually needed. The change to using a single participant is trivial, it is the bit that relates to introspection that is less obvious. Currently, the Cyclone RMW layer just queries its discovery data on demand and otherwise ignores all changes save for triggering the graph guard condition. I guess it will now have to call functions in So the only thing I can say definitively is that it we'll be looking into the details and hope to be able to this soon. |
Yes, completly agreed.
Please, let me know if I can help, or if anything has to be modified in
Great, thanks! |
118119f
to
ad48796
Compare
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 left some comments. This is a bit larger than I recall. @eboasson mind to take a look too?
rmw_cyclonedds_cpp/src/rmw_node.cpp
Outdated
goto fail_alloc; | ||
} | ||
size_t i; | ||
i = 0; |
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.
@ivanpauno nit: last two lines can be collapsed into one.
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.
Same as above, it comes from rmw_get_node_names
.
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.
There is a pretty fun reason: when done in the same line, the compiler fails saying that the above goto
is skipping an initialization. In general, it's safe to skip the initialization of a trivially destructible object (AFAIU), but the compiler shows the warning anyways (and we transform that warning in an error, because of our compiler flags).
When split in two lines, the compiler doesn't show a warning.
P.S.: This kind of warning never happens in pure C code, it only applies when you use C++.
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.
Hmm interesting, odd but interesting.
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 define ‘i’ higher in the scope to avoid this instead. This what is done in all the other functions that use goto.
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 think @wjwwood comment still applies.
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.
The code is pre-existent, and I don't want to retrigger CI because of this. If you don't mind, I will ignore the comment.
0dc9edd
to
429cd7e
Compare
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 think rmw_get_nodes_names*
API implementation is improving, but it needs some adjustment.
{ | ||
if (RMW_RET_OK != rmw_check_zero_rmw_string_array(security_contexts)) { | ||
return RMW_RET_ERROR; | ||
} |
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.
@ivanpauno is it invalid to call rmw_get_node_names_with_security_contexts()
with a null security_contexts
? If not, I think this check should be relocated to the implementation function and conditionally executed based on security_contexts
value.
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.
In rmw_get_node_names_impl
security_contexts
can be NULL
.
In rmw_get_node_names_with_security_contexts
security_contexts
cannot be NULL
.
I think the current logic is fine.
I closed and reopened to trigger the new checks. |
@rotu the check called Also, it's impossible that the checks using I'm not completely sure how to solve the problem, but I think we should only have actions for checking compatibility with past releases, and just rely on CI for master (which should be run manually by the user when the PR is ready). P.S.: Having github actions for this is great, thanks for adding them! |
@ivanpauno I'm aware of the issue and it's due to my mistake in setting this up. Should be fixed by #133. Yes, the checks ONLY check changes in this repo - the goal is to prevent accidental breakage. A change like this PR, involving a constellation of different commits across repos that do not move in lock-step, does break things and presents a versioning challenge. Assuming that it would be too burdensome to preserve compatibility, the answer is to merge it with these checks failing. |
FYI: here's me blundering through setting up CI. Using "source" was a mistake. ros-tooling/action-ros-ci#120 |
That sounds good to me. I know that the actions using Most of the PRs that have broken To avoid ignoring the result of the checks in those cases, I think we should either:
|
Agreed. That should be the behavior since #133 (the setting
Manually run CI is still part of the picture. The actions in this repo only check things up to |
e09008b
to
deb1957
Compare
To rerun this against latest CI workflow, I think you need to rebase onto master. I think if you fork this repo on Github, you can simply enable CI in the Actions tab of your fork and it will run on every push. |
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>
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>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
…curity_contexts Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
deb1957
to
cc48725
Compare
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
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
rmw_cyclonedds_cpp/src/rmw_node.cpp
Outdated
goto fail_alloc; | ||
} | ||
size_t i; | ||
i = 0; |
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 think @wjwwood comment still applies.
Note for reviewers: All checks are passing, except the ones using |
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno I've finally carved out the time to work on this, but before really commencing I wanted to make sure there won't be any duplication of work, which there might be if you happen to also be looking into it. (If you are, I'm totally happy to leave you to it, by the way 😁) Also looking at the scope of the changes, and as I have mentioned before, I think this is the point where we should stop providing compatibility with dashing and eloquent in rmw_cyclonedds master. Hopefully we'll be able to merge #123 into master first: that would then be a single version that supports all features and works with Dashing, Eloquent and Foxy, and so would also be valuable for anyone who needs to integrate multiple versions of ROS2 in a single system. It'll take me some time to get all the changes done and tested locally, so I'd say there's plenty of time for updating #123 to match the current state of master and merging it. |
Sounds good to me.
100% agreed, creating a new branch makes sense.
Sounds great to me! If you need any reviews or if you have any questions, let me know, and I will be willing to help. |
Needed since ros2/rmw#189.