Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

move to proto3 #2887

Merged
merged 5 commits into from
Jan 19, 2018
Merged

move to proto3 #2887

merged 5 commits into from
Jan 19, 2018

Conversation

berler
Copy link
Contributor

@berler berler commented Jan 19, 2018

Goals (and why): Upgrade to the latest protobuf library, so that we can similarly upgrade internal projects that depend on atlas. The old version we use now is vulnerable to protocolbuffers/protobuf#760

Implementation Description (bullets):

  • deletes defunct proto project/dir
  • use the newest protobuf gradle plugin
  • use protobuf 3.5.1 (currently the latest)
  • correctly lock in protobuf-java version
  • .proto files are now located in a place that makes more sense
  • generated proto java sources are no longer checked into git

Note that all .proto files still use the proto2 spec. It is not trivial to change existing protobufs to use the proto3 spec in a way that is backwards compatible due to using features not present in proto3 (custom default values, enums don't have a zero value)

Concerns (what feedback would you like?):

Where should we start reviewing?:

Priority (whenever / two weeks / yesterday): Normal


This change is Reviewable

* deletes defunct proto project/dir
* use the newest protobuf gradle plugin
* use protobuf 3.5.1
* correctly lock in protobuf-java version
* .proto files are now located in a place that makes more sense
* generated proto java sources are no longer checked into git

Note that all .proto files still use the proto2 spec.
@berler berler requested review from gsheasby and clockfort January 19, 2018 02:57
The proto3 compiler generates slightly different classes, so we must use
GeneratedMessageV3 instead.
typo in the release notes table
Copy link
Contributor

@gsheasby gsheasby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified that running ./gradlew idea correctly generates the ...Persistence java files.

@berler
Copy link
Contributor Author

berler commented Jan 19, 2018

After looking at this more, I think actually changing GeneratedMessage -> AbstractMessage instead will be better, since both GeneratedMessage and GeneratedMessageV3 extend AbstractMessage. Hopefully this will mean that schemas compiled with the proto2 compiler would still work (so people shouldn't even need to recompile).

This should hopefully make schemas compiled with the old protobuf
compiler still work.
@berler berler merged commit ed78890 into develop Jan 19, 2018
@berler berler deleted the feature/proto3 branch January 19, 2018 20:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants