ptypes: timestamp.pb.go registers the file descriptor under incorrect source .proto path

[ivucica]

G'day,

For protos that live at ${GOPATH}/src/example.com/pkg/myproject, I am importing the well-known google.protobuf.Timestamp type/message:

option go_package = "example.com/pkg/myproject/proto";
import "google/protobuf/timestamp.proto";

I am generating the output as follows:

protoc --go_out=plugins=grpc:${GOPATH}/src -I. contests.proto

I'm doing so because I'd like to make the contests.proto usable in other languages as well.

If I try to get grpc_cli to call a method in the Contests service, or even list the methods inside it, I get this:

$ /home/ivucica/projects/grpc/grpc_cli ls localhost:1343  com.example.pkg.myproject.proto.Contests
[libprotobuf ERROR external/submodule_protobuf/src/google/protobuf/descriptor.cc:3369] Invalid proto descriptor for file "contests.proto":
[libprotobuf ERROR external/submodule_protobuf/src/google/protobuf/descriptor.cc:3372]   contests.proto: Import "google/protobuf/timestamp.proto" was not found or had errors.
[libprotobuf ERROR external/submodule_protobuf/src/google/protobuf/descriptor.cc:3372]   com.example.pkg.myproject.proto.Contest.start_time: "google.protobuf.Timestamp" seems to be defined in "github.com/golang/protobuf/ptypes/timestamp/timestamp.proto", which is not imported by "contests.proto".  To use it here, please add the necessary import.
[libprotobuf ERROR external/submodule_protobuf/src/google/protobuf/descriptor.cc:3372]   com.example.pkg.myproject.proto.Contest.end_time: "google.protobuf.Timestamp" seems to be defined in "github.com/golang/protobuf/ptypes/timestamp/timestamp.proto", which is not imported by "contests.proto".  To use it here, please add the necessary import.
[libprotobuf ERROR external/submodule_protobuf/src/google/protobuf/descriptor.cc:3369] Invalid proto descriptor for file "contests.proto":
[libprotobuf ERROR external/submodule_protobuf/src/google/protobuf/descriptor.cc:3372]   contests.proto: Import "google/protobuf/timestamp.proto" was not found or had errors.
[libprotobuf ERROR external/submodule_protobuf/src/google/protobuf/descriptor.cc:3372]   com.example.pkg.myproject.proto.Contest.start_time: "google.protobuf.Timestamp" seems to be defined in "github.com/golang/protobuf/ptypes/timestamp/timestamp.proto", which is not imported by "contests.proto".  To use it here, please add the necessary import.
[libprotobuf ERROR external/submodule_protobuf/src/google/protobuf/descriptor.cc:3372]   com.example.pkg.myproject.proto.Contest.end_time: "google.protobuf.Timestamp" seems to be defined in "github.com/golang/protobuf/ptypes/timestamp/timestamp.proto", which is not imported by "contests.proto".  To use it here, please add the necessary import.
[libprotobuf ERROR external/submodule_protobuf/src/google/protobuf/descriptor.cc:3372]   Service or method com.example.pkg.myproject.proto.Contests not found.

---

This is instructing me to import github.com/golang/protobuf/ptypes/timestamp/timestamp.proto inside contests.proto.

If I perform that change as follows, the problem is resolved:

// import inside contests.proto
import "github.com/golang/protobuf/ptypes/timestamp/timestamp.proto";
// regeneration
protoc --go_out=plugins=grpc:${GOPATH}/src -I../../.. -I. contests.proto

In that case, clearly, the import matches the self-registered file path that the generated github.com/golang/protobuf/ptypes/timestamp/timestamp.pb.go embeds.

---

However, I strongly dislike importing a language-specific path to the definition of timestamp.proto, even if they are identical. If I were in future to generate Python client code, would I be expected to still refer to the github.com/golang/protobuf copy of the proto file? That seems wrong.

It also seems like a horrible idea to have a build step, before running protoc, that would do approximately:

sed -E 's@import "google/protobuf/(.*)\.proto"@import "github.com/golang/protobuf/ptypes/\1/\1.proto"@'

---

Could the files please be regenerated in a way that makes them refer to google/protobuf/timestamp.proto as the filename?

Alternatively, could protoc (or, less likely, protoc-gen-go) be hacked to import github.com/golang/protobuf/ptypes/timestamp/timestamp.proto when import of google/protobuf/timestamp.proto is requested?

Is there maybe a better alternative?

[ivucica]

(This is vaguely, but not fully, related to #222)

[ahmetb]

I am having the same issue. Opened an issue at grpc repository, though I'm not sure if this is a grpc-go or protoc-gen-go problem at all. grpc/grpc#10304 grpc/grpc-go#1163

[jvsteiner]

I've had this issue with paths other than google/protobuf/timestamp.proto - From my perspective, if protoc can generate the code, go can compile it, and the service can interact with it, but the reflection service can't provide the required definitions, it's clear that there is a problem in the refection code.

[dsnet]

The .proto files for the golang/protobuf repository were mirrored here primarily for reference when generating the .pb.go files for well-known types. When protoc compiles a .proto file, it should be provided an include path that points to the main google/protobuf repository for the well-known types. Users should not be importing the .proto files in this repository.

Making as a documentation bug since the README could explain this better.

[ivucica]

But, @dsnet, when I originally reported this bug, I was merely importing google/protobuf/timestamp.proto which back then I provided in the current working directory. At least as far as I remember?

Additionally, the timestamp.proto in this repo seems to match the one in github.com/google/protobuf://src/google/protobuf/timestamp.proto, anyway.

[dsnet]

The contents of timestamp.proto here is identical to that of the google/protobuf repo, but the include path is not correct.

From the error message, it seems that golang/protobuf was part of the include path for protoc, but the google/protobuf repo was not. Depending on how protoc was installed on your system, the correct definition of the well-known types may or may not be included by default.

[ivucica]

I don’t follow. If the protoc could find the proto file, surely it was in the include path? Surely then the runtime error from grpc_cli should not refer to anything other than the filename I specified — google/protobuf/timestamp.proto, given that I specified no other include path? To be fair, at this point I’m just assuming what I wrote above was correct. Because honestly, so much time has passed and I haven’t seen this behavior since then that whatever issue was there was since fixed either in protoc-gen-go, or in my build scripts. I’m just confused as to which path do you suspect I installed timestamp.proto? How could I add pwd into the include search path (-I.), therefore presumably put the file in ${PWD}/google/protobuf/timestamp.proto, but then observe grpc runtime searching for the file with identifier containing the go_package option, as shipped upstream? Why would that happen? Or how would protoc know where to find the file in the go_package, if I included google/protobuf/timestamp.proto? I’m very confused and think that, whatever I reported back then, was a genuine bug :-) I could maybe try reproducing with a commit from around that time and report back?

On Mon 20 Aug 2018 at 02:55 Joe Tsai ***@***.***> wrote: The *contents* of timestamp.proto here is identical to that of the google/protobuf repo, but the include path is not correct. From the error message, it seems that golang/protobuf was part of the include path for protoc, but the google/protobuf repo was not. Depending on how protoc was installed on your system, the correct definition of the well-known types may or may not be included by default. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#298 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAdJnD1tQdnIFtljvWmXYnIo6FByzv6Qks5uShcngaJpZM4MLLA4> .

-- Sent from Gmail Mobile

[dsnet]

Several things to note:

• grpc_cli is invoking protoc, which is invoking protoc-gen-go.
• This error is produced protoc and not protoc-gen-go. This repository is only responsible for protoc-gen-go and not any of the previously mentioned binaries that invoke it.
• Actually, protoc did not find the file, it found a file similar to it (i.e., the golang/protobuf copy), but it is not the correct file since the include path is different. For this reason, it uses the wording "seems to be defined in [elsewhere]", meaning it did not find what it was looking.
• (not positive about this), but it seems that protoc has gotten better in recent years about including the well-known types by default. I do remember hitting similar problems in the past, but not really.

