Skip to content

Conversation

@rbino
Copy link

@rbino rbino commented Jul 3, 2020

gpb changed the occurence field from optional to defaulty before version
4.13 (see
tomas-abrahamsson/gpb@a363325).
This made Exprotobuf fail when compiling protobufs with gpb >= 4.13.

gpb changed the occurence field from optional to defaulty before version
4.13 (see
tomas-abrahamsson/gpb@a363325).
This made Exprotobuf fail when compiling protobufs with gpb >= 4.13.
@rbino
Copy link
Author

rbino commented Jul 3, 2020

@bitwalker I'm not 100% sure about the implications, let me know if it's ok or if you want give me some pointers to provide the correct solution

@tomas-abrahamsson
Copy link

tomas-abrahamsson commented Jul 3, 2020

In gpb-4.13.0, I changed the internal structure from optional to defaulty (in proto3 message fields only) to make room for the upcoming optional that's new in Google's protobuf 3.12.0. My intention was to make the transition a bit more seamless by making it opt-in, but it seems something has gone wrong, don't know exactly where though. Sorry about that, hoping we can work out a way forward. Here is my thinking:

  • The gpb_compile:file(..., [to_proto_defs, ...]) by default converts back and returns on the old format, ie optional and not defaulty. Only the {proto_def_version,2} option is specified, it would return on the new format. Same for calls to gpb_compile:string
  • If the .proto would contain optional, the gpb_compile:file would produce an error, basically saying that the proto is not representable in the old proto_defs_version format. Exprotobuf could opt-in to the new format by specifying {proto_defs_version,2}.

I wrote this in some more defails in the doc/dev-guide/proto-defs-versions.md file

Edited to include a link to Google's protobuf 3.12.0 field presence documentation

@tomas-abrahamsson
Copy link

It looks like exprotobuf is using functions in gpb_parse and gpb_scan directly instead of going through the api in gpb_compile. This is why the aforementioned seamless opt-in failed here.

I consider the functions in gpb_scan and gpb_parse to be internal to gpb, while gpb_compile is public and documented. I suppose origins are from early days when gpb was more fuzzy about what was internal or public. I think the functions in gpb_compile can do the same work and be more future-proof. If not, please let me know. For reference, some details related to this was discussed earlier.

@tomas-abrahamsson
Copy link

tomas-abrahamsson commented Jul 10, 2020

I think the functions in gpb_compile can do the same work ...

I've come to realize this was not currently the case, due to the way exprotobuf handles multiple files, as described in the imports upgrade guide. However, the way exprotobuf uses functions in gpb_parse and gpb_scan is not very viable either, for example, I've been working a bit on replacing the parser in gpb with a recursive descent-parser to get around the problem of keywords in message names etc, and I have a (temporary) branch new-parser for it now merged that into 4.14.0.

I'll open a separate issue for making exprotobuf use gpb_compile functions instead, and also meanwhile try to think if of some way to make change gpb to better support this use case.

Edit: I have merged the branch and it is included in 4.14.0

@sgerrand
Copy link

👋 @bitwalker, are you able to review this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants