-
Notifications
You must be signed in to change notification settings - Fork 43
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
Allow transport to generate messages #425
Conversation
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
1a226ba
to
b8db3b6
Compare
@@ -68,7 +68,7 @@ | |||
#include <thread> | |||
#include <vector> | |||
|
|||
#include <gz/msgs/Utility.hh> | |||
#include <gz/msgs/convert/DiscoveryType.hh> |
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.
Is the plan to leave the conversion functions in gz-msgs even for protos that have been moved elsewhere?
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 wasn't particularly clear on that. The conversion functions are in the gz::msgs namespace, so putting them in other packages could end up being a little awkward?
) | ||
|
||
gz_msgs_generate_messages( | ||
TARGET gz-transport${PROJECT_VERSION_MAJOR}-msgs |
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.
We discussed making the generated library a component so it can be used by third-party applications without having to link against the containing library. Is that still possible?
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.
You should be able to use the msgs component without using the core library for each package that has it.
*/ | ||
|
||
syntax = "proto3"; | ||
package gz.msgs; |
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.
Okay, so we're keeping the package name gz.msgs
. I think it's okay, but would be a little confusing for future users.
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.
It's a little confusing, but also limits the number of changes that need to be made. I think if we want, we can then alter the package names in a later revision, but it makes the diff quite large here.
Make use of gazebosim/gz-msgs#368