-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix protobuf regeneration script. #5291
Conversation
Need to explicitly include --proto_path /usr/local/include in order to pick up the standard protobuf files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @danielmai and @manishrjain)
a discussion (no related file):
minor comment below.
protos/Makefile, line 46 at r1 (raw file):
@protoc \ --proto_path=/usr/local/include \ --proto_path=${PROTO_PATH} \
Formatting looks odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fixes it on my machine but it changes the go.mod and go.sum files.
if that's expected or once it's fixed.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @danielmai and @manishrjain)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @danielmai and @manishrjain)
Unlike go get, go install does not update the dependency version in go.mod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @manishrjain, @martinmr, and @parasssh)
a discussion (no related file):
Previously, parasssh wrote…
:lgtm: minor comment below.
Done.
protos/Makefile, line 46 at r1 (raw file):
Previously, parasssh wrote…
Formatting looks odd.
Done.
release/v20.03-specific change: use dgo/v2 instead of dgo/v200. This fixes the make regenerate command that should create the generated protobuf code. 1. make regenerate fails to run (error log attached in PR) 2. The protobuf dependencies are unintentionally updated to the latest version. 3. It was referencing protobuf definitions from GOPATH. So, if a package was not updated to the right version (e.g., updating github.com/dgraph-io/badger to the latest master), then there could be errors like the following when referencing the wrong version of a proto: pb.proto:285:18: "badgerpb2.KV" is not defined. pb.proto:297:11: "badgerpb2.KV" is not defined. pb.proto:545:5: "badgerpb2.KVList" is not defined. pb.proto:537:60: "badgerpb2.KVList" is not defined. pb.proto:28:1: warning: Import github.com/dgraph-io/badger/pb/pb.proto is unused. Changes * Include proto path /usr/local/include in protoc command to fix (1). * Update depcheck.sh to check that this directory exists. Error out with installation instructions otherwise if it's missing. * Install protobuf libraries from the versions in go.mod, and use go install instead of go get so the protobuf versions aren't updated automatically to fix (2). * Copy to temporary directory for proper import paths from go mod versions to remove any dependencies on $GOPATH to fix (3). * protos from proto library * proto from badger * proto from dgo * Regenerate protos
release/v20.03-specific change: use dgo/v2 instead of dgo/v200. This fixes the make regenerate command that should create the generated protobuf code. 1. make regenerate fails to run (error log attached in PR) 2. The protobuf dependencies are unintentionally updated to the latest version. 3. It was referencing protobuf definitions from GOPATH. So, if a package was not updated to the right version (e.g., updating github.com/dgraph-io/badger to the latest master), then there could be errors like the following when referencing the wrong version of a proto: pb.proto:285:18: "badgerpb2.KV" is not defined. pb.proto:297:11: "badgerpb2.KV" is not defined. pb.proto:545:5: "badgerpb2.KVList" is not defined. pb.proto:537:60: "badgerpb2.KVList" is not defined. pb.proto:28:1: warning: Import github.com/dgraph-io/badger/pb/pb.proto is unused. Changes * Include proto path /usr/local/include in protoc command to fix (1). * Update depcheck.sh to check that this directory exists. Error out with installation instructions otherwise if it's missing. * Install protobuf libraries from the versions in go.mod, and use go install instead of go get so the protobuf versions aren't updated automatically to fix (2). * Copy to temporary directory for proper import paths from go mod versions to remove any dependencies on $GOPATH to fix (3). * protos from proto library * proto from badger * proto from dgo * Regenerate protos
release/v1.2-specific change: use dgo/v2. This fixes the make regenerate command that should create the generated protobuf code. 1. make regenerate fails to run (error log attached in PR) 2. The protobuf dependencies are unintentionally updated to the latest version. 3. It was referencing protobuf definitions from GOPATH. So, if a package was not updated to the right version (e.g., updating github.com/dgraph-io/badger to the latest master), then there could be errors like the following when referencing the wrong version of a proto: pb.proto:285:18: "badgerpb2.KV" is not defined. pb.proto:297:11: "badgerpb2.KV" is not defined. pb.proto:545:5: "badgerpb2.KVList" is not defined. pb.proto:537:60: "badgerpb2.KVList" is not defined. pb.proto:28:1: warning: Import github.com/dgraph-io/badger/pb/pb.proto is unused. Changes * Include proto path /usr/local/include in protoc command to fix (1). * Update depcheck.sh to check that this directory exists. Error out with installation instructions otherwise if it's missing. * Install protobuf libraries from the versions in go.mod, and use go install instead of go get so the protobuf versions aren't updated automatically to fix (2). * Copy to temporary directory for proper import paths from go mod versions to remove any dependencies on $GOPATH to fix (3). * protos from proto library * proto from badger * proto from dgo * Regenerate protos
release/v1.2-specific change: use dgo/v2. This fixes the make regenerate command that should create the generated protobuf code. 1. make regenerate fails to run (error log attached in PR) 2. The protobuf dependencies are unintentionally updated to the latest version. 3. It was referencing protobuf definitions from GOPATH. So, if a package was not updated to the right version (e.g., updating github.com/dgraph-io/badger to the latest master), then there could be errors like the following when referencing the wrong version of a proto: pb.proto:285:18: "badgerpb2.KV" is not defined. pb.proto:297:11: "badgerpb2.KV" is not defined. pb.proto:545:5: "badgerpb2.KVList" is not defined. pb.proto:537:60: "badgerpb2.KVList" is not defined. pb.proto:28:1: warning: Import github.com/dgraph-io/badger/pb/pb.proto is unused. Changes * Include proto path /usr/local/include in protoc command to fix (1). * Update depcheck.sh to check that this directory exists. Error out with installation instructions otherwise if it's missing. * Install protobuf libraries from the versions in go.mod, and use go install instead of go get so the protobuf versions aren't updated automatically to fix (2). * Copy to temporary directory for proper import paths from go mod versions to remove any dependencies on $GOPATH to fix (3). * protos from proto library * proto from badger * proto from dgo * Regenerate protos
This fixes the make regenerate command that should create the generated protobuf code. 1. make regenerate fails to run (error log attached in PR) 2. The protobuf dependencies are unintentionally updated to the latest version. 3. It was referencing protobuf definitions from GOPATH. So, if a package was not updated to the right version (e.g., updating github.com/dgraph-io/badger to the latest master), then there could be errors like the following when referencing the wrong version of a proto: pb.proto:285:18: "badgerpb2.KV" is not defined. pb.proto:297:11: "badgerpb2.KV" is not defined. pb.proto:545:5: "badgerpb2.KVList" is not defined. pb.proto:537:60: "badgerpb2.KVList" is not defined. pb.proto:28:1: warning: Import github.com/dgraph-io/badger/pb/pb.proto is unused. Changes * Include proto path /usr/local/include in protoc command to fix (1). * Update depcheck.sh to check that this directory exists. Error out with installation instructions otherwise if it's missing. * Install protobuf libraries from the versions in go.mod, and use go install instead of go get so the protobuf versions aren't updated automatically to fix (2). * Copy to temporary directory for proper import paths from go mod versions to remove any dependencies on $GOPATH to fix (3). * protos from proto library * proto from badger * proto from dgo * Regenerate protos
In master, the
make regenerate
command that should create the generated protobuf code doesn't work:make regenerate
fails to run, showing the following error messages:make regenerate
error logThe protobuf dependencies are updated in the latest version, which is not intended.
It was referencing protobuf definitions from GOPATH. So, if a package was not updated to the right version (e.g., updating
github.com/dgraph-io/badger
to the latest master), then there could be errors like these:Changes
go install
instead ofgo get
so the protobuf versions aren't updated automatically to fix (2).$GOPATH
to fix (3).This change is