All this to say: this isn't an issue with protoc-gen-go, but rather with how protoc is invoked. It could also be an issue with the set of include directories that grpc_cli passes to protoc, however I have seen this issue myself (in the distant past) and believe it can be documented better.

[puellanivis]

So, if I understand your position @dsnet , the documentation needs to be made to protoc, not the more narrow case of protoc-gen-go ?

[ivucica]

On Mon 20 Aug 2018 at 05:28 Joe Tsai ***@***.***> wrote: Several things to note: - This error is not produced by protoc-gen-go, but rather by protoc, which is not something we maintain in this repository.

The error was produced by grpc_cli based on paths embedded in .pb.go.

- Actually, protoc did not find the file, it found a file similar to it (i.e., the golang/protobuf copy), but it is not the correct file since the include path is different. For this reason, it uses the wording "seems to be defined in [elsewhere]", meaning it did not find what it was looking.

Again, error was happening at runtime long after protoc finished invoking protoc-gen-go and .pb.go files were produced with descriptors containing the wrong path.

- (not positive about this), but it seems that protoc has gotten better in recent years about including the well-known types by default. I do remember hitting similar problems in the past, but not really. All this to say: this isn't an issue with protoc-gen-go, but rather with how protoc is invoked.

This may very well be a problem with how I invoked protoc and protoc-gen-go, but I don’t think in the way you described? Again, I will try to take some time and reproduce this so I can be sure what I was talking about last year, or so I can say “sorry, even I cannot reproduce this”. Because I’m just going by my original report, which says that the error is thrown by grpc_cli after it uses introspection to fetch descriptors (read from .pb.go files) so it could dynamically construct binary protos out of ascii protos provided on CLI.

[dsnet]

Again, error was happening at runtime long after protoc finished invoking
protoc-gen-go and .pb.go files were produced with descriptors containing
the wrong path.

At runtime, it seem that grpc_cli either shells out to protoc or directly links in libprotobuf and calls protoc-like APIs. Either way, my argument for why this is an issue with how grpc_cli invokes the protobuf compiler still applies. It seems users did file this issue against grpc, but I don't know why that issue was closed without explanation. The last comment has a "workaround" which is definitely not the proper solution.

If possible, it would help greatly to debug the problem if there was a way to get grpc_cli to print the include directories it is passing to protoc.

@puellanivis, the degree of documentation that we can do in this golang/protobuf repo is to document in the ptypes package that users should import "google/protobuf/timestamp.proto" as opposed to "github.com/golang/protobuf/ptypes/timestamp/timestamp.proto". We could even consider deleting the mirrored .proto files to remove the temptation for misuse.

[ivucica]

At runtime, it seem that grpc_cli either shells out to protoc or directly links in libprotobuf and calls protoc-like APIs

It links in libprotobuf. https://github.com/grpc/grpc/tree/master/test/cpp/util

Either way, my argument for why this is an issue with how grpc_cli invokes the protobuf compiler still applies.

It doesn't.

I've first taken grpc-ecosystem/grpc-gateway from Feb 24 2017 f3aa758b8ae01abecf727dba035444545dbe41de and golang/protobuf from Feb 17 2017 69b215d which would have been the latest as of writing the original bug report.

Then I have rebuilt my project, though it turns out I did not have to.

Because see... it is a bug in what was shipped in github.com/golang/protobuf. And it's not a bug in how an end user such as myself has invoked protoc or how protoc-gen-go was invoked. It's a bug in how the committer of prebuilt files has invoked protoc.

This is the latest version:

protobuf/ptypes/timestamp/timestamp.pb.go

Line 158 in 05f48f4

proto.RegisterFile("google/protobuf/timestamp.proto", fileDescriptor_timestamp_b826e8e5fba671a8)

Alright, next up, @dsnet, can you spot the "possible" issue about why gRPC's standard reflection API would not serve the file back to grpc_cli and thus libprotobuf (or whichever lib is used)?

protobuf/ptypes/timestamp/timestamp.pb.go

Line 109 in 69b215d

proto.RegisterFile("github.com/golang/protobuf/ptypes/timestamp/timestamp.proto", fileDescriptor0)

