protocolbuffers / protobuf

Protocol Buffers - Google's data interchange format
http://protobuf.dev
Other
65.69k stars 15.5k forks source link

It's not possible to have fields name `minor`, `major` #4654

Closed graywolf closed 6 years ago

graywolf commented 6 years ago
 error: 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>. [-Werror]
   ::google::protobuf::uint32 major() const;
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                    

If protobuf does not use major, minor by itself, it should probaby be undefed. Or at least provide a way to do so.

arthur-tacca commented 6 years ago

Relevant: #3203

graywolf commented 6 years ago

maybe we could have ability to prepend the generated code with our own, maybe something like:

syntax = "proto3";

before_messages(c++) {
    // valid c and/or c++ code could go here and it will
    // be included before generated code (but after includes)
    // in c++ output
    //
    // in this case:
    #undef major
    #undef minor
}

message Foo {
    uint32 bar = 1;
}
acozzette commented 6 years ago

This problem is tricky to solve, because within Google we have code that actually uses the major and minor macros after protobuf headers have been included, so we can't easily undefine those without breaking code. In your case is it possible to just undefine those macros before you include your generated pb.h file?

graywolf commented 6 years ago

Am not even including the pb.h in my project yet (didn't get that far). This happens during compilation of pb.cc.

/home/wolf/devel/jibril/build/jibril-lib/general.pb.cc:392:13: error: In the GNU C Library, "minor" 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 "minor", include <sys/sysmacros.h>
 directly. If you did not intend to use a system-defined macro
 "minor", you should undefine it after including <sys/types.h>. [-Werror]
     set_minor(from.minor());

I guess I could include sed step into my build process but that seems more like a hack then a solution. But the before_messages(c++) way should work without breaking code for anyone no?

xfxyjwf commented 6 years ago

@graywolf Such conflicts are very annoying and because system/library headers can pretty much define whatever macro they like, there exist an infinite amount of such conflicts (i.e., you can find an infinite list of names that can't be used in protobuf). This is not a problem that we can fix. If you can't use a certain name in protobuf due to this reason, please try to rename it first. We generally only add special-cases in protocol compiler as the last resort when renaming is impractical. In this particular case of "major"/"minor", as mentioned by Adam already, we can't even undef them for regular protos because it breaks people. And I would recommend against using sed to change the generated code yourself. That's a long term maintenance burden (it complicates your build setup and breaks whenever protobuf changes) and likely will cost you much more than renaming the two offending fields.

jamescooper-blis commented 4 years ago

Sorry to comment on a seemingly stale issue, but we are upgrading to Ubuntu 18.04 and are running into this issue. The real problem is that the protobuf file we're compiling is supplied by Google itself!

https://developers.google.com/authorized-buyers/rtb/downloads/realtime-bidding-proto.txt

I manually built and installed protobuf compiler version 3.11.2 but it didn't help. Do we have to manually change Google's realtime-bidding.proto file ourselves each time it's updated?

JamesOldfield commented 4 years ago

At the very least, the generated code could do:

#pragma push_macro("minor")
#undef minor

// ...

#pragma pop_macro("minor")

(This would obviously need to be behind a check that it is a platform where this is needed, the push_macro/pop_macro pragmas are supported, and that the macros were actually defined.) This would by no means solve the problem, but it would mean that at least the generated code would compile without having to interfere with it as an extra build step.

peterebden commented 4 years ago

I've just run into this (with messages I don't control so renaming fields is not an option) and agree with the above; it would be really nice to have something like graywolf's solution to add an arbitrary preamble to the generated code.

nimaim commented 4 years ago

Any update to this? We define major/minor/patch fields for our own custom version in our proto file and cannot change this right now due to preserving backwards compatibility. All these warnings are very annoying to look at when there are other critical warnings to fix.

acozzette commented 4 years ago

@nimaim This has already been fixed, you might just need to upgrade to a recent version.

nimaim commented 4 years ago

Already upgraded to the latest in Mint using the official instructions (3.11.4). Have you tried the use case with syntax=proto2 and major/minor fields in your proto file? It still produces the same multiple major/minor warnings for me as the OP.

EDIT: Cross compiled in another environment w/ GCC 9.x, protobuf 3.11.4, and Qt 5.12.X and seems to be okay. My host may still have remnants of old protobuf libs. Disregard above comment.

ozaydinoz commented 3 years ago

Unfortunatelly still same issue here with version 3.14.0 I have ubuntu 18.04 using gcc 7.5. I'm using syntax=proto2