Change message generation workflow

[mjcarroll]

This changes the way that we generate headers a bit.

Rather than injecting code directly into the protobuf headers, this instead causes the protobuf headers to be generated in ignition/msgs/detail/msg.pb.h.

We then generate our own ignition/msgs/msg.pb.h header with whatever content we want that completely wraps the protobuf header.

Example of a new header:

// Generated by the protocol buffer compiler.  DO NOT EDIT!
// source: ignition/msgs/actor.proto

#ifndef ignition_msgs_actor
#define ignition_msgs_actor

#include <memory>

#include <ignition/msgs/Export.hh>

#ifdef _MSC_VER
#pragma warning(push)
#pragma warning(disable: 4100 4512 4127 4068 4244 4267 4251 4146)
#endif

#include <ignition/msgs/detail/actor.pb.h>

namespace ignition {
namespace msgs {
typedef std::unique_ptr<Actor> ActorUniquePtr;
typedef std::unique_ptr<const Actor> ConstActorUniquePtr;
typedef std::shared_ptr<Actor> ActorSharedPtr;
typedef std::shared_ptr<const Actor> ConstActorSharedPtr;
}  // namespace msgs
}  // namespace ignition
#ifdef _MSC_VER
#pragma warning(pop)
#endif

#endif  // ignition_msgs_actor

[codecov]

Codecov Report

Merging #256 (fe737e8) into main (d63e557) will increase coverage by 70.91%.
The diff coverage is 100.00%.

❗ Current head fe737e8 differs from pull request most recent head 0591423. Consider uploading reports for the commit 0591423 to get more accurate results

@@             Coverage Diff             @@
##             main     #256       +/-   ##
===========================================
+ Coverage   14.53%   85.44%   +70.91%     
===========================================
  Files         200       10      -190     
  Lines       12505      962    -11543     
===========================================
- Hits         1817      822      -995     
+ Misses      10688      140    -10548     

Impacted Files	Coverage Δ
src/Generator.hh	100.00% <ø> (ø)
src/Utility.cc	82.28% <ø> (ø)
src/Generator.cc	92.75% <100.00%> (+0.21%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32b734e...0591423. Read the comment docs.

[caguero]

This approach looks cleaner to me. Good catch removing the headers.

I think we need to do two small changes in test/Factory_TEST.cc and test/ign_TEST.cc and replace the #include "gz/msgs/test_config.h with #include "test_config.h. Otherwise we'd need to make other Ignition<->gz changes in the project if I'm not wrong as test_config is installed under ignition and not gz.

[Blast545]

👨‍🌾 Quick question, is this meant (or could be) to be backported to Citadel? @mjcarroll

[mjcarroll]

Quick question, is this meant (or could be) to be backported to Citadel? @mjcarroll

I targeted Garden out of maximal ease. I would be willing to put in the effort for Fortress/Citadel, but I don't have a good feeling of if this violates ABI/API?

The biggest divergence is that we are introducing an entirely new folder of headers that have the original protobuf generated code and replacing our previously-generated headers with smaller versions.

What do you think @chapulina ?

[nuclearsandwich]

@Blast545 and I reviewed this on a call together. We spent most of the time talking through the added python script with a few notes there and then reviewing the script's usage in CMake above.

I left a few comments in line. As a Gazebo / protobuf novice, I don't quite follow the plumbline from the detail file, to the replacement protobuf header, to the ignition/gazebo header. I think that could be clearer, either as commentary or function decomposition in the python script or as PR commentary.

[nuclearsandwich]

We (@Blast545 and I) originally scheduled time to look at this before the discussion in yesterday's simulation meeting to review it for ABI/API compatibility. Since this is, at the end of the day, shuffling header files around but ultimately seems to maintain the overall translation unit structure of the headers. It does not appear to be an ABI/API break but is a project organization and structure break. It also adds a build-time dependency on a python3 interpreter which may or may not be a new build time dependency and therefore a "breaking change" in that regard. In short, I couldn't say definitively whether or not this is backportable. There seem to be a lot of "maybe not"s but nothing damning.

[nuclearsandwich]

It also adds a build-time dependency on a python3 interpreter which may or may not be a new build time dependency

In further discussion with @Blast545 we talk about the fact that the script which is currently in Python could be written in pure CMake if needed. But the Python3 script will likely be easier to evolve over time. So if introducing the python3 interpreter dependency is the deal breaker, then the logic could be ported over to CMake at the cost of some humanity.

[mjcarroll]

As a Gazebo / protobuf novice, I don't quite follow the plumbline from the detail file, to the replacement protobuf header, to the ignition/gazebo header. I think that could be clearer, either as commentary or function decomposition in the python script or as PR commentary.

I'll improve the documentation on this. There are some gymnastics going on that were much, much easier to write in python due to the limitations of the protobuf generator API as well as CMake, in general.

[nuclearsandwich]

much, much easier to write in python due to the limitations of the protobuf generator API as well as CMake, in general.

Yeah, I was not necessarily advocating that this should be rewritten in CMake, just noting the possibility since the primary activity seems to be filename / pathname transformations after executing protoc. If this was meant to be a one-and-done thing where we write it once and then don't expect to iterate or evolve it that would strengthen my desire to see it done in CMake once it was settled to drop the build-time python3 dependency but we'd want to be extremely confident that this wouldn't evolve as it will be much easier to work on in Python.

[mjcarroll]

If this was meant to be a one-and-done thing where we write it once and then don't expect to iterate or evolve it that would strengthen my desire to see it done in CMake once it was settled to drop the build-time python3 dependency but we'd want to be extremely confident that this wouldn't evolve as it will be much easier to work on in Python.

I anticipate that this will likely evolve, potentially in light of discussions around data representations here: gazebosim/gz-sim#494

[mjcarroll]

This is ready for review.

[ahcorde]

In general review ignition or ign to gz

[Blast545]

We were expecting this PR to remove the warnings generated by protobuf all over the stack, right?

I ran a job on gz-transport to check and the warnings are still there:
https://build.osrfoundation.org/job/ign_transport-ci-win/36/

Just checking, maybe an extra release of gz-msgs is required?

[Blast545]

FYI: @Crola1702

[mjcarroll]

That was the expectation, and it looks like that built against the newest version. I will take a look, maybe something is misconfigured.