version.proto 'major' and 'minor' fields may turn into C++ code conflicting with system headers

[tho-]

Environment

• OS Version: NetBSD 10
• gz-msgs11 source build

Description

From the version.proto message, protoc generates a C++ class that defines int32_t major() const and int32_t minor() const member methods.

It happens that - at least on NetBSD - major(3) and minor(3) are macros defined in sys/types.h and the protoc generated code does not compile due to the macros exanding in the wrong context. These macros can be disabled from system headers by defining e.g. _POSIX_C_SOURCE at compile time, but defining this might have other unwanted side effects regarding system headers. It is also cumbersome to propagate this option to every single software that needs to include the message definitions.

On the NetBSD side, it will be very hard (not to say impossible) to change/rename these macros (as it was done e.g. on Linux) because they've always been there and the chances to break something somewhere are high.

My current workaround is to patch gz/msgs/convert/FuelMetadata.hh and the generated MessageTypes.hh by adding in the preambule before anything:

#include <sys/types.h>
#undef major
#undef minor

This is a bit fragile, and does not fix any code willing to include the messages directly.

So while noone is to blame really in this case, I was wondering whether it could possibly be envisaged to name those members a bit differently, like major_number or vmajor or anything not conflicting with system headers. I know this will break compatibility, but I was thinking that maybe this message is not widely used (only from fuel_metadata.proto from what I have seen so far) so it might not actually be that bad as a breaking change.

Any thoughts?

[caguero]

Thanks for reporting it, we'll discuss it in the next Gazebo PMC meeting.

We could add a new version message Version2.proto with the renamed parameters in it. Then, replace all Gazebo code using Version with Version2 and that should preserve ABI. If we come up with a good name for the new Version proto file we'll just need to deprecate/remove the original Version.proto in future releases.

If Version2 is just a workaround, we might need to also add extra fields to Version.proto and ending up removing the workaround Version2.proto and the major and minor fields in Version.proto.

We'll create a proposal and paste it here.

[tho-]

Ok, great. Thanks for your comments.

[tho-]

I just came accross this similar issue (old, now closed and for Linux) in protobuf itself :
protocolbuffers/protobuf#4654

I thought it might be useful material for your gazebo meeting as it shows that protobuf itself can't do much for preventing this to happen.