name conflict in files generated from rpc.proto

[MeatBoy106]

The field minor and major of the criu_version message conflict with macros defined in system header giving the following error :

***/rpc.pb.h:2920:13: warning: In the GNU C Library, "major" is defined
by <sys/sysmacros.h>. For historical compatibility, it is
currently defined by <sys/types.h> as well, but we plan to
remove this soon. To use "major", include <sys/sysmacros.h>
directly. If you did not intend to use a system-defined macro
"major", you should undefine it after including <sys/types.h>.
::google::protobuf::int32 major() const;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Undefining these macros before including the file is not good enough to me ;)

[adrianreber]

Could you provide some more details about what you are trying to do? Maybe also the steps you took to get this error.

[MeatBoy106]

To reproduce the issue:

1. Use ubuntu 18.04
2. Use gcc 7.3 (the default one on ubuntu 18.04)
3. Generate the C++ sources from rpc.proto using protoc (version >= 3.0.0)
4. Compile rpc.pb.cc

This will give you a bunch of the error message cited above.
This is caused by the fields major and minor of the criu_version message which share their names with macros defined by sys/sysmacros.h.

[adrianreber]

Thanks, this has been reported in other projects already:

protocolbuffers/protobuf#4654
google/gapid#1995

It seems others have just renamed those fields. Not sure we can do that easily as it would break RPC clients.

[adrianreber]

Thinking more about it, this should actually be easy to solve as the names are not really important in protobuf, as long as the ID of those properties does not change it should be easy to change it.

For external RPC users we change the protobuf definitions and the corresponding code using it whenever it is necessary, but the communication between CRIU and its RPC clients should always keep working.

[MeatBoy106]

I'm not a protobuf expert but this sounds reasonable to me. I've personally opted for the "#undef {minor,major}" workaround as it suits my needs. Le jeu. 21 févr. 2019 à 17:42, Adrian Reber <notifications@github.com> a écrit :

…

Thinking more about it, this should actually be easy to solve as the names are not really important in protobuf, as long as the ID of those properties does not change it should be easy to change it. For external RPC users we change the protobuf definitions and the corresponding code using it whenever it is necessary, but the communication between CRIU and its RPC clients should always keep working. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#625 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AO9skfOx1fLlDva1tl5_9RJtouzp8AoRks5vPsxhgaJpZM4bFc9_> .

-- Gabriel Busnot

[adrianreber]

This changes will require code changes to everyone using our protobuf RPC definition. It will not break running programs, even if CRIU is updated, but compilation will fail. So kind of an API break without breaking ABI... (I hope I understand protobuf correctly).

[MeatBoy106]

It my actually be even better than that. If I undestand protobuff, the field names exist only in the .proto file and the .cc and .h files generated from it. Everything after that point should be field names agnostic and relie on field ids only. As a result, as long as the API user doesn't regenerate the .cc and .h files from the updated .proto file, his code will compile just as before and run as well. Also, I don't understand how such usual words as minor and major can end up as macros in a fundamental system header. Le jeu. 21 févr. 2019 à 17:57, Adrian Reber <notifications@github.com> a écrit :

…

This changes will require code changes to everyone using our protobuf RPC definition. It will not break running programs, even if CRIU is updated, but compilation will fail. So kind of an API break without breaking ABI... (I hope I understand protobuf correctly). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#625 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AO9skUxrICObMhiWBCVEgkRoBWPzrekaks5vPs_ngaJpZM4bFc9_> .

-- Gabriel Busnot

[adrianreber]

Yes, that is also how I understand it. I will prepare a patch and hopefully this will be part of the next release if no one sees any other problems with this approach.

[avagin]

Should we close this issue?

[MeatBoy106]

Done, thanks for your work !