New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mentioning the bit about proto_path up front #355

Merged
merged 2 commits into from Nov 23, 2017

Conversation

Projects
None yet
2 participants
@agnivade
Contributor

agnivade commented Nov 17, 2017

I had to spend quite some time trying to debug and understand how to setup the proto_path. The example in the Makefile just shows some relative paths which are not very helpful.

I think mentioning them directly in the README will save every one a lot of time. (It sure would have done for mine !)

@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Nov 17, 2017

Member
Member

awalterschulze commented Nov 17, 2017

@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Nov 17, 2017

Member
Member

awalterschulze commented Nov 17, 2017

@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Nov 23, 2017

Member

Sorry about the vacation time. That got in the way here.

I think maybe skewing the example to gogo_out and protoc-gen-gogo, so that its immediately useful and then adding a sentence below about that how this could be replaced with your own binary.

What do you think?

Member

awalterschulze commented Nov 23, 2017

Sorry about the vacation time. That got in the way here.

I think maybe skewing the example to gogo_out and protoc-gen-gogo, so that its immediately useful and then adding a sentence below about that how this could be replaced with your own binary.

What do you think?

@agnivade

This comment has been minimized.

Show comment
Hide comment
@agnivade

agnivade Nov 23, 2017

Contributor

Alright. But gogo_out is the same as usual protobuf right ? How about using gogofaster_out ?

Contributor

agnivade commented Nov 23, 2017

Alright. But gogo_out is the same as usual protobuf right ? How about using gogofaster_out ?

@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Nov 23, 2017

Member

Ok wait. My bad.

Member

awalterschulze commented Nov 23, 2017

Ok wait. My bad.

@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Nov 23, 2017

Member

Now that I read it again, I think what you have here is perfect.
Sorry I forgot in which section we were :)

Member

awalterschulze commented Nov 23, 2017

Now that I read it again, I think what you have here is perfect.
Sorry I forgot in which section we were :)

@awalterschulze awalterschulze merged commit 971cbfd into gogo:master Nov 23, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Nov 23, 2017

Member

Thank you for the clarification in the readme.
I think this will help a lot of newcomers :)

Member

awalterschulze commented Nov 23, 2017

Thank you for the clarification in the readme.
I think this will help a lot of newcomers :)

@agnivade agnivade deleted the agnivade:agnivade-protopath branch Nov 23, 2017

@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Feb 6, 2018

Member

Btw @agnivade great blog post, but I guess I am a bit biased :)
Would you like to add it to the Readme.md in the mentioned section?

Could I maybe ask a favour, that you clarify this statement:

Yes, using gogoproto, you cannot use protocol buffer 3 because it does not support extensions. There is an active issue open which discusses this in further detail.

By saying

Yes, using gogoproto, you cannot use proto3 if you intend to share your protobuf definition with languages which do not support proto2, like php, because proto3 does not support extensions. There is an active issue open which discusses this in further detail.

This means that mostly you can use proto3 with gogoprotobuf.
Also another way to get around the issue would be to not use any extensions and simply call gogoslick_out, for the case in the blog post.

Member

awalterschulze commented Feb 6, 2018

Btw @agnivade great blog post, but I guess I am a bit biased :)
Would you like to add it to the Readme.md in the mentioned section?

Could I maybe ask a favour, that you clarify this statement:

Yes, using gogoproto, you cannot use protocol buffer 3 because it does not support extensions. There is an active issue open which discusses this in further detail.

By saying

Yes, using gogoproto, you cannot use proto3 if you intend to share your protobuf definition with languages which do not support proto2, like php, because proto3 does not support extensions. There is an active issue open which discusses this in further detail.

This means that mostly you can use proto3 with gogoprotobuf.
Also another way to get around the issue would be to not use any extensions and simply call gogoslick_out, for the case in the blog post.

@agnivade

This comment has been minimized.

Show comment
Hide comment
@agnivade

agnivade Feb 7, 2018

Contributor

Thanks for reading the blog post :)

Hmm .. interesting. I actually got a compiler error when I tried to use syntax = "proto3";. Is it because I used gogofaster and not gogoslick ?

Contributor

agnivade commented Feb 7, 2018

Thanks for reading the blog post :)

Hmm .. interesting. I actually got a compiler error when I tried to use syntax = "proto3";. Is it because I used gogofaster and not gogoslick ?

@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Feb 7, 2018

Member
Member

awalterschulze commented Feb 7, 2018

@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Feb 7, 2018

Member
Member

awalterschulze commented Feb 7, 2018

@agnivade

This comment has been minimized.

Show comment
Hide comment
@agnivade

agnivade Feb 7, 2018

Contributor

I just updated the blog post. Will send a PR soon.

Regarding the error. This is my proto file -

syntax = "proto3";
package xxxx;

import "github.com/gogo/protobuf/gogoproto/gogo.proto";

option (gogoproto.gostring_all) = true;
option (gogoproto.goproto_stringer_all) = false;
option (gogoproto.stringer_all) =  true;
option (gogoproto.marshaler_all) = true;
option (gogoproto.sizer_all) = true;
option (gogoproto.unmarshaler_all) = true;