See, the file was being registered by the upstream code into the proto registry (seemingly used as an in-memory file system when the reflection service is exposed) under an incorrect name. The bug was in the golang/protobuf repo.

Therefore:

1. this bug absolutely deserves the original name -- user has not mis-included the .proto file and the user has not incorrectly changed the include path or import path or mapping from .proto to the Go package.
2. this bug can be closed, but for the reason that the underlying issue (which I originally reported -- incorrect path in the .pb.go!) has been solved! The .pb.go file has been regenerated containing the correct source filename when doing file registration in the runtime.

---

Now, a bit of very friendly feedback on our interactions 😃

I'll be frank: I understand you may have been trying to help, but I would have seriously appreciated if you had read the original bug report before outright telling me that I am invoking protoc incorrectly.

• Yes, I have not looked at this particular bug in a while (so I was initially not 100% sure what I was referring to).

• Yes, I have not encountered the bug in a while (so, again, I was initially not 100% sure what I was referring to).

• Yes, I have not looked at the bug in detail immediately when you responded to the bug (as I went to work).

• But, I actually did triage the issue correctly! After all, I stated this:

  In that case, clearly, the import matches the self-registered file path that the generated github.com/golang/protobuf/ptypes/timestamp/timestamp.pb.go embeds.

  and

  Could the files please be regenerated in a way that makes them refer to google/protobuf/timestamp.proto as the filename?

  which is exactly what the problem was.

But if you wanted to address this bug, understanding of gRPC's reflection, accompanied by knowledge of grpc_cli tool's existence (which, in my view, is very important to anyone working with gRPC or trying to address gRPC-specific bugs -- just like stubby CLI is important internally) would have revealed to you very quickly where the bug may have lied >1yr ago, without needing to suggest my incompetence in invoking the tool.

Seriously, look at the original bug report 😃 In it, I have documented how I invoked the tool. If you see, I am nowhere trying to tell protoc and protoc-gen-go to look for timestamp.proto elsewhere. Only attempting to work around the issue did I start using the wrong path. I was not using the wrong path originally. The bug was that I was unable to use the correct path.

Hypothetically, you could assume I am doing something very strange at runtime -- but that's an assumption you did not make. You have repeatedly stated that I am invoking protoc incorrectly. I was not.

Please, please, please read the bugs and only then tell people that they have done something they have not. 😃

Most importantly: thank you for your work on this project!

[dsnet]

@ivucica, thank you for your analysis and feedback. You are correct that I mis-diagnosed the issue. I apologize.

Only attempting to work around the issue did I start using the wrong path. I was not using the wrong path originally. The bug was that I was unable to use the correct path.

It seems that I may have latched onto the wrong part of the initial post and incorrectly derived my conclusions from it. It was not my intent to say you were incompetent; your analysis clearly shows you are very competent at getting to the root of issues.

Please, please, please read the bugs and only then tell people that they have done something they have not. 😃

I apologize for not reading this issue deeply enough. There were other old issues that I did actually spend quite a bit of time on only to discover that there was nothing actionable to do on golang/protobuf's end. Unfortunately, this was not one of them 😐.

Unfortunately this project has been in a state of relative non-maintenance for a period of 2 years or so until more recent months. I have been periodically going through the backlog of issues, pull-requests, and other development work (including work not related to protobufs), so I'll admit that the time I spent on each issue is unfortunately not as much as it ought to. As a form of reverse feedback, it would be helpful to include a commit hash of the version of golang/protobuf used.

Again, thank you for the feedback. It is appreciated.

[ivucica]

Unfortunately this project has been in a state of relative non-maintenance for a period of 2 years or so until more recent months. I have been periodically going through the backlog of issues, pull-requests, and other development work (including work not related to protobufs)

This is extremely appreciated. Thank you for your efforts.

And definitely thank you for bringing this bug up! Were it not for you looking at it, it would have lingered unclosed for much longer.

As a form of reverse feedback, it would be helpful to include a commit hash of the version of golang/protobuf used.

I absolutely recognize this mistake -- not doing so gave me trouble as well. Apologies!