Skip to content
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 make protoc #37

Closed
wants to merge 2 commits into from
Closed

Fix make protoc #37

wants to merge 2 commits into from

Conversation

orkunkl
Copy link
Contributor

@orkunkl orkunkl commented Aug 14, 2019

I fixed the bug that blocked #23.
In gogo/protobuf#601, @apelisse pointed me to the problems cause. It was spec/github.com/iov-one/weave/codec.proto.
Reason:

  • includes field contains which import paths will be used when running protoc.
    • . means that import "x/custom/codec.go" will work inside this app (recommended to avoid GOPATH issues)
    • spec means that we can import "github.com/iov-one/weave/coins/codec.proto" and it will use the protobuf file from our spec directory (which must be kept up to date with the weave repo).
    • spec/github.com/iov-one/weave is needed as if we import eg. github.com/iov-one/weave/x/cash/codec.proto, it imports coins/codec.proto using relative imports to it's project. When we then import this via spec, the relative import doesn't work without this second line.

Everything seems okay until running make protoc. spec folder and spec/github.com/iov-one/weave folder includes weave package so the protoc generates these imports in x/custom/codec.pb.go:

import (
  github_com_iov_one_weave "github.com/iov-one/weave"
  weave "github.com/iov-one/weave"
)

The duality of github.com/iov-one/weave does not effect applications runtime but proto compiler gets confused. Solution is to add

option go_package = "github.com/iov-one/weave";

to spec/github.com/iov-one/weave/codec.proto. Now everything returned back to normal in protoc's aspect. Note: if you want to read more about option go_package here is an article.

If you have a better solution than Makefile, please let me know :)
Also thank you @apelisse :)

Resolves gogo/protobuf#601

@orkunkl orkunkl requested review from husio, alpe and ruseinov August 14, 2019 17:33
@husio
Copy link

husio commented Aug 15, 2019

Great finding! How about adding it to weave instead of patching here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

protoc-gen-gogofaster failed with status code 1 yet compiles successfully
2 participants