-
Notifications
You must be signed in to change notification settings - Fork 315
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
Switching to examples_interfaces #377
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: CursedRock17 <[email protected]>
Signed-off-by: CursedRock17 <[email protected]>
Signed-off-by: CursedRock17 <[email protected]>
Signed-off-by: CursedRock17 <[email protected]>
Hmm looks like a regression with one test, do you think it needed to be patched in #374? I can't imagine changing to |
@CursedRock17 #374 is already included in your repo https://github.com/CursedRock17/examples/tree/deprecated_msgs? what do you mean by #377 (comment)? |
Yeah, I just didn't know if that specific test had passed all the way through, I'm trying to figure out why that specific test continues to be a regression. I guess it did pass in #374's CI so my changes must have broke it somehow. |
@@ -14,14 +14,14 @@ | |||
|
|||
import unittest | |||
|
|||
from example_interfaces.msg import String |
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 believe this changes led to https://ci.ros2.org/job/ci_linux/20965/testReport/junit/launch_testing_examples.launch_testing_examples/check_msgs_launch_test/launch_testing_examples_check_msgs_launch_test/.
this is because message type does not match anymore,
demo_nodes_cpp
uses std_msgs/String
, so i think this change requires the same change to https://github.com/ros2/demos
Ok I understand it's the changes I've made that cause downstream CI failures, but I just don't see the correlation between this repo and something like rmw_implementation which is currently failing. Since neither repo includes each other I assumed it could've been |
To be perfectly honest, I'm not entirely sure we should do this at all. I know we technically marked those messages as deprecated, but they are very heavily used; looking at the packages released into rolling, I see hundreds of uses of those messages. It just seems that they are too useful to be able to remove them. Personally, I would actually be for reverting ros2/common_interfaces#116 , and closing this PR and the associated issue. But we'll have to discuss that with the rest of the team. |
Meant to resolve #356, this pull request switches all the deprecated instances of
std_msgs
toexample_interfaces
after deprecation of the many elements occured. Though, any msg withHeader
is still astd_msgs
since they were not part of that deprecation.