Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

mjcarroll
Copy link
Contributor

Make use of gazebosim/gz-msgs#368

@mjcarroll mjcarroll requested a review from caguero as a code owner July 27, 2023 23:55
@mjcarroll mjcarroll marked this pull request as draft July 27, 2023 23:55
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Jul 27, 2023
@mjcarroll mjcarroll force-pushed the mjcarroll/message_library_generation branch from 1a226ba to b8db3b6 Compare July 28, 2023 21:02
@@ -68,7 +68,7 @@
#include <thread>
#include <vector>

#include <gz/msgs/Utility.hh>
#include <gz/msgs/convert/DiscoveryType.hh>
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor Author

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;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@azeey azeey closed this Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants