-
Notifications
You must be signed in to change notification settings - Fork 412
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
[rclcpp_action] removed rosidl_generator_c dependency #992
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.
lgtm, with CI.
I'd like @jacobperron to have a look too, just in case we're missing something he sees.
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 (after DCO bot is happy)
The generator packages shouldn't be necessary unless were actually generating messages. These dependencies are probably vestigial from an early iteration.
…endencies Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
f52a5f0
to
1e852aa
Compare
should I run the CI? |
Please check the sources of the package. Again, as in other similar PRs, the package uses symbols from the proposed-to-be-removed dependency:
|
Yes, I would expect CI to catch things like Dirk pointed out. |
Not necessarily if other dependencies bring the needed dependency in transitively. |
Signed-off-by: Alejandro Hernández <ahcorde@gmail.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.
Please update the PR title accordingly.
"rosidl_generator_c" | ||
"rosidl_generator_cpp") | ||
"rclcpp" |
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.
Nitpick: revert order change.
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
Unrelated failures. Merging it |
After discussing yesterday with @wjwwood we think that these dependencies are not needed anymore here.
I compiled from sources master and I executed the action examples and it's working