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

adjust protoc execution to work with Go modules #663

Closed
wants to merge 2 commits into from

Conversation

husio
Copy link
Contributor

@husio husio commented May 23, 2019

This is a draft as there are few tasks to be completed before merging.

We no longer use GOPATH and dep for dependency management. Using Go
modules requires changes to how protobuf files are declared and
included.

  • protofmt command no longer works as it does not accept -I flag
  • protodoc is disabled for now as it requires a single protobuf. This
    is waiting for cmd/collectproto: a new tool for collecting protobuf #659
  • some packages cannot be compiled unless weave top level codec.proto
    is included. Compiler is printing a warning that this file is not
    used, but it does not compile without it. Weird.

fix #568

We no longer use `GOPATH` and `dep` for dependency management. Using Go
modules requires changes to how protobuf files are declared and
included.

- `protofmt` command no longer works as it does not accept `-I` flag
- `protodoc` is disabled for now as it requires a single protobuf. This
   is waiting for #659
- some packages cannot be compiled unless weave top level `codec.proto`
  is included. Compiler is printing a warning that this file is not
  used, but it does not compile without it. Weird.

fix #568
@husio husio requested review from alpe and ethanfrey May 23, 2019 11:49
@ethanfrey
Copy link
Contributor

Note this in https://github.com/iov-one/weave/blob/master/prototool.yaml

  # Additional paths to include with -I to protoc.
  # By default, the directory of the config file is included,
  # or the current directory if there is no config file.
  includes:
    - .
    - ./vendor
    - ../../..

I'm looking to see how to configure this via cli or environment, but this is where you can pass the -I info down the chain

@ethanfrey
Copy link
Contributor

ethanfrey commented May 23, 2019

Looking here I also noticed:

  # The Protobuf version to use from https://github.com/protocolbuffers/protobuf/releases.
  # By default use 3.6.1.
  # You probably want to set this to make your builds completely reproducible.
  version: 3.6.1

While we install v3.7.0 via the Makefile (make /usr/local/bin/protoc)

(This is an older issue, predating this PR)

@husio
Copy link
Contributor Author

husio commented May 23, 2019

I have just found out on the prototool page that it does not play well with the long names as well. https://github.com/uber/prototool#tips-and-tricks

@ethanfrey
Copy link
Contributor

Running make protoc prints out some interesting errors
like vendor/github.com/tendermint/tendermint/abci/types/types.proto: Import "github.com/gogo/protobuf/gogoproto/gogo.proto" was not found or had errors.

Please exclude ./vendor from the find (maybe this goes away when we no longer use current vendor based tools approach, but nice for safety and for the transition)

@ethanfrey
Copy link
Contributor

ethanfrey commented May 23, 2019

Also, lines like this seem to be odd:
But is this what you meant by "doesn't compile without reference to codec.proto" (eg self-reference).
I was able to remove both these lines without problem
(compiles well once I removed the vendor dir)

import "codec.proto";
import "gogoproto/gogo.proto";

import "codec.proto";
import "gogoproto/gogo.proto";

@ethanfrey
Copy link
Contributor

I have just found out on the prototool page that it does not play well with the long names as well. https://github.com/uber/prototool#tips-and-tricks

Two of those three tips are against our attempts to vendor...
But... do we need to vendor gogoproto for prototool? Or just for protoc?

@ethanfrey
Copy link
Contributor

Maybe this is a good config to use as reference: https://github.com/QubitProducts/iris/blob/master/prototool.yaml

Or we just use prototool in a docker container. And save a whole lot of headaches. We can fork the docker container and set the versions inside to anything we want. But don't worry about having to build protobuf with the same source/deps as the actual running code. Maybe we just use protoc in docker as well...
https://github.com/charithe/prototool-docker

@ethanfrey
Copy link
Contributor

If I remove the protoc.includes directives from prototool.yaml, this works fine:

USER="$(id -u):$(id -g)"
docker run -it --rm --user $USER --mount type=bind,source="$(pwd)",target=/work --tmpfs /tmp:exec charithe/prototool-docker prototool format /work/codec.proto

@ethanfrey
Copy link
Contributor

Very interesting...

docker run -it --rm --user $USER --mount type=bind,source="$(pwd)",target=/work --tmpfs /tmp:exec charithe/prototool-docker prototool compile --dry-run /work/codec.proto
will show the paths that it would compile

docker run -it --rm --user $USER --mount type=bind,source="$(pwd)",target=/work --tmpfs /tmp:exec charithe/prototool-docker prototool generate /work
should compile them all.

We can just make sure our prototool.yaml is set up well, and run all the scripts from the docker image, and I think this will save a whole lot of nightmares with protoc setup and tool configurations.

This was referenced May 23, 2019
@husio
Copy link
Contributor Author

husio commented May 27, 2019

@ethanfrey this can be closed because your approach is working, correct?

@ethanfrey
Copy link
Contributor

Yes, protoc and prototools now work in docker.

@ethanfrey ethanfrey closed this May 27, 2019
@ethanfrey ethanfrey deleted the fix_protobuf_imports_issue_568 branch May 27, 2019 11:00
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.

Support Go modules: fix protobuf imports
2 participants