// For tests
option (gogoproto.testgen_all) = true;
option (gogoproto.equal_all) = true;
option (gogoproto.populate_all) = true;

// This is the message sent to the cloud server
message ClientMessage {
  string Field1 = 1 [(gogoproto.nullable) = false];
  string Field2 = 2 [(gogoproto.nullable) = false];
  int64 Timestamp = 3 [(gogoproto.nullable) = false];
}

And this is the error -

protoc -I=. -I=/home/agniva/play/go/src -I=/home/agniva/play/go/src/github.com/gogo/protobuf/protobuf --gogofaster_out=. *.proto
ERROR: field ClientMessage.Field1 is a native type and in proto3 syntax with nullable=false there exists conflicting implementations when encoding zero values--gogofaster_out: protoc-gen-gogofaster: Plugin failed with status code 1.

This works fine -

syntax = "proto2";
package xxxx;

import "github.com/gogo/protobuf/gogoproto/gogo.proto";

option (gogoproto.gostring_all) = true;
option (gogoproto.goproto_stringer_all) = false;
option (gogoproto.stringer_all) =  true;
option (gogoproto.marshaler_all) = true;
option (gogoproto.sizer_all) = true;
option (gogoproto.unmarshaler_all) = true;

// For tests
option (gogoproto.testgen_all) = true;
option (gogoproto.equal_all) = true;
option (gogoproto.populate_all) = true;

// This is the message sent to the cloud server
message ClientMessage {
  required  string Field1 = 1 [(gogoproto.nullable) = false];
  required  string Field2 = 2 [(gogoproto.nullable) = false];
  required  int64 Timestamp = 3 [(gogoproto.nullable) = false];
}
Contributor

agnivade commented Feb 7, 2018

I just updated the blog post. Will send a PR soon.

Regarding the error. This is my proto file -

syntax = "proto3";
package xxxx;

import "github.com/gogo/protobuf/gogoproto/gogo.proto";

option (gogoproto.gostring_all) = true;
option (gogoproto.goproto_stringer_all) = false;
option (gogoproto.stringer_all) =  true;
option (gogoproto.marshaler_all) = true;
option (gogoproto.sizer_all) = true;
option (gogoproto.unmarshaler_all) = true;

// For tests
option (gogoproto.testgen_all) = true;
option (gogoproto.equal_all) = true;
option (gogoproto.populate_all) = true;

// This is the message sent to the cloud server
message ClientMessage {
  string Field1 = 1 [(gogoproto.nullable) = false];
  string Field2 = 2 [(gogoproto.nullable) = false];
  int64 Timestamp = 3 [(gogoproto.nullable) = false];
}

And this is the error -

protoc -I=. -I=/home/agniva/play/go/src -I=/home/agniva/play/go/src/github.com/gogo/protobuf/protobuf --gogofaster_out=. *.proto
ERROR: field ClientMessage.Field1 is a native type and in proto3 syntax with nullable=false there exists conflicting implementations when encoding zero values--gogofaster_out: protoc-gen-gogofaster: Plugin failed with status code 1.

This works fine -

syntax = "proto2";
package xxxx;

import "github.com/gogo/protobuf/gogoproto/gogo.proto";

option (gogoproto.gostring_all) = true;
option (gogoproto.goproto_stringer_all) = false;
option (gogoproto.stringer_all) =  true;
option (gogoproto.marshaler_all) = true;
option (gogoproto.sizer_all) = true;
option (gogoproto.unmarshaler_all) = true;

// For tests
option (gogoproto.testgen_all) = true;
option (gogoproto.equal_all) = true;
option (gogoproto.populate_all) = true;

// This is the message sent to the cloud server
message ClientMessage {
  required  string Field1 = 1 [(gogoproto.nullable) = false];
  required  string Field2 = 2 [(gogoproto.nullable) = false];
  required  int64 Timestamp = 3 [(gogoproto.nullable) = false];
}
@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Feb 7, 2018

Member
Member

awalterschulze commented Feb 7, 2018

@agnivade

This comment has been minimized.

Show comment
Hide comment
@agnivade

agnivade Feb 7, 2018

Contributor

Confirmed. Works. 😄

Yes, the error message could certainly improve. Especially the last part - there exists conflicting implementations when encoding zero values--gogofaster_out: protoc-gen-gogofaster. The user doesn't know exactly what needs to be done.

Contributor

agnivade commented Feb 7, 2018

Confirmed. Works. 😄

Yes, the error message could certainly improve. Especially the last part - there exists conflicting implementations when encoding zero values--gogofaster_out: protoc-gen-gogofaster. The user doesn't know exactly what needs to be done.

@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Feb 7, 2018

Member
Member

awalterschulze commented Feb 7, 2018

@agnivade

This comment has been minimized.

Show comment
Hide comment
@agnivade

agnivade Feb 7, 2018

Contributor

Absolutely.

Contributor

agnivade commented Feb 7, 2018

Absolutely.

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