-
Notifications
You must be signed in to change notification settings - Fork 36
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
Ensure .proto file name is globally unique #125
Conversation
Signed-off-by: Eric Weber <eric.weber@suse.com>
18f6a75
to
5fa8754
Compare
TMP_DIR="${TMP_DIR_BASE}/github.com/longhorn/backing-image-manager/pkg/rpc" | ||
mkdir -p "${TMP_DIR}" | ||
cp "${PKG_DIR}/rpc.proto" "${TMP_DIR}/rpc.proto" | ||
protoc -I "${TMP_DIR_BASE}" -I "proto/vendor/protobuf/src/" "${TMP_DIR}/rpc.proto" --go_out=plugins=grpc:"${TMP_DIR_BASE}" |
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.
Quick question. Is it ok to combine the import path like:
protoc -I "${TMP_DIR_BASE}" "proto/vendor/protobuf/src/" "${TMP_DIR}/rpc.proto" --go_out=plugins=grpc:"${TMP_DIR_BASE}"
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.
Looks like no
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.
I spoke with @PhanLe1010 offline and we are agreed. I tried to find a good primary source for protoc
(not protoc-gen-go
) flags, but struggled. The protoc --help
output provides the following direction:
-IPATH, --proto_path=PATH Specify the directory in which to search for
imports. May be specified multiple times;
directories will be searched in order. If not
given, the current working directory is used.
If not found in any of the these directories,
the --descriptor_set_in descriptors will be
checked for required proto file.
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.
LGTM. Thanks for the PR
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.
LGTM.
Suggest applying the same way to other repos if needed.
cc @longhorn/dev
In service of longhorn/longhorn#5595.
Longhorn-manager has a BackingImageManagerService client, and thus imports backing-image-manager. After upgrading the Kubernetes dependency of longhorn-manager to v1.27.1, I came across this error when executing longhorn-manager:
After reading the referenced web page and doing some additional digging, I came across golang/protobuf#1122. For reasons debated in that issue, the name that feeds into the
proto.RegisterFile
function in any.pb.go
file must be globally unique in a Go program. Unfortunately, the Kubernetes version update introduced a new transitive dependency ongo.etcd.io/etcd/api/v3
, which also has anrpc.proto
file that conflicts with backing-image-manager'srpc.proto
file. Since we can't easily change that one, it makes sense to more fully specify ours.The entire purpose of this PR is to change this:
proto.RegisterFile("rpc.proto", fileDescriptor_77a6da22d6a3feb1)
,to this:
proto.RegisterFile("github.com/longhorn/backing-image-manager/pkg/rpc/rpc.proto", fileDescriptor_cdf73a3350f27fd7)
.Sadly,
protoc
makes this a bit annoying to do, resulting in the file copying I did ingenerate_grpc.sh
.